fix: improve biome setting to avoid writing directly to chunk #2757
Conversation
dordsor21
commented
Jun 2, 2024
•
dordsor21
commented
- 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
|
From my understanding, the linked issue is caused by the fact that we manipulate the
So if we change anything about the Am I missing something? |
|
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 |
| 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; |
There was a problem hiding this comment.
Whoops one more question, why isn't this using Refraction like the other fields?
There was a problem hiding this comment.
// 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
There was a problem hiding this comment.
If you tested that on paper, you probably encountered the remapped version - but paper should also successfully remap j to biomes then
There was a problem hiding this comment.
Yep, but got fieldNotFound exception so idk what was up. Didn't bother to look into it further than this fixed it tbh
There was a problem hiding this comment.
I was on 1.20.4, I also didn't bother checking every version, just assumed it could be an issue there as well
There was a problem hiding this comment.
It also works on 1.20.4 for me, Refraction properly detects that it's running in a mojmapped environment
There was a problem hiding this comment.
I'll stick it back to normal and assume it was something off just once
