fabric
fabric copied to clipboard
FabricMC
fabric-enchanting-api-v1 updated for 1.16.3
Updated the proposed enchantment target api for 1.16.3. Seems like Mojang removed EnchantmentTarget.ALL, so this version uses a null value to indicate that functionality should be handled in another way. In the process I replaced the redirect mixin with a more compatibility forward inject. This should also fix the concerns about thread safety. Closes #203. Closes #202.
One thing I am curious about is how would someone make an enchantment that can, for example, be applied to a certain item only via enchanted book but not via enchanting table? Siphalor in their PR sorta explained this here: https://github.com/FabricMC/fabric/pull/203/files#r491412775
Idk if this is possible with your current api. A potential idea is to override that one method and make it final, and then add two new methods with better names for enchanting vs. applying enchants. Looks very close to good to merge, nice work!
Yeah that sounds like that might be a good direction to go into. I wonder how vanilla does it, might be something to look into. If it does become two methods (one for can enchant in table and one for can enchant with book) I think it would also make sense to then have the can enchant in table method override the Item.isEnchantable method because currently an item that doesn't say that it's enchantable can't be enchanted in an enchantment table no matter what, but if the enchantment is saying it should specifically in a table then maybe there's reason to overwrite that.
Apparently the way vanilla does it, is that some enchantments indicate that they can be added to an item, but those items don't indicate that they're enchantable, so the result is that the only way the enchantment can be added to the item is by using an anvil. I think this means we should just add one "isEnchantable" method that both overwrites the item's indication of enchantability, and states that the enchantment can be applied to the item.
Actually it might be wiser to just let individual mod authors mix into the item they specifically want to target, because in the grand scheme of things it's about the simplest inject mixin possible and so it's really not essential to be in fabric
This would actually probably be a good idea because it would cut out this extra registry iterator I had to make by instead having static compile time knowledge of the items that need to be enchantable or not.
Okay I addressed the functionality Pros mentioned in https://github.com/FabricMC/fabric/pull/1102/commits/6aa093b2255a6fd201329c3bb23f70192fcb58bd. There was some discussion on how to handle this anvil situation. I chose to keep the actual code in the PR but disable the mixin. I also included an explanation at the head of the file that it's disabled because it doesn't fit with vanilla, but included for the sake of illustrating how to fix the problem if so desired.
Other than that this PR should be done I'm going to re-request reviews from the people who submitted feedback and then someone should request a review from muss and player.
Alrighty, this should be done now! I can't conceive of anything else I'd want to add to it. There's now a callback event for applying an enchantment in the anvil, and enchanting in the table. This should hopefully justify the use of redirects in these sticky situations. Additionally, there's a R/W lock on the whole capture local cache kind of thing so that should quell any concerns over thread safety. That being said I am not a multi-threading professional and I've only done it in limited capacity in Rust and never in Java so conceptually I think it works, and it seems like it's functional in game, but I don't know how terse a solution it is.
Real talk I have no clue what's going on with the build test right now, it would be awesome if someone could take a look at that, because locally everything seems to work fine.
You have a checkstyle violation somewhere, would be useful if the gradle log output showed it. There are IDE plugins that will aid with this.
Fixed, could've sworn it was meeting the checkstyle earlier, really thought it was something else causing the error.
Everything has been changed to match the suggestions, the one thing I didn't change was the author tags in the javadocs for the reason I outlined above. I figure it can't hurt, if it's really a problem I'll just make another commit to remove them. This should be good to pull in now at the discretion of the modding gods above.
you need to replace the current registerAll method in EnchantmentEvents with
public static void registerAll(BiFunction<Enchantment, ItemStack, TriState> callback) {
ACCEPT_ENCHANTMENT.register(callback::apply);
ACCEPT_APPLICATION.register(callback::apply);
}
I really need this API, when is it released? :sweat_smile: The requested changes seem simple.
I've told modmuss and player to look into this PR. Hopefully they get to reviewing it today or tomorrow.
Quick poll for those still interested in this PR. Player mentioned the idea of exposing a world, nullable block pos, and a nullable player to each of the enchantment callbacks. While that sounds useful it would require restructuring the mixins in this PR so that the enchantment table related mixins aren't into the EnchantmentHelper class, but instead into the enchantment table block entity itself.
The obvious issue this presents it is means that we would completely bypass the EnchantmentHelper class (which by nature of its implementation works outside of the context of the game state meaning you can query the enchantments for an item without actually having to have any access to a world, block, block position, etc.) and thus mods using the EnchantmentHelper class for logic would have unexpected results with mods that add custom enchantments. Really this comes to how invasive we want to be with this module. I could see it certainly being helpful to expose this additional functionality (and probably useful to many mods) but doing so would be more breaking and invasive than the current implementation. We'd effectively have to create a FabricEnchantmentHelper class and ask that all developers use that instead of the vanilla EnchantmentHelper class (otherwise their mod wouldn't play well with others).
For those still interested in this PR (mainly it seems that would be @sfPlayer1 @modmuss50 @i509VCB @oliviathevampire @Prospector @NeusFear @maityyy (sorry for the pings wasn't really sure how else to make sure this was seen and I'm not totally sure how to proceed)) we're gonna take a little poll so you can let me know what you think sounds best and I'll do that. 👍🏻 for the more invasive version but more versatile PR and 👎🏻 for the current version of the PR without that additional level of exposure.
I think that forcing Fabric mod developers to use an alternative to the EnchantmentHelper class is a very bad idea, if only because the Fabric API cannot be fully installed or used by players and mod devs. Also, it seems to me that this is contrary to the Fabric API concept. Fabric API are handy hooks for working with vanilla code, not independent Minecraft Modding API (like Bukkit API). It can also cause conflicts between mods.
There may be a more appropriate name than FabricEnchantmentHelper?
@maityyy We do provide some extensions of vanilla things like a GameRuleVisitor which includes the double and enum rules. As long as the javadoc refers the mod dev to the vanilla method in the case of a reduced context it seems fine to me to include the extra context method.
There are quite a number of unresolved comments on it if you read the latest comments since the last commit
