Enchantment api #291
Enchantment api #291
Conversation
- Added support for customizable item groups for custom enchantment books
- Added more powerful "enchantment targets"
- Enchantment weight can now be defined per level/context
- Developers have access to the full world, player, enchantment power, etc.
- Added a test mod illustrating a custom enchantment target for only hoes
- Does not include support for more a powerful anvil application context
- Is in a very rough stage and will be changed significantly before stable
...nt_other/enchantment/src/main/java/org/quiltmc/qsl/enchantment/impl/EnchantmentGodClass.java
Outdated
Show resolved
Hide resolved
ix0rai
added
new: module
| /** | ||
| * Determines whether the provided enchantment can be applied to this item. | ||
| * <p> | ||
| * This takes highest priority for applying enchantments. | ||
| * @param stack the stack containing this item | ||
| * @param enchantment the enchantment to apply to this item | ||
| * @return {@code true} if the enchantment can be applied, or {@code false} otherwise | ||
| */ | ||
| @Contract(pure = true) | ||
| default boolean canEnchant(ItemStack stack, Enchantment enchantment) { | ||
| return enchantment.isAcceptableItem(stack); | ||
| } |
There was a problem hiding this comment.
I wonder if a TriState might be better? Either override true, override false, or default to enchantment choice. As of now, it's a complete override if the item implements this interface, hence the default implementation, so calling super will allow the enchantment to decide (Note that the enchantment's decision likely doesn't take the other enchantments on the item into account, as there's the separate Enchantment#canCombine(Enchantment)).
There was a problem hiding this comment.
You could do a tristate. Maybe an enum would be clear? Not sure what best way is to convey meaning of overriding behaviors
There was a problem hiding this comment.
On the other hand, the modder does have the enchantment and can just check the enchantment's canEnchant directly if they want to know what the default was
|
A random thought given the approach chosen here for handling contexts; it does seem fragile, especially for maintenance as there's a point of failure if the context fails to be cleared for a given thread. Wouldn't a better approach to be sending an event with the context that could be in use, given it looks to be intended for mods? There doesn't seem to be any concurrency issues with same thread, as the context looks to be created and destroyed within the same method, tho the potential for error is there if it gets spread around multiple threads tho. |
...ary/item/enchantment/src/main/java/org/quiltmc/qsl/enchantment/api/QuiltEnchantableItem.java
Show resolved
Hide resolved
I like events! They're in the planned todo above but I just hadn't gotten to them. But yeah, I wish there were a safer, more robust way to gather the context. |
library/item/enchantment/src/main/java/org/quiltmc/qsl/enchantment/api/EnchantmentEvents.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Glitch <glitch.g3431@gmail.com>
...src/main/java/org/quiltmc/qsl/enchantment/api/context/PlayerUsingBlockEnchantingContext.java
Show resolved
Hide resolved
|
Any reason this hasn't been merged yet? |
|
It's still a draft PR, plus it doesn't target the current development version for QSL. |
This is a reboot of #20. Below are quoted points by @Vaerian when constructing the original PR.
Currently, I have implemented a common enchanting context, that is then expanded upon by more specific scopes. It currently requires 3 different contexts (base, entity, and player+block) to reach all 6 locations.
Progress
Enchantment#isAcceptableItemandQuiltEnchantment#isAcceptableContext)Enchantment compatibility takes into account the enchantment level (potentially the entire enchantment context)(superceded by event callbacks which can do just as much)