Skip to content

Conversation

@Favorlock
Copy link
Contributor

Goal of this PR is to address the issue described in #403.

Signed-off-by: Favorlock <favorlock@gmail.com>
@Favorlock Favorlock added bug Something isn't working incompatibilty This issue is caused by interaction with another mod I'm on it Currently in development for the upcoming version priority: high High priority labels Apr 16, 2025
@Favorlock Favorlock self-assigned this Apr 16, 2025
Signed-off-by: Favorlock <favorlock@gmail.com>
…0.6.4

Signed-off-by: Favorlock <favorlock@gmail.com>
….11+

Signed-off-by: Favorlock <favorlock@gmail.com>
Copy link
Owner

@RubixDev RubixDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not relevant for this PR, but perhaps try to name your branches following the pattern {fix,feature,...}/<ticket-number>-<ticket-title> in the future. So for example fix/403-curios-update for this branch.

And is it just me, or does the mod compile without problems? I only get some curios mixin error at runtime if I try to run with curios. The conditional-mixin usage looks correct to me.

@Favorlock
Copy link
Contributor Author

Not relevant for this PR, but perhaps try to name your branches following the pattern {fix,feature,...}/<ticket-number>-<ticket-title> in the future. So for example fix/403-curios-update for this branch.

And is it just me, or does the mod compile without problems? I only get some curios mixin error at runtime if I try to run with curios. The conditional-mixin usage looks correct to me.

Will do, I can follow that structure for the branch names.

Compilation of the mod works regardless of which version is built against. As far as I can tell everything should function properly, but it only works for whatever version of curios you compiled against even if there was no issue compiling a conditional mixin for a different version. For example:

Inventorio will select the correct mixin class (e.g. V1 for 5.10.x- and V2 for 5.11.x+) however, if we built Inventorio against Curios 5.10.x- it will still throw a runtime error when we use Curios 5.11.x+ and the V2 mixin is injected at runtime. The reverse will happen if we build Inventorio against 5.11.x+. At runtime it would work with the V2 version of the mixin without issue, but if you were to downgrade the runtime version of curios to 5.10.0 we would get that same runtime error regarding a missing signature even though the V1 mixin was compiled with the correct signature.

As such, that's why I'm thinking in order to maintain compatibility we actually have to compile against two versions of curios for each file in order to maintain backwards compatibility with 5.10.x while ensuring the mod works with newer versions as well. But I find it hard to rationalize creating additional gradle projects just for compatibility fixes of a specific mod. And if there is a means to accomplish this without doing so I haven't really come across a solution for it.

Signed-off-by: Favorlock <favorlock@gmail.com>
Signed-off-by: Favorlock <favorlock@gmail.com>
@Favorlock
Copy link
Contributor Author

Resolved the issue by explicitly specifying the full signature of the method for the Inject method value. This prevents the compiler from simply replacing lambda$handle$0 with curios 5.14.x signature.

@Favorlock Favorlock marked this pull request as ready for review April 17, 2025 23:59
@RubixDev
Copy link
Owner

Resolved the issue by explicitly specifying the full signature of the method for the Inject method value. This prevents the compiler from simply replacing lambda$handle$0 with curios 5.14.x signature.

that makes sense actually, good work.

Are you certain that this issue only exists in 1.20.1 forge though and not maybe on 1.20.2 and 1.20.4 neoforge as well with different curios versions? But if that's not the case, I think this is ready to merge

@Favorlock
Copy link
Contributor Author

I'll double check when I get home later and see if any other platforms are affected.

@Favorlock
Copy link
Contributor Author

Doesn't look like we'll need to do anything for Neoforge as the changes to SPacketSyncModifers don't appear to have been made to Curios for Neoforge 1.20.2/3.

@RubixDev RubixDev merged commit fa7f402 into main Apr 19, 2025
4 checks passed
@RubixDev RubixDev deleted the fix-403 branch April 19, 2025 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working I'm on it Currently in development for the upcoming version incompatibilty This issue is caused by interaction with another mod priority: high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants