Carpet TIS Addition

Carpet TIS Addition

461k Downloads

Improve compatibility for `carpettisaddition.mixins.rule.chunkTickSpeed.ServerWorldMixin `

zly2006 opened this issue · 6 comments

commented

Bug description

Game crashed.

This issue:

zly2006/reden-is-what-we-made/issues/39

Code Digging:

https://github.com/TISUnion/Carpet-TIS-Addition/blob/master/src/main/java/carpettisaddition/mixins/rule/chunkTickSpeed/ServerWorldMixin.java

// carpettisaddition.mixins.rule.chunkTickSpeed.ServerWorldMixin
    @Inject(
        method = {"tick"},
        at = {@At(
    value = "INVOKE",
    target = "Lnet/minecraft/server/world/ServerWorld;getChunkManager()Lnet/minecraft/server/world/ServerChunkManager;"
)}
    )
    void resetTickChunkDepth(BooleanSupplier shouldKeepTicking, CallbackInfo ci) {
        this.depth = 0;
    }

Suggested:Lnet/minecraft/world/chunk/ChunkManager;tick(Ljava/util/function/BooleanSupplier;Z)V

In the previous source code, the injection point is getChunkManager. Turning it into tick could make it more reasonable and reduce the chance of incompatibility.

I can make a PR for it if you agree to modify your source code.

Steps to reproduce

Launch the game with zly2006/reden-is-what-we-made

Expected behavior

No response

Actual behavior

No response

Relevant logs

No response

Minecraft version

1.20.1

Carpet TIS Addition version

mc1.20.1-v1.51.0

Fabric Carpet version

n/a

Other information

No response

Check list

  • I have verified that the issue persists in the latest version of the mod.
  • I have searched the existing issues and confirmed that this is not a duplicate.
commented

Well I can fix this myself, but I still suggest you change the injection point because using tick is more reasonable.

commented

Turning it into tick could make it more reasonable and reduce the chance of incompatibility

  1. Please define "more reasonable"
  2. @Inject is one of the most harmless mixin operation, it only conflicts with @Overwrite which rewrites the entire method and destroys everything. If this could lead to a crash, I'll suggest to look into reden, or another mod might triggers the crash in the same postion next time

https://github.com/zly2006/reden-is-what-we-made/blob/72fdbc873ff2015318bf121d229ff4bb1fa1d3b2/src/main/java/com/github/zly2006/reden/mixin/debugger/MixinServerWorld.java#L92-L97

Tweak the mixin inject position from getChunkManager() to getChunkManager#tick is doable, but I won't do it without any proper reason, or just because other mod @Overwrites everything

commented

so what if another mod, with a lower mixim priority, called the getChunkManager? your depth will be set to zero and that is a side effect. I don't think a getter should have unnecessary side effects and that may be problematic.

commented

so what if another mod, with a lower mixim priority, called the getChunkManager? your depth will be set to zero and that is a side effect

Nothing wrong will happen. The resetTickChunkDepth method is only for resetting the depth on exception being thrown, e.g. for update suppressors. On normal executtion, it doesn't matter how many times (0, 1, 100...) it will get invoked

commented

If it's possible, don't use @Overwrite, especially on such complicated and commonly used method. It's very desctructive and can break tons of mods

If you have to use @Overwrite, please make sure the vanilla method signatures / orders are kept untouched, or mixin confliction with other mod will happen, sooner or later

commented

ok, it is just a suggestion, and i have found the bug in my project and fixed it. thank you for explaining. i am gonna closing this issue.