Skip to content

Add PlayerAdvancementCriterionRevokeEvent and PlayerAdvancementRevokeEvent - #14445

Open
FlowerDevelops wants to merge 9 commits into
PaperMC:mainfrom
FlowerDevelops:advancement-revoke-events
Open

Add PlayerAdvancementCriterionRevokeEvent and PlayerAdvancementRevokeEvent#14445
FlowerDevelops wants to merge 9 commits into
PaperMC:mainfrom
FlowerDevelops:advancement-revoke-events

Conversation

@FlowerDevelops

@FlowerDevelops FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown

No description provided.

@FlowerDevelops
FlowerDevelops requested a review from a team as a code owner November 1, 2026 00:16
@github-project-automation github-project-automation Bot moved this to Awaiting review in Paper PR Queue Nov 1, 2026
@FlowerDevelops FlowerDevelops mentioned this pull request Nov 1, 2026
Open
ImKarven
ImKarven suggested changes Nov 1, 2026
View reviewed changes

@ImKarven ImKarven left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PlayerAdvancementRevokeEvent should also have changes similar to PlayerAdvancementRevokeCriterionEvent.

Javadoc should be added to getAdvancement() (for both events) and getCriterion().

Actually, the firing logic for PlayerAdvancementRevokeCriterionEvent should be moved to PlayerAdvancements#revoke so that the event is also fired when a plugin revoke an advancement through the API.

Comment thread ...r-api/src/main/java/io/papermc/paper/event/player/PlayerAdvancementCriterionRevokeEvent.java Resolved
Comment thread ...r-api/src/main/java/io/papermc/paper/event/player/PlayerAdvancementCriterionRevokeEvent.java Resolved
Comment thread ...r-api/src/main/java/io/papermc/paper/event/player/PlayerAdvancementCriterionRevokeEvent.java Resolved
Comment thread ...r-api/src/main/java/io/papermc/paper/event/player/PlayerAdvancementCriterionRevokeEvent.java Resolved
Comment thread paper-server/patches/sources/net/minecraft/server/commands/AdvancementCommands.java.patch Resolved
Comment thread paper-server/patches/sources/net/minecraft/server/commands/AdvancementCommands.java.patch Resolved
@github-project-automation github-project-automation Bot moved this from Awaiting review to Changes required in Paper PR Queue Nov 1, 2026
@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

I'll check all comment and change my code.

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

I made 2 commits that should fix this

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

If there are any changes you'd still want to see, tell me

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

Should all be fixed and working!

@valaphee

valaphee commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Should all be fixed and working!

Your patch seem to be gone

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

Should all be fixed and working!

Your patch seem to be gone

Hm? What patch is gone?

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

That's weird, 1s

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

Let me fix this, really weird

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

I ran

11:37:46 AM: Executing ':paper-server:fixupSourcePatches'…

Reusing configuration cache.

> Task :paper-server:fixupSourcePatches
[main 4da20a5] fixup! paper File Patches
 1 file changed, 23 insertions(+), 2 deletions(-)

BUILD SUCCESSFUL in 728ms
1 actionable task: 1 executed
Configuration cache entry reused.
Updated tag 'file' from 3ca688bba77b2a62e71c05d471959649cd2fb623 to 6a96fab52a9fc83ed9f35166fc40f6ebd9a6e75a
Successfully rebased and updated refs/heads/main.
11:37:47 AM: Execution finished ':paper-server:fixupSourcePatches'.

But for some reason it deleted the patch?

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

Yeah okay, forgot to rebuild patches 😓

Doc94
Doc94 requested changes Nov 1, 2026
View reviewed changes

@Doc94 Doc94 left a comment

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.

Missing docs in methods for the new events

Comment thread paper-server/patches/sources/net/minecraft/server/PlayerAdvancements.java.patch Resolved
@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

Missing docs in methods for the new events

I dont know how to make docs

@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

My first PR as you've probably noticed

@Machine-Maker Machine-Maker linked an issue Nov 1, 2026 that may be closed by this pull request
Add PlayerAdvancementRevokeEvent and PlayerAdvancementCriterionRevokeEvent #13799
Open
@FlowerDevelops

FlowerDevelops commented Nov 1, 2026

Copy link
Copy Markdown
Author

Did some research and added Javadocs

@Doc94

Doc94 commented Nov 2, 2026

Copy link
Copy Markdown
Member

Okay im gonna check and maybe make a few changes.

Doc94
Doc94 reviewed Nov 2, 2026
View reviewed changes
Comment thread paper-server/patches/sources/net/minecraft/server/PlayerAdvancements.java.patch
Comment on lines +78 to +85
+ if (wasDone && !progress.isDone()) {
+ // Paper start - Add PlayerAdvancementRevokeEvent
+ final io.papermc.paper.event.player.PlayerAdvancementRevokeEvent event = new io.papermc.paper.event.player.PlayerAdvancementRevokeEvent(this.player.getBukkitEntity(), advancement.toBukkit());
+ if (!event.callEvent()) {
+ progress.grantProgress(criterion);
+ return false;
+ }
+ // Paper end - Add PlayerAdvancementRevokeEvent

@Doc94 Doc94 Nov 2, 2026

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.

Okay i check this... not sure about a good case for cancel (the Done event not has that because for this its better handle criterion rather than the whole "advancement)
and when test this event its not fired in the final of the criterion remove not sure if that is the correct behaviour....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Doc94 Doc94 Doc94 requested changes
@ImKarven ImKarven Awaiting requested review from ImKarven

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Paper PR Queue
Status: Changes required

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add PlayerAdvancementRevokeEvent and PlayerAdvancementCriterionRevokeEvent

4 participants

@FlowerDevelops @valaphee @Doc94 @ImKarven