Gegy / colored-lights

A compromise solution to colored lighting in Minecraft by tinting based on area
https://modrinth.com/mod/colored-lights
GNU Lesser General Public License v3.0
57 stars 11 forks source link

conflict with Emotecraft / bendy-lib #13

Open Fourmisain opened 3 years ago

Fourmisain commented 3 years ago

Issue

colored-light's ModelPartMixin conflicts with bendy-lib's IModelPartMixin.

Both are trying to redirect the same ModelPart.renderCuboids() call.

Attempt 1

I initially tried solving this by replacing the colored-lights @Redirect with a @ModifyArgs, but this caused issues which I can only describe as undefined behavior.

This is how it looked like: test Initially, running with a bunch of other mods, args.<Integer>get(2); was reading a client/render/BufferBuilder instead of an int/Integer. Disabling all other mods except Emotecraft changed this to a client/render/SpriteTexturedVertexConsumer. Both these classes are not used in ModelPart or the mixin-methods. Even weirder, enabling some mods again, it suddenly started working.

In short: It seems you cannot work with redirected methods at all.

Attempt 2

My second try was compiling against bendy-lib and adding a mixin for bendy-lib's IModelPartMixin - but this doesn't work since mixin classes cannot be refered to directly.

One Possible Solution

If bendy-lib moves it's IModelPartMixin.redirectRenderCuboids() code into any referable class, then colored-lights can redirect all cuboid.renderCuboid() calls inside that method instead of ModelPart.render().

This is doable by using a IMixinConfigPlugin: If bendy-lib is installed, disable the ModelPartMixin and enable the mixin for bendy-lib.

Gegy commented 3 years ago

Thank you for the detailed report on this conflict! These patches definitely need quite a bit of work- especially to allow compatibility with mods such as Sodium which entirely replaces this logic. It seems the best solution would be to replace the VertexConsumer as is currently done with particles- it’s just mainly a matter of figuring out how to do that well. It’s challenging in the case where light data can be passed after color data, so we have to be able to retroactively update color without hurting performance too much. Not sure what the best way to go about it is yet 😄

KosmX commented 3 years ago

What is, if we try Attempt 1 again, but with setting the mixin priority? I'll check the class after the mixin, if we do modifyArgs

KosmX commented 3 years ago

If I make colored-lights to redirect with mixin priority higher that bendy-lib
@Mixin(value = ModelPart.class, priority = 800) //default priority is 1000

colored-lights mixin will apply first, this should be stable

Here is the decompiled mixin class, where both mixins was loaded

    public void render(MatrixStack matrices, VertexConsumer vertices, int light, int overlay, float red, float green, float blue, float alpha) {
        if (this.visible) {
            if (!this.cuboids.isEmpty() || !this.children.isEmpty()) {
                matrices.push();
                this.rotate(matrices);
                2 var10001 = 2.of(matrices.peek(), vertices, light, overlay, red, green, blue, alpha);
                this.args$zhh000$modArgs(var10001);
                Entry var10003 = var10001.$0();
                VertexConsumer var10004 = var10001.$1();
                int var10005 = var10001.$2();
                int var10006 = var10001.$3();
                float var10007 = var10001.$4();
                float var10008 = var10001.$5();
                float var10009 = var10001.$6();
                float var19 = var10001.$7();
                float var18 = var10009;
                float var17 = var10008;
                float var16 = var10007;
                int var15 = var10006;
                int var14 = var10005;
                VertexConsumer var13 = var10004;
                Entry var12 = var10003;
                this.redirect$zeg000$redirectRenderCuboids(this, var12, var13, var14, var15, var16, var17, var18, var19);
                Iterator var9 = this.children.values().iterator();

                while(var9.hasNext()) {
                    ModelPart modelPart = (ModelPart)var9.next();
                    modelPart.render(matrices, vertices, light, overlay, red, green, blue, alpha);
                }

                matrices.pop();
            }
        }
    }
KosmX commented 3 years ago

PR #14

Fourmisain commented 3 years ago

Ohh, TIL about mixin priority - well, I knew about it, but I never thought about using it 😄

@KosmX May I ask how you got the decompiled mixin class after applying mixins? That seems like highly useful knowledge to me too!

KosmX commented 3 years ago

Mixin export

I've just read the Fabric docs. https://fabricmc.net/wiki/tutorial:mixin_export image

You can edit the run/debug args in IntelliJ. Just add it there
Then you'll find it in the run/mixin.out/class/.../...class file. If you open it with IntelliJ, it will decompile it for you.

Fourmisain commented 3 years ago

TIL to RTFM... goddammit 😅 Thanks!

I just tested the PR and I wasn't able to break it. Should be good as a temporary workaround until Sodium compatibility gets done.