-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
allow different mapping for reflection #3470
Conversation
src/main/java/org/spongepowered/common/util/ReflectionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spongepowered/common/util/ReflectionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spongepowered/common/util/ReflectionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spongepowered/common/util/ReflectionUtil.java
Outdated
Show resolved
Hide resolved
Should be fixed now. |
src/main/java/org/spongepowered/common/util/ReflectionUtil.java
Outdated
Show resolved
Hide resolved
fixed. |
This looks like a fairly straightforward implementation -- but it doesn't capture the situation of using a SpongeForge/SpongeFabric jar in a dev environment with different mappings. Forge has an (iirc) |
I looked at the fabric part. So, to remap a method, it also needs to also pass a descriptor. I also don't want this API to be restricted to method only. It might be necessary to use reflection on class name or field one day. I think it will be a little bit tricky to design an API that could facilitate potential future use & quite clean right now. I think I will need to look at the forge part too to have a better understanding of what will be the best way. |
Hi, @zml2008 , do you think this new approach works better? or is it overkill? |
vanilla/src/launch/java/org/spongepowered/vanilla/launch/mapping/VanillaMappingManager.java
Outdated
Show resolved
Hide resolved
vanilla/src/launch/java/org/spongepowered/vanilla/launch/mapping/VanillaMappingManager.java
Outdated
Show resolved
Hide resolved
vanilla/src/launch/java/org/spongepowered/vanilla/launch/mapping/VanillaMappingManager.java
Outdated
Show resolved
Hide resolved
This is a step in the right direction -- however, taking a look at Fabric's Mapping resolver API, this API would fail when used in development environments, particularly if a user depended on your fabric implementation from a Yarn-based workspace (since Sponge will at most know about official names and intermediary). You'd have a mismatch between the class literals (remapped by TR), and the method names (string constants), unless you were in a prod environment using intermediary class names. |
As you can see in the fabric mapping resolver, there is a
|
Ah I see that, thanks for clairfying -- it's a touch non-traditional, but the class literal-based API is less error prone. I'm fine with that, but would be even better to see a class literal-based API exposed directly upstream in loader. |
styles fixed. |
Since SpongeForge is developed using forge-loom (which also uses tiny-remapper IIRC), FabricMC/tiny-remapper#70 might be a better solution than this PR. |
It's configured using json file.I've included one for sponge vanilla.