Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve mixin quality for Status Effect API #383

Open
wants to merge 3 commits into
base: 1.21
Choose a base branch
from
Open

Improve mixin quality for Status Effect API #383

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cb3206
Improve mixin quality for Status Effect API
OroArmor Aug 23, 2024
1edef9f
oops
OroArmor Aug 23, 2024
cdb9328
Update library/entity/status_effect/src/main/java/org/quiltmc/qsl/ent…
OroArmor Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/entity/status_effect/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ qslModule {
moduleDependencies {
core {
api("qsl_base")
testmodOnly("resource_loader")
}
}
injectedInterface("net/minecraft/class_1309") {
Expand Down
3 changes: 2 additions & 1 deletion .../main/java/org/quiltmc/qsl/entity/effect/api/QuiltLivingEntityStatusEffectExtensions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import net.minecraft.entity.LivingEntity;
import net.minecraft.entity.effect.StatusEffect;
import net.minecraft.entity.effect.StatusEffectInstance;
import net.minecraft.registry.Holder;

import org.quiltmc.qsl.base.api.util.InjectedInterface;

Expand All @@ -36,7 +37,7 @@ public interface QuiltLivingEntityStatusEffectExtensions {
* @param reason the reason to remove the status effect
* @return {@code true} if the status effect was successfully removed, or {@code false} otherwise.
*/
default boolean removeStatusEffect(@NotNull StatusEffect type, @NotNull StatusEffectRemovalReason reason) {
default boolean removeStatusEffect(@NotNull Holder<StatusEffect> type, @NotNull StatusEffectRemovalReason reason) {
throw new UnsupportedOperationException("No implementation of removeStatusEffect could be found.");
}

Expand Down
2 changes: 1 addition & 1 deletion ...tus_effect/src/main/java/org/quiltmc/qsl/entity/effect/api/StatusEffectRemovalReason.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public class StatusEffectRemovalReason {
public static final StatusEffectRemovalReason GENERIC_ALL = new StatusEffectRemovalReason(QuiltStatusEffectInternals.id("generic.all"));

/**
* Used when an effect is removed via the vanilla {@link LivingEntity#removeStatusEffect(StatusEffect)} method.
* Used when an effect is removed via the vanilla {@link LivingEntity#removeStatusEffect(net.minecraft.registry.Holder) LivingEntity#removeStatusEffect(Holder&lt;StatusEffect&gt;)} method.
*/
public static final StatusEffectRemovalReason GENERIC_ONE = new StatusEffectRemovalReason(QuiltStatusEffectInternals.id("generic.one"));

Expand Down
16 changes: 9 additions & 7 deletions ...y/status_effect/src/main/java/org/quiltmc/qsl/entity/effect/mixin/EffectCommandMixin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@

package org.quiltmc.qsl.entity.effect.mixin;

import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Redirect;

import net.minecraft.entity.LivingEntity;
import net.minecraft.entity.effect.StatusEffect;
Expand All @@ -27,24 +28,25 @@

import org.quiltmc.qsl.entity.effect.api.StatusEffectRemovalReason;

@Mixin(EffectCommand.class)
// See LivingEntityMixin
@Mixin(value = EffectCommand.class, priority = 500)
public abstract class EffectCommandMixin {
@Redirect(
@WrapOperation(
method = "executeClear(Lnet/minecraft/server/command/ServerCommandSource;Ljava/util/Collection;)I",
at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/LivingEntity;clearStatusEffects()Z")
)
private static boolean quilt$addRemovalReason(LivingEntity instance) {
private static boolean quilt$addRemovalReason(LivingEntity instance, Operation<Boolean> original) {
return instance.clearStatusEffects(StatusEffectRemovalReason.COMMAND_ALL) > 0;
}

@Redirect(
@WrapOperation(
method = "executeClear(Lnet/minecraft/server/command/ServerCommandSource;Ljava/util/Collection;Lnet/minecraft/registry/Holder;)I",
at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/LivingEntity;removeStatusEffect(Lnet/minecraft/registry/Holder;)Z"
)
)
private static boolean quilt$addRemovalReason(LivingEntity instance, Holder<StatusEffect> type) {
return instance.removeStatusEffect(type.value(), StatusEffectRemovalReason.COMMAND_ONE);
private static boolean quilt$addRemovalReason(LivingEntity instance, Holder<StatusEffect> type, Operation<Boolean> original) {
return instance.removeStatusEffect(type, StatusEffectRemovalReason.COMMAND_ONE);
}
}
103 changes: 67 additions & 36 deletions ...ty/status_effect/src/main/java/org/quiltmc/qsl/entity/effect/mixin/LivingEntityMixin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,20 @@

package org.quiltmc.qsl.entity.effect.mixin;

import java.util.Collection;
import java.util.Iterator;
import java.util.Map;

import com.google.common.collect.Iterators;
import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
import org.jetbrains.annotations.NotNull;
import org.spongepowered.asm.mixin.Final;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Overwrite;
import org.spongepowered.asm.mixin.Shadow;
import org.spongepowered.asm.mixin.Unique;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Inject;
import org.spongepowered.asm.mixin.injection.Redirect;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;

import net.minecraft.entity.Entity;
Expand All @@ -41,8 +44,11 @@
import org.quiltmc.qsl.entity.effect.api.StatusEffectRemovalReason;
import org.quiltmc.qsl.entity.effect.api.StatusEffectUtils;
import org.quiltmc.qsl.entity.effect.impl.QuiltStatusEffectInternals;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;

@Mixin(LivingEntity.class)
// We want to make sure that our wrap operations are put before other mods, so that we wrap the vanilla call and not a mods call.
OroArmor marked this conversation as resolved.
Show resolved Hide resolved
// This is because we do not call the vanilla method, so any mod adding something will not be called.
@Mixin(value = LivingEntity.class, priority = 500)
Copy link
Member

Choose a reason for hiding this comment

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

it would probably be wise to set the default priority to around this number globally

public abstract class LivingEntityMixin extends Entity implements QuiltLivingEntityStatusEffectExtensions {
@SuppressWarnings("ConstantConditions")
public LivingEntityMixin() {
Expand All @@ -51,7 +57,7 @@ public LivingEntityMixin() {

@Shadow
@Final
private Map<StatusEffect, StatusEffectInstance> activeStatusEffects;
private Map<Holder<StatusEffect>, StatusEffectInstance> activeStatusEffects;

@Shadow
protected abstract void onStatusEffectRemoved(StatusEffectInstance effect);
Expand All @@ -61,7 +67,7 @@ public LivingEntityMixin() {

@SuppressWarnings("ConstantConditions")
@Override
public boolean removeStatusEffect(@NotNull StatusEffect type, @NotNull StatusEffectRemovalReason reason) {
public boolean removeStatusEffect(@NotNull Holder<StatusEffect> type, @NotNull StatusEffectRemovalReason reason) {
var effect = this.activeStatusEffects.get(type);
if (effect == null) {
return false;
Expand Down Expand Up @@ -117,44 +123,79 @@ public void onStatusEffectRemoved(@NotNull StatusEffectInstance effect, @NotNull
StatusEffectEvents.ON_APPLIED.invoker().onApplied((LivingEntity) (Object) this, effect, false);
}

@Redirect(
@WrapOperation(
method = "onStatusEffectRemoved",
at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/effect/StatusEffect;onRemoved(Lnet/minecraft/entity/attribute/AttributeContainer;)V"
)
)
private void quilt$callOnRemovedWithReason(StatusEffect instance, AttributeContainer attributes) {
StatusEffectInstance effect = this.activeStatusEffects.get(instance);
private void quilt$callOnRemovedWithReason(StatusEffect instance, AttributeContainer attributes, Operation<Void> original, StatusEffectInstance effect) {
instance.onRemoved((LivingEntity) (Object) this, attributes, effect, this.quilt$lastRemovalReason);
StatusEffectEvents.ON_REMOVED.invoker().onRemoved((LivingEntity) (Object) this, effect, this.quilt$lastRemovalReason);
}

/**
* @author The Quilt Project
* @reason Adding removal reason
*/
@Overwrite
public boolean removeStatusEffect(Holder<StatusEffect> type) {
return this.removeStatusEffect(type.value(), StatusEffectRemovalReason.GENERIC_ONE);
@Inject(
method = "removeStatusEffect(Lnet/minecraft/registry/Holder;)Z",
at = @At(
value = "HEAD"
),
cancellable = true
)
public void quilt$shouldRemoveEffect(Holder<StatusEffect> effect, CallbackInfoReturnable<Boolean> cir) {
StatusEffectInstance instance = this.activeStatusEffects.get(effect);
if (instance != null) {
if (!StatusEffectUtils.shouldRemove((LivingEntity) (Object) this, instance, StatusEffectRemovalReason.GENERIC_ONE)) {
cir.setReturnValue(false);
}
}
}

@WrapOperation(
method = "removeStatusEffect(Lnet/minecraft/registry/Holder;)Z",
at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/LivingEntity;onStatusEffectRemoved(Lnet/minecraft/entity/effect/StatusEffectInstance;)V"
)
)
public void quilt$addRemoveStatusEffectReason(LivingEntity instance, StatusEffectInstance effect, Operation<Void> original) {
this.quilt$lastRemovalReason = StatusEffectRemovalReason.GENERIC_ONE;
original.call(instance, effect);
this.quilt$lastRemovalReason = QuiltStatusEffectInternals.UNKNOWN_REASON;
}

/**
* @author The Quilt Project
* @reason Adding removal reason
*/
@Overwrite
public boolean clearStatusEffects() {
return this.clearStatusEffects(StatusEffectRemovalReason.GENERIC_ALL) > 0;
@WrapOperation(
method = "clearStatusEffects",
at = @At(
value = "INVOKE",
target = "Ljava/util/Collection;iterator()Ljava/util/Iterator;"
)
)
private Iterator<StatusEffectInstance> quilt$filterStatusEffects(Collection<StatusEffectInstance> instance, Operation<Iterator<StatusEffectInstance>> original) {
return Iterators.filter(original.call(instance), effect -> StatusEffectUtils.shouldRemove(
(LivingEntity) (Object) this, effect, StatusEffectRemovalReason.GENERIC_ALL
));
}

@Redirect(method = "tickStatusEffects", at = @At(
@WrapOperation(method = "tickStatusEffects", at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/LivingEntity;onStatusEffectRemoved(Lnet/minecraft/entity/effect/StatusEffectInstance;)V")
)
private void quilt$removeWithExpiredReason(LivingEntity instance, StatusEffectInstance effect) {
instance.onStatusEffectRemoved(effect, StatusEffectRemovalReason.EXPIRED);
StatusEffectEvents.ON_REMOVED.invoker().onRemoved(instance, effect, StatusEffectRemovalReason.EXPIRED);
private void quilt$removeWithExpiredReason(LivingEntity instance, StatusEffectInstance effect, Operation<Void> original) {
this.quilt$lastRemovalReason = StatusEffectRemovalReason.EXPIRED;
original.call(instance, effect);
}

@WrapOperation(
method = "onStatusEffectUpgraded",
at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/effect/StatusEffect;onRemoved(Lnet/minecraft/entity/attribute/AttributeContainer;)V"
)
)
private void quilt$removeWithUpgradeApplyingReason(StatusEffect instance, AttributeContainer attributes, Operation<Void> original, StatusEffectInstance statusEffectInstance) {
instance.onRemoved((LivingEntity) (Object) this, attributes, statusEffectInstance, StatusEffectRemovalReason.UPGRADE_REAPPLYING);
StatusEffectEvents.ON_REMOVED.invoker().onRemoved((LivingEntity) (Object) this, statusEffectInstance, StatusEffectRemovalReason.UPGRADE_REAPPLYING);
}

@SuppressWarnings("ConstantConditions")
Expand All @@ -169,14 +210,4 @@ public boolean clearStatusEffects() {
private void quilt$callOnAppliedEvent_upgradeReapplying(StatusEffectInstance effect, boolean reapplyEffect, Entity source, CallbackInfo ci) {
StatusEffectEvents.ON_APPLIED.invoker().onApplied((LivingEntity) (Object) this, effect, true);
}

@Redirect(method = "onStatusEffectUpgraded", at = @At(
value = "INVOKE",
target = "Lnet/minecraft/entity/effect/StatusEffect;onRemoved(Lnet/minecraft/entity/attribute/AttributeContainer;)V")
)
private void quilt$removeWithUpgradeApplyingReason(StatusEffect instance, AttributeContainer attributes) {
StatusEffectInstance effect = this.activeStatusEffects.get(instance);
instance.onRemoved((LivingEntity) (Object) this, attributes, effect, StatusEffectRemovalReason.UPGRADE_REAPPLYING);
StatusEffectEvents.ON_REMOVED.invoker().onRemoved((LivingEntity) (Object) this, effect, StatusEffectRemovalReason.UPGRADE_REAPPLYING);
}
}
10 changes: 6 additions & 4 deletions .../status_effect/src/main/java/org/quiltmc/qsl/entity/effect/mixin/MilkBucketItemMixin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,21 @@

package org.quiltmc.qsl.entity.effect.mixin;

import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Redirect;

import net.minecraft.entity.LivingEntity;
import net.minecraft.item.MilkBucketItem;

import org.quiltmc.qsl.entity.effect.api.StatusEffectRemovalReason;

@Mixin(MilkBucketItem.class)
// See LivingEntityMixin
@Mixin(value = MilkBucketItem.class, priority = 500)
public abstract class MilkBucketItemMixin {
@Redirect(method = "finishUsing", at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/LivingEntity;clearStatusEffects()Z"))
private boolean quilt$addRemovalReason(LivingEntity instance) {
@WrapOperation(method = "finishUsing", at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/LivingEntity;clearStatusEffects()Z"))
private boolean quilt$addRemovalReason(LivingEntity instance, Operation<Boolean> original) {
return instance.clearStatusEffects(StatusEffectRemovalReason.DRANK_MILK) > 0;
}
}
Loading