Carpet

Carpet

2M Downloads

[Internals Suggestion] Using MixinExtras

altrisi opened this issue ยท 0 comments

commented

MixinExtras is an "addon" to Mixin that provides two new features: ModifyExpressionValue and WrapWithCondition.

The reason I believe it'd be a good idea to use them is because unlike using Redirect, those can be chained with the ones from other mods in a compatible way, allowing multiple mods to add to a condition. The author claims they are even compatible with existing redirects from other mods to the same target.

Not only that, they're obviously more expressive in the code given they have the specific purpose, and it's not a general redirect.

So let's get to what they do (probably better explained on their wiki, but here I add examples from the carpet codebase):

ModifyExpressionValue

This basically allows to add to a condition without having to make a Redirect to the complete method call (or to an expression). So for example, all those redirect mixins in the creative no clip mixin (https://github.com/gnembon/fabric-carpet/blob/af368a159d2c3c287acbd22d9a9c0f0094519c56/src/main/java/carpet/mixins/PlayerEntity_creativeNoClipMixin.java), they'd be ModifyExpressionValue that would take the original result of the expression, and Carpet could then add an original || (CarpetSettings.creativeNoClip && playerEntity.isCreative() && playerEntity.getAbilities().flying). While still being compatible with any mod redirecting the same expression.

Something like this (not tested directly):

@Redirect(method = "tick", at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/player/PlayerEntity;isSpectator()Z")
)
private boolean canClipTroughWorld(PlayerEntity playerEntity)
{
return playerEntity.isSpectator() || (CarpetSettings.creativeNoClip && playerEntity.isCreative() && playerEntity.getAbilities().flying);
}

to

@ModifyExpressionValue(
    method = "tick", 
    at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/PlayerEntity;isSpectator()Z")
)
private boolean canClipTroughWorld(boolean original)
{
    return original || (CarpetSettings.creativeNoClip && playerEntity.isCreative() && playerEntity.getAbilities().flying);
}

This is done a lot in the carpet codebase.

WrapWithCondition

This one wouldn't be used as much, but it's still nice that it makes those compatible with other mods doing similar things.

As the name indicates, this allows to wrap a method call (or field write) inside an if condition. The easiest example in the carpet codebase I could find is from the cleanLogs rule. Instead of completely redirecting the method call (and therefore causing incompatibilities with other mods trying the same, as it already happened in #788), the check could be changed from something like this:

@Redirect(method = "get",
at = @At(value = "INVOKE", target = "Lorg/apache/logging/log4j/Logger;warn(Ljava/lang/String;Ljava/lang/Object;)V")
)
private void skipLogs(Logger logger, String message, Object p0)
{
if (!CarpetSettings.cleanLogs) logger.warn(message, p0);
}

to

@WrapWithCondition(
    method = "get", 
    at = @At(value = "INVOKE", target = "Lorg/apache/logging/log4j/Logger;warn(Ljava/lang/String;Ljava/lang/Object;)V")
)
private boolean skipLogs(Logger logger, String message, Object p0) {
    return !CarpetSettings.cleanLogs;
}

Other notes

Another nice thing about it is that it's not a mod or something that depends on the game/is installed separately, but a completely stand-alone java library, that just depends on Mixin and ASM. That'd allow Carpet to just shadow it the 7-8 classes into the jar and use it without fear of one day failing with a newer mc version or unwillingly changing other behaviour.

Not only that, the author claims that even if some other mod already uses a normal Redirect on an expression, those injectors should still be compatible with them (or fail as usual if the other redirector changed too much of the expression, but it doesn't get worse at any point).