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

EnchantmentType - prepare for MC 1.21 custom enchantments #6687

Merged
merged 29 commits into from
Jul 1, 2024

Conversation

ShaneBeee
Copy link
Contributor

@ShaneBeee ShaneBeee commented May 11, 2024

Description

This PR aims to prepare Skript for future changes with Enchantments.
Minecraft has moved enchantments to be data driven in 1.21.
This also means the ability to create custom enchantments.
This PR helps prepare Skript for that change by using the Bukkit registry rather than hard coding and using the specific fields.

Mojang is planning a 1.21 release for sometime this summer. I figured it would be best to prepare ourselves now.

NOTES:

  • parsing will first attempt to grab from lang file
  • if parsing fails from lang file, we move on to get from key from Registry (this allows for parsing custom enchantments)
  • toString will first attempt to find the a name from the lang file (ex: "Curse of Vanishing 1")
  • if that fails, will send the key for a vanilla enchantment (ex: "vanishing_curse") or the full namespace for custom enchants (ex: "my_thing:explosive")
  • Static Enchantment related methods/fields have been moved to EnchantmentUtils

Target Minecraft Versions: 1.21+
Requirements: none
Related Issues: none

@APickledWalrus
Copy link
Member

APickledWalrus commented May 13, 2024

I might prefer to keep support for custom names for Minecraft-provided enchantments. That is, one could write minecraft:vanishing_curse or curse of vanishing. This would keep the process automated but allow us to provide support for the typical representation of an enchantment's name. I'm not a super big fan of just vanishing_curse or vanishing curse (i think requiring the namespace is better when not using a Skript-provided name alias)

@ShaneBeee
Copy link
Contributor Author

I might prefer to keep support for custom names for Minecraft-provided enchantments. That is, one could write minecraft:vanishing_curse or curse of vanishing. This would keep the process automated but allow us to provide support for the typical representation of an enchantment's name. I'm not a super big fan of just vanishing_curse or vanishing curse (i think requiring the namespace is better when not using a Skript-provided name alias)

thats probably a good idea, for a little backwards compatibility too

@sovdeeth
Copy link
Member

Seconding pickle
Always have the key-based names, but also allow custom names via .lang

@ShaneBeee ShaneBeee marked this pull request as ready for review May 13, 2024 19:30
ShaneBeee and others added 2 commits May 13, 2024 13:04
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
- skip adding names if not in lang file
- toString should return full namespaced key if no name found
@ShaneBeee ShaneBeee requested a review from APickledWalrus May 13, 2024 22:23
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Looks good. May be wise to wait on #6695 if that implementation can be used here instead.

@sovdeeth sovdeeth added enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. 2.9 Targeting a 2.9.X version release labels May 31, 2024
@ShaneBeee ShaneBeee marked this pull request as draft June 9, 2024 01:43
@ShaneBeee
Copy link
Contributor Author

Converted back to draft.
Once my registry PR is merged, I'll rework this a bit to work with that also.

@ShaneBeee ShaneBeee marked this pull request as ready for review June 21, 2024 23:16
@ShaneBeee
Copy link
Contributor Author

Ok, back to being ready!

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

I would like to see use the new Registry classes when possible, but we can discuss the feasibility.

@sovdeeth sovdeeth requested a review from APickledWalrus July 1, 2024 19:59
@sovdeeth sovdeeth merged commit 5c3074e into SkriptLang:dev/feature Jul 1, 2024
5 checks passed
@ShaneBeee ShaneBeee deleted the feature/enchantment_prep branch July 1, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 Targeting a 2.9.X version release enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants