search

KnightMiner / Inspirations

Mod adding various smaller features to Minecraft
MIT License
44 stars 18 forks source link

fixed crash on latest recommended Forge version (14.23.5.2768), fixes #89 #90

Closed SanAndreaP closed 5 years ago

SanAndreaP commented 5 years ago

Given that I couldn't get the Access Transformer to transform the class PotionHelper$MixPredicate to be public (it is package-private now), I resorted to using reflection. Shouldn't impact performance, as they only get accessed during load and the field references are cached.

KnightMiner commented 5 years ago

That should not be needed, just add back the AT that last Forge commit removed

SanAndreaP commented 5 years ago

hmmm... I tried that already and it didn't work (after executing gradlew clean -> gradlew cleanCache -> gradlew setupDecompWorkspace) I'll try again and see if I did anything wrong then.

SanAndreaP commented 5 years ago

urgh, I love gradle sometimes... I've deleted the .gradle folder in my user profile folder and now it works. New PR incoming.

SanAndreaP commented 5 years ago

I can't get it to work for some reason with the AT. After re-indexing the project it shows me the same errors again. The only field that gets transformed is the "reagent" field. I've attached an image to show how it works on my end. Maybe you have more luck with that, but I can't make a PR with errors in the project. at_not_working

KnightMiner commented 5 years ago

Hmm, I'll look into it. I wonder if the Forge patch somehow breaks the AT. If I cannot figure it out I'll comment back here and we will see about reflection.

KnightMiner commented 5 years ago

I've looked into this more, and it seems the Forge patches are breaking the access transformer, so reflection is the only solution now. If you want to reopen this, feel free. Though, is there any reason you do not use ReflectionHelper like I did in utils? If there is a good reason, it might make sense to transition my other reflection to this method as well.

SanAndreaP commented 5 years ago

The only reason was I didn't know about it :V

I'll reopen, look into your ReflectionHelper and make appropriate changes to this PR when I get home.

SanAndreaP commented 5 years ago

I've updated this PR now. I still kept the ReflectionUtil, because there are 2 advantages to just using the ReflectionHelper:

  1. fields are cached: better performance instead of always looking up the field when trying to access it.
  2. easier update: all SRG names are in one class, so they can be easily changed once the mappings change.

I also added a fallback if the mod gets used on older Forge versions.

If you want I can look through the code, replace the rest of the ReflectionHelper usage with the Util class and make a separate PR.

KnightMiner commented 5 years ago

Yeah, if you don't mind I'd prefer using the same reflection method everywhere. Either this PR or another. You can probably also remove the ATs, they are only needed if we directly access the fields.

It might be worth me switching to storing delegates rather than potion types as well, but that's an idea for another day.

Anyways, I'll add some more specific comments when I have a better chance to look at this.

SanAndreaP commented 5 years ago

updated PR again, with replacement of the ReflectionHelper occurences to ReflectionUtil

SanAndreaP commented 5 years ago

updated PR

SanAndreaP commented 5 years ago

method names are now in the right order and proper exceptions are now catched.

The silk touch method is now properly looked up via the Block class. I also added a way to lookup private classes and incorporated into the reflection stuff.

KnightMiner commented 5 years ago

Alright, merged. Thanks!