feat: scrolling astral map screen#2003
Conversation
|
ill test now |
fix: removal of biome synchronisation (data is already available on the client) fix: translation key for biome and structure not found
b5d8225 to
3f447d5
Compare
|
my intellij is bugging out i cant test |
Will be in like 1hr or smth cus I'm not on my pc rn |
Addi3
left a comment
There was a problem hiding this comment.
LGTM. Tested in game, locates structures and biomes fine, no noticeable issues with the GUI either and scales well with game window
|
|
||
| private DisplayedCategories shownCategory = DisplayedCategories.BOTH; | ||
| private final List<Entry> entries = new ArrayList<>(); | ||
| private final Map<String, String> mods = new HashMap<>(); |
There was a problem hiding this comment.
Wouldn't it be better if this was static? This way it doesn't have to re-think what the mods' names are. It could be lazily initialized to avoid querying mods for no reason.
| if (shownCategory != DisplayedCategories.STRUCTURES) { | ||
| for (Identifier id : client.world.getRegistryManager().get(RegistryKeys.BIOME).getIds()) { | ||
| this.entries.add(new Entry(id, true)); | ||
| } | ||
| } | ||
| if (shownCategory != DisplayedCategories.BIOMES) { | ||
| for (Identifier id : AstralMapBlock.structureIds) { | ||
| this.entries.add(new Entry(id, false)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is rather confusing and will become more complicated if more filters are added.
I suggest the following:
| if (shownCategory != DisplayedCategories.STRUCTURES) { | |
| for (Identifier id : client.world.getRegistryManager().get(RegistryKeys.BIOME).getIds()) { | |
| this.entries.add(new Entry(id, true)); | |
| } | |
| } | |
| if (shownCategory != DisplayedCategories.BIOMES) { | |
| for (Identifier id : AstralMapBlock.structureIds) { | |
| this.entries.add(new Entry(id, false)); | |
| } | |
| } | |
| boolean both = DisplayedCategories.BOTH; | |
| if (shownCategory == DisplayedCategories.BIOMES || both) { | |
| for (Identifier id : client.world.getRegistryManager().get(RegistryKeys.BIOME).getIds()) { | |
| this.entries.add(new Entry(id, true)); | |
| } | |
| } | |
| if (shownCategory == DisplayedCategories.STRUCTURES || both) { | |
| for (Identifier id : AstralMapBlock.structureIds) { | |
| this.entries.add(new Entry(id, false)); | |
| } | |
| } |
Alternatively, the enum could be replaced with a bitset (be it a normal int or BitSet, effectively allowing to combine values).
Alternatively no.2, you could add an abstract method to the enum that would supply the entries, something like this:
public enum DisplayedCategories {
STRUCTURES {
@Override
public void getEntries(Consumer<AstralMapListWidget.Entry> consumer, MinecraftClient client) {
for (Identifier id : AstralMapBlock.structureIds) {
consumer.accept(new Entry(id, false));
}
}
},
BIOMES {
@Override
public void getEntries(Consumer<AstralMapListWidget.Entry> consumer, MinecraftClient client) {
for (Identifier id : client.world.getRegistryManager().get(RegistryKeys.BIOME).getIds()) {
consumer.accept(new Entry(id, true));
}
}
},
BOTH {
@Override
public void getEntries(Consumer<AstralMapListWidget.Entry> consumer, MinecraftClient client) {
for (DisplayedCategories category : DisplayedCategories.VALUES) {
category.getEntries(consumer, client);
}
}
};
private static final DisplayedCategories VALUES = values();
public abstract void getEntries(Consumer<AstralMapListWidget.Entry> consumer, MinecraftClient client);
}
// ...
// in #refreshEntries:
this.shownCategory.getEntries(this.entries::add, client);| public static String identifierToName(Identifier id) { | ||
| try { | ||
| return WorldUtil.fakeTranslate(id.getPath()); | ||
| } catch (Exception e) { | ||
| return id.toString(); | ||
| } | ||
| } |
There was a problem hiding this comment.
WorldUtil#fakeTranslate shouldn't throw exceptions. If it does, please wrap it in a try-catch.
There was a problem hiding this comment.
I just left this bit as is from @duzos initial astral map commit
| private final Text text; | ||
| private final String modName; | ||
|
|
||
| public Entry(Identifier identifier, boolean isBiome) { |
There was a problem hiding this comment.
Having a hardcoded isBiome is kinda bad, since there could be other types added in future that could also have a translation.
In this scenario, I think the enum abstract method option I've mentioned previously would work the best, since then you could pass the Text directly to the Entry, instead of the Entry coming up with it.
There was a problem hiding this comment.
Generally I would agree with you but what other types? I feel like the enum abstract method complicates things a bit too much for a hypothetical
There was a problem hiding this comment.
Unless you have something in mind?
There was a problem hiding this comment.
It doesn't complicate stuff that much, plus it allows to add other categories more easily. For example, we could add the home location as a category. Or, perhaps, the waypoints stored in waypoint banks.
Obviously, out of scope for this PR, but doing the enum abstract method and passing Texts directly would make it implementable without many changes.
There was a problem hiding this comment.
It doesn't complicate stuff that much, plus it allows to add other categories more easily. For example, we could add the home location as a category. Or, perhaps, the waypoints stored in waypoint banks.
never happening btw :al_clueless:
| boolean isStructure = buf.readBoolean(); | ||
|
|
||
| if (isBiome) { | ||
| handleBiomeRequest(player, target); | ||
| } else { | ||
| if (isStructure) { | ||
| handleStructureRequest(player, target); | ||
| } else { | ||
| handleBiomeRequest(player, target); |
|
LARP final boss |
About the PR
Why / Balance
Better for usability
Technical details
Media
Requirements
Breaking changes
No more IdentifierSwitcher or AstralMapBlock.biomeIds but they weren't used by anything else anyway
Changelog