Skip to content

Add ChunkStatusChangeEvent#9921

Closed
Cryptite wants to merge 6 commits into
PaperMC:masterfrom
Cryptite:chunkstatuschangeevent
Closed

Add ChunkStatusChangeEvent#9921
Cryptite wants to merge 6 commits into
PaperMC:masterfrom
Cryptite:chunkstatuschangeevent

Conversation

@Cryptite
Copy link
Copy Markdown
Contributor

@Cryptite Cryptite commented Nov 8, 2023

Chunk Statuses are a wily thing, but it is nice to know when the state of one changes for concept like lazy-loading, etc.

I'm not sure this event should ever be anything other than read-only but this basic version has worked well for me for awhile.

Chunk Statuses are a wily thing, but it is nice to know when the state of one changes for concept like lazy-loading, etc.

I'm not sure this event should ever be anything other than read-only but this basic version has worked well for me for awhile.
@Cryptite Cryptite requested a review from a team as a code owner November 8, 2023 15:25
Comment on lines +65 to +70
+ public enum ChunkStatus {
+ INACCESSIBLE,
+ FULL,
+ BLOCK_TICKING,
+ ENTITY_TICKING;
+ }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cant we use Chunk$LoadLevel?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

kinda, but they messed up the order of them so it breaks the nice ordinal thing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, was nice for the isUpgrade() method

Comment thread patches/api/0446-ChunkStatusChangeEvent.patch
new file mode 100644
index 0000000000000000000000000000000000000000..7d446b6219f425aabdb6a7fbb587f89e8a0f23d8
--- /dev/null
+++ b/src/main/java/io/papermc/paper/chunk/ChunkStatusChangeEvent.java
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wrong package. you are missing a /event. i think this could go into io.papermc.paper.event.world or a sub-package of that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops

@Spottedleaf
Copy link
Copy Markdown
Member

The event is fired incorrectly. The chunk system changes the chunk status incrementally, and carefully handles concurrent requests to lower it. It is possible that the overall result of the operation is to lower the status, even if initially it was requested to be higher. You can see this by how the function updateCurrentState works.

You should look more carefully at the operations done in each status change to determine where to place the event. In general this area of code is the worst part of the chunk system and needs to be refactored, so it may not be possible to actually place an event in there without causing issues.

@Cryptite
Copy link
Copy Markdown
Contributor Author

Cryptite commented Nov 8, 2023

The event is fired incorrectly. The chunk system changes the chunk status incrementally, and carefully handles concurrent requests to lower it. It is possible that the overall result of the operation is to lower the status, even if initially it was requested to be higher. You can see this by how the function updateCurrentState works.

You should look more carefully at the operations done in each status change to determine where to place the event. In general this area of code is the worst part of the chunk system and needs to be refactored, so it may not be possible to actually place an event in there without causing issues.

I do see lower in handleFullStatusChange() that nextState is captured for subsequent potential status changes. Might be sufficient to just lower the event to after that that nextState.isOrAfter(currState) check? nextState isn't even used in that method otherwise...

@rayanbzd
Copy link
Copy Markdown

Hello, does any change are needed for this before being merged ? If yes, i can take time to do it.

@Warriorrrr Warriorrrr moved this from Awaiting review to Waiting For Author in Paper PR Queue Mar 5, 2025
@kennytv kennytv added the pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch label Mar 23, 2025
@kennytv kennytv deleted the branch PaperMC:master March 23, 2025 19:15
@kennytv kennytv closed this Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch pre-softspoon

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants