ExperienceOrb stacking results in XP loss on item repair

[Machine-Maker]

Expected behavior

Stacked XP orbs (nbt value of Count > 1) give the correct amount of item repair.

Observed/Actual behavior

Stacked orbs are treated as not stacked, resulting in the loss of Count * Value XP used for repairing items.

Steps/models to reproduce

1. /give @s minecraft:diamond_chestplate{Damage:100}
2. /enchant @s minecraft:mending 1
3. Fly up and run /summon minecraft:experience_orb ~ ~-5 ~ {Count: 2, Value: 5}
4. Observe durability of diamond chestplate (F3 + H)
5. Collect the XP orb
6. Re-observe the durability, it should have increased by 10.

Repeat this on a vanilla server or singleplayer world, the durability will increase by 20.

Plugin and Datapack List

none

Paper version

f9f9079

Other

This was broken by upstream in a "fix":

Original Report: https://hub.spigotmc.org/jira/browse/SPIGOT-6708
PR: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/pull-requests/926/overview

---

One solution is to just not change the value field on ExperienceOrb. But that causes the above bug. So an alternative is needed to expose whatever that issue is asking for. Maybe @Phoenix616 can chime in.

Could alternatively reset the value field after handling the repair logic. I don't really like this because orb.getValue() doesn't accurately track the value of the orb.

[Machine-Maker]

Just realized this also affects the merging on spawn logic as well. It does not account for the count of orbs being different. For example, if you have an orb in the world with {Count: 2, Value: 5} (10 xp) and you spawn an orb with {Count: 1, Value: 5} (5 xp), it will merge the two orbs, creating {Count: 2, Value: 10} which is a 20 xp orb, which is not the sum of 5 and 10.

[Phoenix616]

Not 100% sure but I remember something about the count only being meant for the visual display of the orb as stacked and the value simply being the sum of all stacked orbs. (Which would invalidate this ticket as "works as intended")

Otherwise this whole mechanic of stacking orbs with different values would be pretty much impossible with the current approach as it would require a Value->Count map being stored.

[Machine-Maker]

Not 100% sure but I remember something about the count only being meant for the visual display of the orb as stacked and the value simply being the sum of all stacked orbs. (Which would invalidate this ticket as "works as intended")

It'd be WAI that two identical XP orbs give different repair amounts on vanilla and paper? That seems not possible. If they are different it's pretty much by definition not WAI. The count is not used for just visuals, and it only applies to orbs of the same value but separated by 40 in entity ID count.

That's why the function became recursive and doesn't modify the value field on the XP orb, because the value field shouldn't change because of the count is > 1, it has to use up the whole amount "count" many times.

[Phoenix616]

Yeah you are right, it's definitely used. I might have thought of the pre 1.17 merging code.

I think the main issue here was that there was no good way of exposing (and changing) the amount of exp used for the repairing in the event itself and this change made it compatible with how it behaved pre 1.17 (and also fixed an exploit which allowed infinite repairs). I don't think I ever thought about how merged exp orbs behaved here as that wasn't part of Vanilla pre 1.17 but handled by Spigot itself in a different way. (it simply added to the value of the orb, not using a separate count field)

I think my original change can basically be reverted as long as it's possible in some way to get and edit the used exp amount in the event itself (and the exploit is fixed, not sure if Vanilla got to that at any point)

I guess one solution would be to revert to the pre 1.17 merging behaviour of directly adding to the value and not using the count but that would of course remove the Vanilla feature of having a count on the experience orb entity itself.

[Machine-Maker]

I think my original change can basically be reverted as long as it's possible in some way to get and edit the used exp amount in the event itself

What about #7382? It basically adds a way to set in the event the function that will be used to calculate how much XP is used up for the set repair amount. Can plug in the repair amount from the event to that function and get the XP used.

[Phoenix616]

That seems like a good solution to the problem of adjusting the repair amount to exp relation, yeah. (But that would of course address the weird behaviour regarding repairs not taking exp that I noticed back then, but I would have to look into if that is indeed still the case)

[DeliveryAagent]

Hey there,

Sorry to interrupt,
Will this PR be pushed through Folia too? Not sure how many code is shared by Paper and Folia.

Thanks

[lynxplay]

Yes, folia is a maintained fork of paper.
If the PR is merged into paper, folia will pull the changes a bit afterwards.