Skip to content

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Aug 4, 2025

Problem

There was no way of replicating the behavior of the vanilla enchanting table without hardcoding weights and chances manually.

Solution

Adds an option to EffEnchant to use ItemFactory#enchantWithLevels (available on paper 1.20.4+), which gives an itemstack a random set of appropriate enchants based on the given level, identical to the behavior of the enchanting table. I did not add the TagKey-based enchant limiting option as I didn't want to get into implementing the key set system and the only option to do it somewhat nicely seems to be reflection.

Testing Completed

manual confirmation
image

Supporting Information

Enchanting a book at a level returns an enchanted book instead. Support for those should also be added, but is out of scope of this PR.


Completes: none
Related: none

@sovdeeth sovdeeth requested a review from a team as a code owner August 4, 2025 05:19
@sovdeeth sovdeeth requested review from Efnilite and Absolutionism and removed request for a team August 4, 2025 05:19
@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Aug 4, 2025
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Aug 4, 2025
@sovdeeth sovdeeth moved this to In Review in 2.13 Releases Aug 4, 2025
@sovdeeth sovdeeth changed the base branch from master to dev/feature August 4, 2025 05:30
@github-project-automation github-project-automation bot moved this from In Review to Awaiting Merge in 2.13 Releases Aug 5, 2025
@skriptlang-automation skriptlang-automation bot added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed needs reviews A PR that needs additional reviews labels Aug 5, 2025
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.

Couple minor thoughts+questions

}
case ENCHANT_AT_LEVEL -> {
ItemFactory factory = Bukkit.getItemFactory();
Number levelValue = level.getSingle(event);
Copy link
Member

Choose a reason for hiding this comment

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

Could maybe do getOptionalSingle with -1 as the default value:

Suggested change
Number levelValue = level.getSingle(event);
int level = this.level.getOptionalSingle(event).orElse(-1).intValue();

Then you simply check level < 0 as you do now

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that just makes the code less readable, honestly. Without the optional it's very clear what cases cause failure, but with it you have to keep in mind that -1 is for both null values and truly negative ones.

@github-project-automation github-project-automation bot moved this from Awaiting Merge to In Review in 2.13 Releases Aug 21, 2025
@skriptlang-automation skriptlang-automation bot removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants