ChunkEvent.Unload is called before ChunkDataEvent.Save

[Adam8234]

Minecraft Version: 1.20.1

Forge Version: 47.1.76

Steps to Reproduce:

https://github.com/neoforged/NeoForge/blob/0974ecc143f449087139a6923aa3f0823dce421d/patches/minecraft/net/minecraft/server/level/ChunkMap.java.patch#L15

I suggest the patch needs to be updated to move the ChunkEvent.Unload to be after save() probably near the level.unload() in the actual src

Description of issue:

What is the correct lifecycle for Chunks?

Right now it's ChunkEvent.Load -> ChunkDataEvent.Load -> ChunkEvent.Unload -> ChunkDataEvent.Save

I use these events to keep a Map of loaded Levels and a Map of all Chunks loaded, and save data that mapped data via these ChunkData Events. Having this mapped relationship makes it crucial for some functionality - This is similar to CodeChickenLib's Legacy WorldExtension and ChunkExtension here

I believe the lifecycle should be ChunkEvent.Load -> ChunkDataEvent.Load -> ChunkDataEvent.Save -> ChunkEvent.Unload and here's why:

In this scenario, when a chunk is unloaded, I effectively remove the ChunkExtension from the map. With the current lifecycle, that will be done before the Save event is fired and thus data that should be saved in the ChunkExtension isn't.

It's not like I can just remove the ChunkEvent.Unload functionality on my end, because that would cause major memory leaks.

[Technici4n]

If you want to store data in chunks, capabilities are the recommended solution.

[TelepathicGrunt]

@Technici4n should this then be closed if Data Attachments are better for this use case?

Though looking at MC's code, they do call the save method before the unload method

Was there a specific reason the unload is put before save instead of right after the save call? Since after would be closer to where vanilla does the actual unload call?