[1.12.2] Access Transformer problem

[Cadiboo]

I've got an access transformer that is making a protected method public, but when compiling I get the error:

"/build/tmp/recompileMc/sources/net/minecraft/client/renderer/chunk/CompiledChunk.java:17: error: setLayerUsed(BlockRenderLayer) in <anonymous net.minecraft.client.renderer.chunk.CompiledChunk$1> cannot override setLayerUsed(BlockRenderLayer) in CompiledChunk"

 

Here is the code

@SideOnly(Side.CLIENT)
public class CompiledChunk
{
    public static final CompiledChunk DUMMY = new CompiledChunk()
    {
        	//LINE 17 - THIS DOESNT APPEAR TO BE BEING MADE PUBLIC
        protected void setLayerUsed(BlockRenderLayer layer)
        {
            throw new UnsupportedOperationException();
        }
        public void setLayerStarted(BlockRenderLayer layer)
        {
            throw new UnsupportedOperationException();
        }
        public boolean isVisible(EnumFacing facing, EnumFacing facing2)
        {
            return false;
        }
    };
    private final boolean[] layersUsed = new boolean[BlockRenderLayer.values().length];
    private final boolean[] layersStarted = new boolean[BlockRenderLayer.values().length];
    private boolean empty = true;
    private final List<TileEntity> tileEntities = Lists.<TileEntity>newArrayList();
    private SetVisibility setVisibility = new SetVisibility();
    private BufferBuilder.State state;

    public boolean isEmpty()
    {
        return this.empty;
    }

  	//AT THIS TO PUBLIC
    protected void setLayerUsed(BlockRenderLayer layer)
    {
        this.empty = false;
        this.layersUsed[layer.ordinal()] = true;
    }

    public boolean isLayerEmpty(BlockRenderLayer layer)
    {
        return !this.layersUsed[layer.ordinal()];
    }

 

Is there a solution to this?

[Cadiboo]

My AT is public net.minecraft.client.renderer.chunk.CompiledChunk func_178486_a(Lnet/minecraft/util/BlockRenderLayer;)V # setLayerUsed

[Cadiboo]

I'm thinking that if there isn't a good solution I can AT the empty and layersUsed fields to public and replicate the functionality.

[Cadiboo]

It seems to me as though ATs don’t replace overriden methods? If this is right it makes sense, but also renders them slightly useless as they can’t target anonymous classes which Minecraft uses a lot. Unless there’s a way to target anonymous classes??

[V0idWa1k3r]

This is happening because java doesn't allow overrides to lower the access level of the overriden method. If the method in class A is public, then class B that extends A and overrides the method must also have that method as public, not protected or private. In your case you are making

3 hours ago, Cadiboo said:

protected void setLayerUsed(BlockRenderLayer layer)

This as public but look at

public static final CompiledChunk DUMMY = new CompiledChunk()
    {
        	//LINE 17 - THIS DOESNT APPEAR TO BE BEING MADE PUBLIC
        protected void setLayerUsed(BlockRenderLayer layer)
        {
            throw new UnsupportedOperationException();
        }
        public void setLayerStarted(BlockRenderLayer layer)
        {
            throw new UnsupportedOperationException();
        }
        public boolean isVisible(EnumFacing facing, EnumFacing facing2)
        {
            return false;
        }
    };

In this class. It overrides the method and has it as protected which isn't allowed in java. You need to change this method as well. Try to also add this inner class to the AT as  net.minecraft.client.renderer.chunk.CompiledChunk$1. This may or may not work. If it doesn't revert to your field tactic. Or use reflection. It is not that bad. Method lookups and changing the access levels are expensive with reflection. Simply invoking a cached method that already had it's access level set to accessible isn't.

Edited October 23, 2018 by V0idWa1k3r

[diesieben07]

Don't change the access level of methods that could be overridden by mods. You have discovered the reason why. Even if you make all methods that override it in Minecraft public, you will still break every mod that overrides this method.

Use reflection instead.

[Cadiboo]

This method is going to be invoked for every block render layer for every non-air block in every chunk (16x16x16), will the overhead created by reflection be noticable?

[V0idWa1k3r]

I wasn't really able to get a consistent result, but here are the averages of 10 runs of code that does 10000000 operations with direct access to a method VS a reflection invocation of a method:

 

Direct: Took 15ms, avg 1.5E-5ms per operation.
Reflection: Took 25ms, avg 2.5E-5ms per operation.

 

I don't think the JIT optimizations were a thing either in a debug environment.

Here is the benchmark code:

private static Method foo;

    public static void main(String[] args) throws NoSuchMethodException, InvocationTargetException, IllegalAccessException
    {
        foo = ReflectionTest.class.getDeclaredMethod("foo");
        foo.setAccessible(true);
        long currentTimeMillis = System.currentTimeMillis();
        for (int i = 0; i < 10000000; ++i)
        {
            ReflectionTest.foo();
        }

        long diff = System.currentTimeMillis() - currentTimeMillis;
        System.out.println("Direct: Took " + diff + "ms, avg " + (float)diff / 1000000F + "ms per operation.");
        currentTimeMillis = System.currentTimeMillis();
        for (int i = 0; i < 10000000; ++i)
        {
            foo.invoke(null, null);
        }

        diff = System.currentTimeMillis() - currentTimeMillis;
        System.out.println("Reflection: Took " + diff + "ms, avg " + (float)diff / 1000000F + "ms per operation.");
    }

 

[diesieben07]

11 minutes ago, Cadiboo said:

This method is going to be invoked for every block render layer for every non-air block in every chunk (16x16x16), will the overhead created by reflection be noticable?

You'll have to measure. If you notice a performance problem, you can always switch to method handles:

1. Obtain Method instance as normal and call setAccessible.
2. Call MethodHandles.publicLookup().unreflect(method) to obtain a MethodHandle and store it in a static final field (final is important!).
3. To call the method, use MethodHandle#invokeExact on your static final MethodHandle.

The JVM will (usually) optimize this pattern into the same native code that you would get if you were to just call the method directly, there should be no measurable overhead.

[diesieben07]

4 minutes ago, V0idWa1k3r said:

I wasn't really able to get a consistent result, but here are the averages of 10 runs of code that does 10000000 operations with direct access to a method VS a reflection invocation of a method:

 

Direct: Took 15ms, avg 1.5E-5ms per operation.
Reflection: Took 25ms, avg 2.5E-5ms per operation.

 

I don't think the JIT optimizations were a thing either in a debug environment.

Your benchmark is pretty much flawed, sorry to say.

I don't know what the foo method does, but chances are that it does nothing, so the JVM will inline it and figure out that your loop does nothing and then just get rid of the whole loop.

[V0idWa1k3r]

8 minutes ago, diesieben07 said:

chances are that it does nothing

public class ReflectionTest
{
    static int ctr = 0;
    static int op = 0; 

    public static void foo()
    {
        ++ctr;
        int a = (int) Math.max(1, Math.sqrt(ctr));
        op = (int) Math.pow(a, 3);
    }
}

 

[Cadiboo]

Thanks for the help! The $1 from @V0idWa1k3r is what I’m using (I believe that subclasses should make the method public any way, I don’t see why it’s protected in the first place). For some reason I’m not able to get the new code into my dev workspace though (I’m just running setupDecompWorkspace, should I be doing something different?).

 

1 hour ago, diesieben07 said:

You'll have to measure. If you notice a performance problem, you can always switch to method handles:

1. Obtain Method instance as normal and call setAccessible.
2. Call MethodHandles.publicLookup().unreflect(method) to obtain a MethodHandle and store it in a static final field (final is important!).
3. To call the method, use MethodHandle#invokeExact on your static final MethodHandle.

The JVM will (usually) optimize this pattern into the same native code that you would get if you were to just call the method directly, there should be no measurable overhead.

Thanks so much! I’ll be using this in my other mods.

[V0idWa1k3r]

14 minutes ago, Cadiboo said:

I believe that subclasses should make the method public any way, I don’t see why it’s protected in the first place

When you override the method the access level stays the same as it was on the original method by default(with automatic overriding). There is usually no reason for subclasses to expose the method so it stays with the original access level. Especially if the method is considered not public API/internal. And since minecraft doesn't really consider mods there is no need to expose anything beyound the places where the base game needs it. Same reasoning goes for other mods - they won't expect their subclasses to be referenced by anyone else either, especially if they are not a part of the API of the mod. So as D7 said you will likely be breaking any mod that subclasses CompiledChunk. You might want to consider a PR instead. Or just use reflection.

[Cadiboo]

My point was in the actual code, every other method in the class is public

[Cadiboo]

13 hours ago, Cadiboo said:

Thanks for the help! The $1 from @V0idWa1k3r is what I’m using (I believe that subclasses should make the method public any way, I don’t see why it’s protected in the first place). For some reason I’m not able to get the new code into my dev workspace though (I’m just running setupDecompWorkspace, should I be doing something different?).

 

Thanks so much! I’ll be using this in my other mods.

The $1 actually didn't work, I'm using d7's method