Refactor property value retrieval in DynmapPlugin - #4273
Open
filipkober wants to merge 1 commit into
Open
Conversation
- Added a new method `getPropertyValueName` to retrieve the serialized name of a property's value within a block state, ensuring compatibility with modded enums. - Updated instances where property values are appended to state names to use the new method instead of directly calling `toString()`, improving accuracy in rendering block states.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use serialized property names when building block state names
Problem
Modded blocks with enum properties whose Java constant names differ from their
serialized names are silently rendered as air. No warning is logged, no texture is
reported missing, and no amount of renderdata will reach them.
DynmapPlugin.initializeBlockStates()records a name for every block state:For an enum property,
Object#toString()returns the Java constant name. The chunkNBT palette stores the serialized name.
GenericMapChunkCachebuilds its lookup keyfrom the NBT and matches it against the recorded name in
DynmapBlockState.getStateByNameAndState(), which lowercases and compares.Vanilla survives this because vanilla enum constants are just uppercased serialized names
(
NORTH/"north",NORTH_SOUTH/"north_south"). Any mod enum where they divergedoes not match.
The failure is silent and permanent for the session:
getStateByNameAndStatereturnsAIRrather thannullon a miss, so theif (palette[pi] == null) palette[pi] = getBaseStateByName(pname);fallback in thepalette loop never fires, and the miss is cached. The block is gone before anything
consults a texture map, which is why it never shows up in the
dump-missing-blocksreport or the verbose
no texture mappingoutput.Reproduction
TerraFirmaCraft's
Flowenum, used bytfc:fluid/river_water:Lowercased constant name vs serialized name:
NEE→nee,NNE→nne,NNW→nnw,NWW→nwwEEE→eee≠eSWW→sww,SSW→ssw,SSE→sse,SEE→seeN_E→n_e≠neNNN→nnn≠n___→___≠noneExactly the eight three-letter flow values render. Every one-letter, two-letter and
nonevalue becomes air, producing holes in the middle of rivers that follow the flowfield rather than any obvious terrain feature. TFC leaves were affected too, via a
separate enum property, which went unnoticed until this was fixed.
Fix
Use the property's own name-for-value method, which resolves to the serialized name for
enum properties and to
toString()for boolean and integer properties, where the oldbehaviour was already correct.
Property#getName(T)Property#name(T)A small generic helper is needed because
bs.getProperties()yieldsProperty<?>, andtwo separate uses of the same wildcard in one expression capture independently. The
helper binds the capture once.
net.minecraft.Util#getPropertyNamein vanilla exists forthe same reason.
The Bukkit helpers are unaffected. They already derive state names by parsing
IBlockData#toString(), which goes throughStateHolder#toString()and therefore usesserialized names.
Scope
20 files,
forge-1.14.4throughforge-1.21.11andfabric-1.14.4throughfabric-1.21.9-10.forge-1.14.4andforge-1.15.2iterate a rawIProperty, so nohelper is required there and the call is inlined.
Verification
forge-1.20(Minecraft 1.20.1, Forge 47.4.13) built fromv3.7-beta-6and run on aTerraFirmaGreg-Modern server. All river water flow states render after a full render;
previously only eight of seventeen did. Other TFC blocks that were silently missing also
came back.
The remaining 19 modules are the same edit against their respective mappings and have not
been individually built. CI coverage on those would be welcome.