Skip to content

Conversation

@dordsor21
Copy link
Member

@dordsor21 dordsor21 commented Jun 2, 2024

  • Removes possibility of writing to the LevelChunkSection biomes PalettedContainer whilst it is being read from the main thread

 - Removes possibility of writing to the LevelChunkSection biomes PalettedContainer whilst it is being read for sending packets
 - I believe this occured mostly on clipboard operations where blocks are written before biomes, so chunks are being sent whilst writing biomes
 - This would explain why the error reported in the below issue (and others) is/was so rare
 - Of course I could be completely wrong about all of this, but given the line in LevelChunkSection#write that the error seems to consistently occur on is when writing biomes to the packet, and that the only place I can find in FAWE where we write to a "live" PalettedContainer is for biomes, I am reasonably confident that this is the cause
 - Should address #2729
@dordsor21 dordsor21 requested a review from a team as a code owner June 2, 2024 16:17
@github-actions github-actions bot added the Bugfix This PR fixes a bug label Jun 2, 2024
@SirYwell
Copy link
Member

SirYwell commented Jun 8, 2024

From my understanding, the linked issue is caused by the fact that we manipulate the LevelChunk object. The ClientboundLevelChunkPacketData writes a chunk in two steps:

  1. Calculate a byte array, with a size calculated by calculateChunkSize(LevelChunk)
    • calls getSections() and calls LevelChunkSection#getSerializedSize() on each of the sections
  2. With this byte array, it now calls extractChunkData, and passes the chunk again
    • calls getSections() again and calls LevelChunkSection#write(...) on each of the sections

So if we change anything about the LevelChunk object in between those two steps (i.e. modifying the sections array, or replacing the biomes palette of a LevelChunkSection (solution proposed here), or writing to a biomes palette (current solution)) we might get a mismatch between the calculated size and the actual size it tries to write.

Am I missing something?

@dordsor21
Copy link
Member Author

Ah yeah that does make more sense. The changes here are ones that should probably go in anyway so I'll edit the PR desc

@dordsor21 dordsor21 requested a review from a team June 9, 2024 07:25
Comment on lines 141 to 148
Field tmpFieldBiomes;
try {
// It seems to actually be biomes, but is apparently obfuscated to "j"
tmpFieldBiomes = LevelChunkSection.class.getDeclaredField("biomes");
} catch (NoSuchFieldException ignored) {
tmpFieldBiomes = LevelChunkSection.class.getDeclaredField("j");
}
fieldBiomes = tmpFieldBiomes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops one more question, why isn't this using Refraction like the other fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// It seems to actually be biomes, but is apparently obfuscated to "j"

MiniMappingViewer said j but when actually using it, it was biomes for whatever reason

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you tested that on paper, you probably encountered the remapped version - but paper should also successfully remap j to biomes then

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but got fieldNotFound exception so idk what was up. Didn't bother to look into it further than this fixed it tbh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for me on 1.20.6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on 1.20.4, I also didn't bother checking every version, just assumed it could be an issue there as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also works on 1.20.4 for me, Refraction properly detects that it's running in a mojmapped environment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll stick it back to normal and assume it was something off just once

@dordsor21 dordsor21 requested a review from a team June 9, 2024 12:26
@NotMyFault NotMyFault merged commit af83b2f into main Jun 15, 2024
@NotMyFault NotMyFault deleted the fix/improved-biome-setting branch June 15, 2024 11:08
dordsor21 referenced this pull request Jun 15, 2024
dordsor21 pushed a commit that referenced this pull request Apr 26, 2025
Hacky workaround by overwriting the text3 StyleSerializer class with our own modified copy.
Note that the hover events "show_entity" and "show_item" aren't supported as they no longer take a rendered component, but just the NBT structure.

Behavior remains unchanged if 1.21.4- is detected.

Fixes #2756.
NotMyFault pushed a commit that referenced this pull request May 10, 2025
* Merge pull request EngineHub/WorldEdit#2740 from ssquadteam/version/7.3.x

Add ability to copy state string from info tool

Co-authored-by: ssquadteam <sahib.2009.sa@gmail.com>

* Update enum-like classes with 1.21.5-rc1 data from MCUtils

* Do not paste unsaveable entities in their default state (#2721)

Remove a redundant passenger check, as entity.save() returns false in that case.

This also causes leash knots to not be copied. I don't think this is a problem because:
- They would not be saved to disk, it's misleading for users that they appear.
- Pasted leashed mobs still think they're leashed to the original position and get unleashed* - no change in behaviour.
    \* Unless they're pasted close enough to the original position, in which case this has better behaviour because they create their own leash_knot entity.

* Update click and hover text component serialization for 1.21.5. (#2757)

Hacky workaround by overwriting the text3 StyleSerializer class with our own modified copy.
Note that the hover events "show_entity" and "show_item" aren't supported as they no longer take a rendered component, but just the NBT structure.

Behavior remains unchanged if 1.21.4- is detected.

Fixes #2756.

* Add text3 bukkit adapter override for Spigot gson change. (#2759)

* Make SpigotAdapter compatible with pre 1.21.5 api

---------

Co-authored-by: Octavia Togami <octavia.togami@gmail.com>
Co-authored-by: ssquadteam <sahib.2009.sa@gmail.com>
Co-authored-by: Maddy Miller <git@madelinemiller.dev>
Co-authored-by: brickmonster <92665597+brickmonster@users.noreply.github.com>
Co-authored-by: wizjany <wizjany@gmail.com>
Co-authored-by: SirYwell <hannesgreule@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugfix This PR fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants