Update visual fire handling with TriState support #12303
Conversation
Replaced the Boolean-based visual fire system with TriState for improved clarity and flexibility, enabling three distinct states: TRUE, FALSE, and NOT_SET. Deprecated older methods in favor of new ones and updated internal handling to reflect these changes. Adjusted serialization and deserialization logic to accommodate the new TriState implementation.
Replaced the Boolean-based visual fire system with TriState for improved clarity and flexibility, enabling three distinct states: TRUE, FALSE, and NOT_SET. Deprecated older methods in favor of new ones and updated internal handling to reflect these changes. Adjusted serialization and deserialization logic to accommodate the new TriState implementation.
|
I don't think this should be a feature patch, also Paper comments are missing |
paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java
Outdated
Show resolved
Hide resolved
Just took another look at the code and I thought that visualFire was introduced by paper |
Even if the changes are in between Paper start/end comments, new ones should also be added to indicate what the new changes are doing |
|
|
||
| - if (this.hasVisualFire) { | ||
| - compound.putBoolean("HasVisualFire", this.hasVisualFire); | ||
| + if (!this.visualFire.equals(net.kyori.adventure.util.TriState.NOT_SET)) { |
There was a problem hiding this comment.
I don't think interfering with vanilla serialization output is a smart choice. Looking at other serializations, I think for such cases HasVisualFire would take this.visualFire.toBooleanOrElse(false) and a Paper.HasVisualFire would take the TriState, the paper one taking priority when deserializing
There was a problem hiding this comment.
The getBoolean method always returns false if there is nothing defined
but I agree, even though I don't like adding more values to the file
There was a problem hiding this comment.
I think I can actually leave it how I did it
the created compound is always empty, so vanilla actually only stores the visual Fire if it is enabled
I can just save it if it is not not_set
| - this.hasVisualFire = compound.getBoolean("HasVisualFire"); | ||
| + // Paper start - improve visual fire API | ||
| + this.visualFire = compound.contains("HasVisualFire") | ||
| + ? net.kyori.adventure.util.TriState.byBoolean(compound.getBoolean("HasVisualFire")) |
There was a problem hiding this comment.
this breaks old behavior, e.g. when loading a vanilla world or a world from an older build.
a custom Paper tag for visual fire TriState would be better here, like @masmc05 stated earlier
There was a problem hiding this comment.
why would this break old worlds?
it still uses the getBoolean while just using NOT_SET if the valua is not present
There was a problem hiding this comment.
isn't it possible for the value to be set as false in the older builds here?
There was a problem hiding this comment.
Technically no, it only writes the value if it is true
Replaced the Boolean-based visual fire system with TriState for improved clarity and flexibility, enabling three distinct states: TRUE, FALSE, and NOT_SET. Deprecated older methods in favor of new ones and updated internal handling to reflect these changes. Adjusted serialization and deserialization logic to accommodate the new TriState implementation.
Resolves #12084