-
-
Notifications
You must be signed in to change notification settings - Fork 40
Item updater #871
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.
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
Open
Huskydog9988
wants to merge
24
commits into
CivMC:main
Choose a base branch
from
Huskydog9988:item-updater
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Item updater #871
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
+ refactors ItemMetaConverterHack
This means functions can predicate potentially expensive followup code (like updating the client about something) on whether there's something to update.
This was considered too speculative. Unfortunately, there's really no other way to achieve the same type-limiting goal (ChatGPT tells me this concept is also called "Programming to an Interface", "Information Hiding", and "Minimized Coupling") without delving into NMS, which *does* have the interface (weird how PaperMC doesn't). This does means that migrators can cast the NMS DataComponentHolder back to an NMS ItemStack... but then again migrators could have reflectively accessed the internal item within ItemDataComponentHolder, so 🤷♂️.
I was basically fretting over this because I knew, I just knew, that someone would write an ItemStack migration that's chock full of ItemUtils calls instead of a ItemMeta migration with MetaUtils calls. That said, even though I do honestly believe that a migration context should be more limited, it's still nonetheless an abstraction on top of ItemUpdater, which unashamedly updates ItemStacks. That said, I've annotated the ItemStack migration interface as experimental and denied it a convenient registration shortcut as a form of "friction as a deterrent".
This will help with doing befores/afters with item updates
This uses the event-driven nature of ItemUpdater and the default implementations, effectively using Bukkit's event system as a pseudo itemUpdater registry. Which is a bit silly but preferable to the alternative.
Per request, this has been removed as unnecessary, that the HashMap overhead is fine.
Turns out the Bukkit method is pretty convenient.
The "items" package was *right there*, why wasn't the "CustomItem" class put in there? 😭
Genuinely cannot wait for JEP Draft 8303099 to be merged 🫠
Plus added a deliberate NMS item migration option
This removes the impl package
This will allow custom items and compact items to have independent version values.
Protonull
reviewed
Oct 24, 2025
| ); | ||
|
|
||
| // TODO: Switch this to PaperMC DataComponentHolder on 1.21.5 | ||
| non-sealed interface OfData extends ItemMigration<DataComponentHolder> { |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PaperMC has since created their own version, available at io.papermc.paper.datacomponent.DataComponentHolder
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reopening #754 in preparation of custom item rework