Skip to content
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

Fix enchantment init #11624

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Doc94
Copy link
Contributor

@Doc94 Doc94 commented Nov 17, 2024

Fixs #11623 like Tags enchantments (and pretty sure many other Registry things) can be removed by datapacks and how CrafBukkit (for legacy) expects vanilla ever exists can cause issues...
This PR just make the Vanillla Enchantments can be null in the Bukkit Enchantment class also mention in docs about this (more "easy" than add Nullable to all the static instances of Enchantment with the same issue (or maybe this can be better)

@Doc94 Doc94 requested a review from a team as a code owner November 17, 2024 14:32
Copy link
Member

@electronicboy electronicboy left a comment

Choose a reason for hiding this comment

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

This is potentially going to be fun if nothing is expecting those fields to be nullable, but, the reality here is that this is somewhat unavoidable

@kennytv
Copy link
Member

kennytv commented Nov 19, 2024

There is one concern that removing enchantments will break even in vanilla somewhere down the line, as is the case with many other client synchronized registries making the client crash once it does try to reference individual ones somewhere in game logic

@Doc94
Copy link
Contributor Author

Doc94 commented Nov 19, 2024

There is one concern that removing enchantments will break even in vanilla somewhere down the line, as is the case with many other client synchronized registries making the client crash once it does try to reference individual ones somewhere in game logic

hmm exists a form to check this? i mean the datapack allow make this in vanilla if a issue happen is more a report for the MOJIRA? if they allow make this

@kennytv
Copy link
Member

kennytv commented Nov 24, 2024

Okay for enchantments specifically I don't see such use, so it might be okay to remove them as long as all loot tables etc. that reference the enchantments are removed by the datapack? but it makes for weird api here, if we merge this we just have to make sure it's very clear they can be null

@lynxplay
Copy link
Contributor

At least my two cents on this.
Given we cannot expand this to all data driven types (see damage type), I am unsure if pulling such a move is smart for our API as the Enchantment value would be the only one that could be null.

Additionally, removing vanilla enchants is also a rather edge-case imo.
You still have to manually remove from them from everything else so I think the workaround of "remove them from everywhere but the registry" is a fine one to make to preserve the API here.

More opinions obviously needed tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting final testing
Development

Successfully merging this pull request may close these issues.

4 participants