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

EnchantmentTarget enum needs a proper fix #13

Open
TelepathicGrunt opened this issue Jul 15, 2021 · 9 comments · May be fixed by #291
Open

EnchantmentTarget enum needs a proper fix #13

TelepathicGrunt opened this issue Jul 15, 2021 · 9 comments · May be fixed by #291
Labels
discussion enhancement New feature or request

Comments

@TelepathicGrunt
Copy link
Contributor

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:
image

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
Copy link

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
Copy link
Contributor Author

@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
Copy link

@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
Copy link

ghost commented Jul 15, 2021

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.

@i509VCB i509VCB added discussion enhancement New feature or request labels Jul 17, 2021
@ghost
Copy link

ghost commented Jul 17, 2021

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
Copy link
Contributor

i509VCB commented Jul 17, 2021

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
Copy link

ghost commented Jul 19, 2021

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.

@ghost ghost mentioned this issue Jul 30, 2021
9 tasks
@deli73
Copy link

deli73 commented Aug 19, 2022

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
Copy link

deli73 commented Aug 19, 2022

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;
				}
			}
		}
	}
}

@Platymemo Platymemo linked a pull request Mar 30, 2023 that will close this issue
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
4 participants