Skip to content

Fix CombatTracker stale on death#12562

Merged
kennytv merged 3 commits into
PaperMC:mainfrom
Doc94:fix/combattracker-override-for-players
May 21, 2025
Merged

Fix CombatTracker stale on death#12562
kennytv merged 3 commits into
PaperMC:mainfrom
Doc94:fix/combattracker-override-for-players

Conversation

@Doc94
Copy link
Copy Markdown
Member

@Doc94 Doc94 commented May 18, 2025

Fix #12558 where player replace the combattracker on death, this PR make the PaperCombatTrackerWrapper part of the NMS CombatTracker for avoid stale instances.

@Doc94 Doc94 requested a review from a team as a code owner May 18, 2025 17:46
@github-project-automation github-project-automation Bot moved this to Awaiting review in Paper PR Queue May 18, 2025
@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented May 18, 2025

not sure also if maybe is better just make the get in the Craft clalss build the wrapper every time for avoid this edge case. but the fixs works.

@lynxplay
Copy link
Copy Markdown
Contributor

Yea I think recreating the instance is a fine alternative.
The current behaviour is gonna run into issues once we (finally) stop reusing the ServerPlayer instance and would need to be re-migrated.

Alternatively, the nms CombatTracker could hold its own API view (similar to Entity's getBukkitEntity) and we return that.
A single record creation there should really not be a big deal tho, no state is copied in the creation, all methods are stateless.

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented May 18, 2025

Yea I think recreating the instance is a fine alternative.
The current behaviour is gonna run into issues once we (finally) stop reusing the ServerPlayer instance and would need to be re-migrated.

Alternatively, the nms CombatTracker could hold its own API view (similar to Entity's getBukkitEntity) and we return that.
A single record creation there should really not be a big deal tho, no state is copied in the creation, all methods are stateless.

Then i can add the wrapper in the nms class like the bukkit entity and use that.

@JustDoom
Copy link
Copy Markdown

Was there anything else that needed to be added or is it just waiting for a review?

@github-project-automation github-project-automation Bot moved this from Awaiting review to Awaiting final testing in Paper PR Queue May 21, 2025
@kennytv kennytv merged commit 87349c3 into PaperMC:main May 21, 2025
@github-project-automation github-project-automation Bot moved this from Awaiting final testing to Merged in Paper PR Queue May 21, 2025
@Doc94 Doc94 deleted the fix/combattracker-override-for-players branch September 7, 2025 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

A player's combat tracker becomes stale on death

5 participants