Skip to content

Fix enchantment init#11624

Closed
Doc94 wants to merge 1 commit into
PaperMC:masterfrom
Doc94:feature/vanilla-enchantments-nullable
Closed

Fix enchantment init#11624
Doc94 wants to merge 1 commit into
PaperMC:masterfrom
Doc94:feature/vanilla-enchantments-nullable

Conversation

@Doc94
Copy link
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
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.

@kennytv
Copy link
Copy Markdown
Member

kennytv commented Nov 28, 2024

After more internal discussion, this is okay as a temporary solution for those that really need it - can you put this behind an unsupported config option (like allow-default-registry-removal)? and otherwise still throw.

Not 100% sure if the config is actually loaded before the Enchantment class init, so that'd be something you should verify first plz

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Dec 23, 2024

Based in the messages of hardfork a better solution require a little more of work currently dont know how to do correctly.

@Doc94 Doc94 closed this Dec 23, 2024
@Doc94 Doc94 deleted the feature/vanilla-enchantments-nullable branch January 8, 2025 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

5 participants