search

Fallen-Breath / conditional-mixin

Use annotation to conditionally apply your mixins
GNU Lesser General Public License v3.0
25 stars 5 forks source link

(Neo)Forge support #5

Closed RubixDev closed 3 months ago

RubixDev commented 4 months ago

This is what I've got so far for adding cross-platform support. See #4.

As you noted in that issue:

Another problem is about the versionPredicate syntax design. Possible workarounds:

  1. Keep using the current fabric syntax and perform translation to the target platform. Compatibly with the current design, but might be strange for forge users
  2. Simply use the syntax that the platform provides. Disadvantage: in multi-platform projects, an annotation instance with version predicate cannot be directly reused in different platforms
  3. Design a new universal syntax, and translate to the target platform. tho I'll prefer 1. than this
  4. Shadows let's say fabric's version checking codes into the mod, and use it for all platforms. A bit too tricky for me

I have had similar thoughts. Currently number 2 is implemented, but only because it's the easiest. I don't think this should be the final solution as it will be very annoying for multi-platform users. I think number 3 would be the cleanest, but also the most effort. Number 1 would be nice as it preserves the current syntax, and I don't think it'd be a huge deal for Forge users to have to use that syntax as it's pretty intuitive. As for number 4, that sounds like an option but I'm afraid I have too little experience with Gradle stuff to get that working.

A few other notable things about the current changes:

You are welcome to make changes directly to this PR and maybe implement the version translation as I currently don't have a clear vision of how I would do that.

Fallen-Breath commented 4 months ago

Wow that's a huge amount of changes, thanks in advanced

Some questions:

  1. Any instructions on how you set up this project, like basing on an example mod? Providing the source might help for better maintainability on the setup
  2. I see gradle stuffs are totally rewritten. It's fine for the maven publish thing, I can fix it latter. Have you test if jitpack works
  3. By using jitpack, you can test if your PR works as expected in both platforms, and wide enough mc version range (e.g. 1.14.4 fabric with old loader and java8)

For the predicate syntax thing, idk if it's possible to make an automatic translation, to detect (should be detectable right) translate any of the fabric / forge dialect into the current runtime dialect. Don't need to cover 100% cases should be fine (for those special cases, just please use the naive dialect). This can be done at the future I think

RubixDev commented 3 months ago

Some questions:

  1. Any instructions on how you set up this project, like basing on an example mod? Providing the source might help for better maintainability on the setup
  2. I see gradle stuffs are totally rewritten. It's fine for the maven publish thing, I can fix it latter. Have you test if jitpack works
  3. By using jitpack, you can test if your PR works as expected in both platforms, and wide enough mc version range (e.g. 1.14.4 fabric with old loader and java8)
  1. The setup is based on the 1.20.1-forge-fabric.zip template from https://github.com/architectury/architectury-templates/releases/tag/release_build_1698078656340
  2. jitpack seems to have had some issues recently, but after increasing the available memory for Gradle the latest build worked fine: https://jitpack.io/com/github/RubixDev/conditional-mixin/5e00270aea/build.log
  3. so far I've only tested all three platforms on different 1.20 versions, but I'm on it to test older versions that use Java 8

With this setup, the publications will have following artifcats:

It would technically also be possible to have one singular artifact which includes all these and decides at runtime which one to use, or with another project setup it would also be possible to manually check for the existence of certain classes and call appropriate code, but I don't think that's worth the hassle. The Architectury project setup should work fine

Fallen-Breath commented 3 months ago

me.fallenbreath:conditional-mixin:<version> - the common module, users can depend on this in platform independent code

Suggestion: use me.fallenbreath:conditional-mixin-common:<version> for this to keep me.fallenbreath:conditional-mixin unused. This is more eplicit, and can prevent existing users from upgrading without noticing the syntax change from conditional-mixin to conditional-mixin-fabric

RubixDev commented 3 months ago

Suggestion: use me.fallenbreath:conditional-mixin-common:<version> for this to keep me.fallenbreath:conditional-mixin unused. This is more eplicit, and can prevent existing users from upgrading without noticing the syntax change from conditional-mixin to conditional-mixin-fabric

Good idea! I changed that.

Alright so I did as much testing as I could. Fabric works perfectly fine on all versions. I tested NeoForge on 1.20.2 and 1.20.4 (the only two supported versions afaik) without any issues. Now when it comes to Forge, I didn't even manage to get a normal Forge mod for 1.15 with mixins working, so I couldn't really try adding conditional-mixin. That being said, in my Inventorio setup using it on Forge 1.20.1 works flawlessly and it should also work for older versions as it is also compiled for Java 8. So I think overall this is ready to be merged.

A few more thinks to note:

Also for my personal use case I currently don't even need the version predicates, so it would be awesome if you could release it as is for now and we add the translation stuff later.

Fallen-Breath commented 3 months ago

Ah the mod id thing is annoying. Maybe at least you can make the fabric version provides conditional_mixin as well, so at least it's viable for those who want to use a single mod id

i.e. in fabric.mod.json, add:

  "provides": [
    "conditional_mixin"
  ],

Also, for this

because the artifact name(s) are no longer the same as the repo name, jitpack users will now have to specify com.github.Fallen-Breath.conditional-mixin:conditional-mixin-: instead of just com.github.Fallen-Breath:conditional-mixin:

I think it's just because the Gradle project now contains multiple subprojects with artifacts

RubixDev commented 3 months ago

Ah the mod id thing is annoying. Maybe at least you can make the fabric version provides conditional_mixin as well, so at least it's viable for those who want to use a single mod id

i.e. in fabric.mod.json, add:

  "provides": [
    "conditional_mixin"
  ],

Could you maybe do that quickly? I don't have access to a computer anymore today.

Fallen-Breath commented 3 months ago

Done the remaining stuffs. Last thing required before merge: testing its functionality in each platform. I don't have good mod development environments for forge / neoforge though

so far I've only tested all three platforms on different 1.20 versions, but I'm on it to test older versions that use Java 8

You can make a list / table of what you have tested when you finishes it

Fallen-Breath commented 3 months ago

Found an unmentioned breaking change: me.fallenbreath.conditionalmixin.api.util.VersionChecker#doesVersionSatisfyPredicate now no longer accepts a version, but accepts a mod id instead. This breaks the method signature and its semantic

Suggestion:

  1. Rename the new "doesVersionSatisfyPredicate" to "doesModVersionSatisfyPredicate"
  2. Reintroduce the old doesVersionSatisfyPredicate, mark it as @Deprecated, and only implements it in the fabric platform. You can use Object as the paramenter type to avoid depending fabric-loader
Fallen-Breath commented 3 months ago

https://github.com/Fallen-Breath/conditional-mixin/pull/5#issuecomment-2016502303 is now resolved. Here's a summary for all changes of me.fallenbreath.conditionalmixin.api.util.VersionChecker in this PR

RubixDev commented 3 months ago

You can make a list / table of what you have tested when you finishes it

I pretty much summarized my testing in https://github.com/Fallen-Breath/conditional-mixin/pull/5#issuecomment-2006570931, but here it is as a table:

Fabric NeoForge Forge
1.14.4 :heavy_check_mark: N/A
1.15.2 :heavy_check_mark: N/A
1.16.5 :heavy_check_mark: N/A
1.17.1 :heavy_check_mark: N/A
1.18.2 :heavy_check_mark: N/A
1.19.4 :heavy_check_mark: N/A
1.20.1 :heavy_check_mark: N/A :heavy_check_mark:
1.20.2 :heavy_check_mark: :heavy_check_mark:
1.20.4 :heavy_check_mark: :heavy_check_mark:

If you want me to, I can do more Forge testing and maybe even some Legacy Fabric tomorrow.

Fallen-Breath commented 3 months ago

Nice. Have you tested this PR with java8 in old mc versions by the way?

RubixDev commented 3 months ago

The Fabric tests before 1.17 were done with Java 8

Fallen-Breath commented 3 months ago

Then the last remaing test is on an old forge version (e.g. mc 1.16), to check if the used forged api still works. After that, it's ready to go

RubixDev commented 3 months ago

I'll do that tomorrow

RubixDev commented 3 months ago

Alright, I've spent too many hours trying to fight various Gradle errors without much luck. Here's the updated table, which I understand is a bit lackluster, but I'm done with Gradle for today :face_exhaling:

Fabric NeoForge Forge
1.14.4 :heavy_check_mark: N/A :question:[^1][^2]
1.15.2 :heavy_check_mark: N/A :question:[^3]
1.16.5 :heavy_check_mark: N/A :heavy_check_mark:
1.17.1 :heavy_check_mark: N/A :heavy_check_mark:
1.18.2 :heavy_check_mark: N/A :question:[^4]
1.19.4 :heavy_check_mark: N/A :question:[^4]
1.20.1 :heavy_check_mark: N/A :heavy_check_mark:
1.20.2 :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
1.20.4 :heavy_check_mark: :heavy_check_mark: :question:[^5]

[^1]: User has to manually depend on Mixin, as it's not bundled in Forge.

[^2]: java.lang.NoSuchFieldException: ucp

I'm having trouble getting Forge to run at all

[^3]: java.lang.NoClassDefFoundError: jdk/nashorn/api/scripting/NashornScriptEngineFactory

I assume this is not related to conditional-mixin, but I just wasn't able to get this running.

[^4]: java.lang.module.ResolutionException: Module commons.logging reads more than one module named cpw.mods.securejarhandler

Again, I don't think this has anything to do with conditional-mixin, but I didn't get past this Gradle error.

[^5]: java.lang.module.FindException: Module org.lwjgl.openal not found, required by org.lwjgl.openal.natives

Another Gradle error...
Fallen-Breath commented 3 months ago

Thanks a lot for your testing! I think these tests are enough as 1.16/1.17 forge arr tested

RubixDev commented 3 months ago

I also just did a quick test on Legacy Fabric for Minecraft 1.7.10 and that also worked.