EnchantmentTarget enum needs a proper fix

[TelepathicGrunt]

As we know, Mojang did a big thonk with EnchantmentTarget by making it be its own thing that never actually calls the enchantment's isAcceptableItem method. Due to that, overriding isAcceptableItem is not enough to make your enchantment be applied to the correct items in an Enchantment Table as the table never calls isAcceptableItem anywhere.

The current solution some people came up with is to set the EnchantmentTarget to BREAKABLE so it passes for all tools and then mixin to the return of EnchantmentHelper.getPossibleEntries and then remove your enchantment from the list if the conditions aren't right. This has its own set of problem of course in theory.

An idea for QSL to do was suggested by EMI here:

There is also a PR to Fabric API that tried to address this issue but that PR has died a while ago sadly. I don't know much about this area so I can't be of much help. But this issue report should help to make sure this doesn't get forgotten down the road. CHASM can help address enum issues iirc but having an API that deals with this specific enchantment issue may be nicer for modder to use instead

[biom4st3r]

I'm personally not a fan of that method even as a theardlocal. My personal solution to this was redirecting the .iterator() call for the ENCHANTMENT registry with a filtered one here . Then just run a slightly modified getPossibleEnchantments at the return here . And I just set all my enchantmentTargets to null(i think I have another mixin that makes that not crash)

[TelepathicGrunt]

@biom4st3r I made this issue report for more of what QSL should do and maybe expose in some way for modders.

For modders right now, I just inject right after the adding to list, check if the enchantment is my own, and then run isAcceptableItem to know if I should remove it from the list. No redirect needed here at mod level to prevent any sort of redirect conflict. Might be something you want to check out for Mo Enchantments just in case someone tries to do your way and ends up conflicting with you. (Just make sure you include books in your isAcceptableItem or change my mixin to do the book check there outside of isAcceptableItem) https://github.com/TelepathicGrunt/Bumblezone-Fabric/blob/latest-released/src/main/java/com/telepathicgrunt/bumblezone/mixin/items/EnchantmentHelperMixin.java

(sorry for double post. I used wrong github account)

[biom4st3r]

@biom4st3r I made this issue report for more of what QSL should do and maybe expose in some way for modders.

For modders right now, I just inject right after the adding to list, check if the enchantment is my own, and then run isAcceptableItem to know if I should remove it from the list. No redirect needed here at mod level to prevent any sort of redirect conflict. Might be something you want to check out for Mo Enchantments just in case someone tries to do your way and ends up conflicting with you. (Just make sure you include books in your isAcceptableItem or change my mixin to do the book check there outside of isAcceptableItem) https://github.com/TelepathicGrunt/Bumblezone-Fabric/blob/latest-released/src/main/java/com/telepathicgrunt/bumblezone/mixin/items/EnchantmentHelperMixin.java

(sorry for double post. I used wrong github account)

Yeah I was proposing an alternative. I have a separate interface ExtendedEnchantment that provides more control over the enchantments.

[ghost]

I wrote the PR for fapi and since not much of the code for enchantments has changed in between versions it's largely all reusable. I wouldn't mind trying to port the PR over to quilt, and sort of heading the effort to make a proper enchantments api since I've familiar with the underlying code.

[ghost]

Hi all, I've been working on getting a much deeper understanding of the situation and reading through the enchantment code, and I've outlined my thoughts in the following gist. There's a lot of information there, but it's nearly comprehensive and outlines how I believe we should proceed. Please read "overview.md" at the very least, and if you're interested you can read through "in-depth.md" as well. After reading please give me your thoughts on how you think we should proceed. Unfortunately, the way this code is written puts it in a difficult position when it comes to expandability, an idea I expand upon more in the document.

https://gist.github.com/Vaerian/971469f7fb3ff602466e8d357f69e5b9#file-overview-md

I'd especially like to get input from @i509VCB considering the nature of my proposal. Also, just to get a temperature check on the content of the gist please thumbs up and thumbs down based on whether you agree with me or not, and/or a comment elaborating.

[i509VCB]

So a few things to note from said large gist:

I've been thinking through ways to make this api a reality and the least obstructive methods of getting there, but consistently I hit the same road block. When the EnchantmentScreenHandler runs EnchantmentHelper.generateEnchantments suddenly we are moved out of the scope of the screen handler and information like the World is no longer accessible.

We could add thread locals to track the enchantment screen handler and world, these would have to be cleared after you exit the generateEnchantments method. For mod developers, we could expose a new api which simply enters the world and screen handler into the thread locals and invokes generateEnchantments. Thread locals here are intentional, since the client also may guess enchantments.

Ideally in the future when ASMR is more well developed and we can add extension methods and add javadoc, we could mark the regular generateEnchantments method as deprecated and recommend use of the new method with world context and provide our alternative directly in EnchantmentHelper.

Sidenote: We should rename it to EnchantmentGodClass lol

[ghost]

On a less important note. The Enchantment constructor requires an Enchantment.Rarity value which is also an enum. This is sort of less important because I doubt most people are wanting to use custom enchantment weights, but while we're making an enchantment api it might be a good idea to try and allow devs to just pass it a float value instead.

[deli73]

yeah this not being implemented yet makes enchantments near impossible to work with in almost any context besides "adding enchants to groups of items that already exist", so we'd very much appreciate a fix!

[deli73]

this mixin seems to work, can't comment on if it's viable for the API

@Mixin(EnchantmentHelper.class)
public class FixEnchantmentHelper {
	@Inject(method = "getPossibleEntries",
			at = @At(value = "INVOKE", target = "Lnet/minecraft/enchantment/Enchantment;isTreasure()Z"),
			locals = LocalCapture.CAPTURE_FAILSOFT, cancellable = false, require = 0
	)
	private static void fixPossibleEntires(int power, ItemStack stack, boolean treasureAllowed, CallbackInfoReturnable<List<EnchantmentLevelEntry>> cir,
										   List<EnchantmentLevelEntry> list, Item item, boolean bl, Iterator<Enchantment> var6, Enchantment enchantment)
	{
		if ((!enchantment.isTreasure() || treasureAllowed) && enchantment.isAvailableForRandomSelection() && (enchantment.isAcceptableItem(stack) || bl)) {
			for(int i = enchantment.getMaxLevel(); i > enchantment.getMinLevel() - 1; --i) {
				if (power >= enchantment.getMinPower(i) && power <= enchantment.getMaxPower(i)) {
					list.add(new EnchantmentLevelEntry(enchantment, i));
					break;
				}
			}
		}
	}
}