Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ChunkEvent.Unload is called before ChunkDataEvent.Save #127

Open
Adam8234 opened this issue Sep 3, 2023 · 2 comments · May be fixed by #1778
Open

ChunkEvent.Unload is called before ChunkDataEvent.Save #127

Adam8234 opened this issue Sep 3, 2023 · 2 comments · May be fixed by #1778
Labels
1.20 Targeted at Minecraft 1.20 bug A bug or error triage Needs triaging and confirmation

Comments

@Adam8234
Copy link

Adam8234 commented Sep 3, 2023 •
edited
Loading

Minecraft Version: 1.20.1

Forge Version: 47.1.76

Steps to Reproduce:

NeoForge/patches/minecraft/net/minecraft/server/level/ChunkMap.java.patch

Line 15 in 0974ecc

+ net.minecraftforge.common.MinecraftForge.EVENT_BUS.post(new net.minecraftforge.event.level.ChunkEvent.Unload(p_203002_));

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
Copy link
Member

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

@sciwhiz12 sciwhiz12 added bug A bug or error 1.20 Targeted at Minecraft 1.20 triage Needs triaging and confirmation labels Sep 17, 2023
@TelepathicGrunt
Copy link
Contributor

TelepathicGrunt commented May 17, 2024 •
edited
Loading

@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
image

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?

Shadows-of-Fire added a commit to Shadows-of-Fire/NeoForge that referenced this issue Dec 15, 2024
@Shadows-of-Fire
Move firing point of ChunkEvent.Unload to after chunk save
66df3f2 
Closes neoforged#127
@Shadows-of-Fire Shadows-of-Fire linked a pull request Dec 15, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1.20 Targeted at Minecraft 1.20 bug A bug or error triage Needs triaging and confirmation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[1.21.x] Move firing point of ChunkEvent$Unload to after chunk save Shadows-of-Fire/NeoForge
4 participants
@Adam8234 @Technici4n @sciwhiz12 @TelepathicGrunt