sp614x / optifine

1.81k stars 418 forks source link

More issues with BlockModelRenderer#renderModel #1046

Closed Wyn-Price closed 6 years ago

Wyn-Price commented 6 years ago

Original thread: https://github.com/AbrarSyed/SecretRoomsMod-forge/issues/204 There's 2 separate issues here. The first is that the secret glass, which reflect the mirrorState, don't cause blocks under it to change when fancy textures is on. I understand why this is but maybe you could add a method into the block class, which could be found using reflect and stored in a HashMap during model baking. The second issue I don't really understand. The picture is in the original thread. You can look at my code for the one way glass here: https://github.com/AbrarSyed/SecretRoomsMod-forge/blob/master/src/main/java/com/wynprice/secretroomsmod/render/fakemodels/OneWayGlassFakeModel.java With the superclass here: https://github.com/AbrarSyed/SecretRoomsMod-forge/blob/master/src/main/java/com/wynprice/secretroomsmod/base/BaseTextureSwitchFakeModel.java And https://github.com/AbrarSyed/SecretRoomsMod-forge/blob/master/src/main/java/com/wynprice/secretroomsmod/render/fakemodels/BaseTextureFakeModel.java And https://github.com/AbrarSyed/SecretRoomsMod-forge/blob/master/src/main/java/com/wynprice/secretroomsmod/render/fakemodels/FakeBlockModel.java

sp614x commented 6 years ago

Not clear what exactly is the problem. Should the secret glass connect to other glass blocks?

Wyn-Price commented 6 years ago

If you look at the pictures at the bottom of the thread, the issue is that the sandstone beath it isn't changing texture when fancy textures is enabled because the block above the bottom layer of sandstone isn't a sandstone block, it's a secret rooms glass block. What I'm saying is if there could be something to add with Java reflect to allow the secret glass to change the texture of the sandstone below it. The other problem is that when more than one secret glass block is placed next to each other, the texture is wrong. It might be to do with how I'm rendering it so I'll try my best to explain it.

To change the texture of the block, I got the list of baked quads from the IBakedModel of the block I was mirroring, then for each BakedQuad in the original secret glass block list, I changed the vertex data of positions 4, 5, 11, 12, 18 & 19 for the secret glass to the vertex data for the mirroredState, with the same position in the vertex data list. I the created new BakedQuads with the new vertex days and added them to a list. After that was done i returned the list.

sp614x commented 6 years ago

CTM can connect by block or by texture. Sandstone is by default connected by texture: matchTiles=sandstone_normal. So CTM checks if the texture (atlas sprite) from the curent model quad matches the texture (atlas sprite) of the neighbour model quad. Your replacement quads should use the same texture (atlas sprite) as the model that you are mirroring.

Wyn-Price commented 6 years ago

Ok. Ill see if that works now

Wyn-Price commented 6 years ago

Theres some pretty intresting behaviours going on. I dont really know what to make of them. I might just have to ditch trying to make ctm work and go back to my old method.

sp614x commented 6 years ago

Looks like the replacement model is using wrong UV coordinates. It shouldn't jump to totally unrelated textures (clock, stained glass, etc.). Make sure that: A. You are not modifying the original model vertex data B. The chunk loading runs multithreaded, all static variables are shared

Wyn-Price commented 6 years ago

Im using System#arrayCopy to change the models vertex data (here). As for the multithreading, I havnt added anything like that so Im not sure if its on by default or I have to do anything to turn it on

Wyn-Price commented 6 years ago

Am I doing anything wrong or is there anything you could suggest to help me. If you could show me how to get the U and V of the texture, that would be very helpful as at the moment im just directly copying the U and V coordinates, which means the texture will be stretched or squashed. Ive tried messing around with TextureAtlasSprites and Ive found the code bit where its originally called to get the values, but I still cant find a way to get the original width and height from the block, and make a new texture atlas sprite with the right UV coords. I might just have to ditch compatability with CTM because it dosnt seem to be working. If you have any advice that would be very helpful, as rendering is the one thing in minecraft I dont understand (and in java in general), and the method Ive come up with took about a day of just messing around with diffrent values, and rendering is somthing that you know pretty well.

sp614x commented 6 years ago

The UV copy is correct, but it looks like it takes the UV from the wrong sprite.

The fixed UV indexes will not work with shaders, they use extended vertex format (56 instead of 28 bytes). The easiest way to make it universal is:

      int step = vertexData.length / 4;
      // 4 quads 
      for (int i = 0; i < 4; i++)
      {
        int pos = i * step;
        // Get UV
        int posU = pos + 4;
        int posV = pos + 5;
        ...
      }
Wyn-Price commented 6 years ago

Ah ok ill try that. That seems like a better idea

Wyn-Price commented 6 years ago

Ok so ive done more testing and ive run into another issue. For the moment, while i make sure the texture is working, Ive commented out the changing of the blocks structure, and make it directly mirror the blocks, as i plan to do it so I change the values of the blocks points, rather than the texture. The issues im having with the rendering, is that the secretroomsblock block does change to the correct block texture, like glass with removed edges and things like that, however the blocks around it dont react to that block, they treat it as a normal block. Code for it here Heres some examples

2017-12-07_16 18 49 In that image, the center block is a secret rooms block. Its difficult to tell, but that block does change its texture to be the glass with no borders, as there is no border for it. It might be easier to spot here: 2017-12-07_16 24 20, where you can clearly see the top of the middle block has changed its texture to fit its surroundings. Another example would be this: 2017-12-07_16 19 14, where you see the middle block (secret rooms block) has got the correct texture but the block below it hasnt changed its texture.

What im asking is if theres a way to get the vanilla blocks to change their texture. You said before about how So CTM checks if the texture (atlas sprite) from the curent model quad matches the texture (atlas sprite) of the neighbour model quad. Your replacement quads should use the same texture (atlas sprite) as the model that you are mirroring., but im directly taking the Quads from the IBakedModel of the glass block, so the TextureAtlasSprite is the same. To make sure that was the case, I did

System.out.println(mirrorQuad.getSprite());

before I added the quads to the list, and sure enough in the console I got

[16:33:40] [main/INFO] [STDOUT/]: [com.wynprice.secretroomsmod.render.fakemodels.BaseTextureFakeModel:func_188616_a:72]: TextureAtlasSprite{name='minecraft:blocks/glass', frameCount=1, rotated=false, x=160, y=176, height=16, width=16, u0=0.15625976, u1=0.17186524, v0=0.34376952, v1=0.37498048}

The only problem I can think that would occur is that Im using static varibles in the TileEntityRender section to set the world, and the position, meaning for the render to work correctly, it has to be run through the TileEntityRender classes. I think that a problem could be that if your getting the BakedQuads, then those varibles wont be set correctly. So, to test this I replaced System.out.println(mirrorQuad.getSprite()); with new Exception().printStackTrace();, so I could see where the code was being run from and when run, the console only printed

java.lang.Exception
    at com.wynprice.secretroomsmod.render.fakemodels.BaseTextureFakeModel.func_188616_a(BaseTextureFakeModel.java:72)
    at net.minecraft.client.renderer.BlockModelRenderer.func_187498_b(BlockModelRenderer.java:105)
    at net.minecraftforge.client.model.pipeline.ForgeBlockModelRenderer.func_187498_b(ForgeBlockModelRenderer.java:107)
    at net.minecraft.client.renderer.BlockModelRenderer.func_187493_a(BlockModelRenderer.java:76)
    at net.minecraft.client.renderer.BlockModelRenderer.func_178267_a(BlockModelRenderer.java:55)
    at com.wynprice.secretroomsmod.base.BaseTERender.func_192841_a(BaseTERender.java:83)
    at net.minecraft.client.renderer.tileentity.TileEntityRendererDispatcher.func_192854_a(TileEntityRendererDispatcher.java:180)
    at net.minecraft.client.renderer.tileentity.TileEntityRendererDispatcher.func_180546_a(TileEntityRendererDispatcher.java:148)
    at net.minecraft.client.renderer.RenderGlobal.func_180446_a(RenderGlobal.java:1001)
    at net.minecraft.client.renderer.EntityRenderer.func_175068_a(EntityRenderer.java:1781)
    at net.minecraft.client.renderer.EntityRenderer.func_78471_a(EntityRenderer.java:1606)
    at net.minecraft.client.renderer.EntityRenderer.func_181560_a(EntityRenderer.java:1377)
    at net.minecraft.client.Minecraft.func_71411_J(Minecraft.java:1117)
    at net.minecraft.client.Minecraft.func_99999_d(Minecraft.java:397)
    at net.minecraft.client.main.Main.main(SourceFile:123)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:483)
    at net.minecraft.launchwrapper.Launch.launch(Launch.java:135)
    at net.minecraft.launchwrapper.Launch.main(Launch.java:28)

Im not sure what else I can do for this. Hopfully you have a way to make blocks around the secret rooms block to change to the correct texture

sp614x commented 6 years ago

CTM connects to other blocks based on texture or block ID (blockstate). This is the method checking if a neighbour block should be connected: https://pastebin.com/uKVjzv56

To make other blocks connect to the mirror block: A. If connected by block IBlockAccess.getBlockState(neighbourPos) has to return the mirrored blockstate B. If connected by sprite BlockRendererDispatcher().getBlockModelShapes().getModelForState(neighbourState) should return a model which uses the mirrored sprite

The first one can be fixed if you hijack the rendering of all blocks and use a proxy IBlockAccess which replaces the "mirror block" states with the mirrored "block states". If the neighbour block state is correct, then the model should also be OK as it is based on the state.

Wyn-Price commented 6 years ago

How could you hijack the rendering of the blocks ? Also assuming isNeighbour is the method being called in the first place, would somthing like this also work. It would let me do the proxy block access thing but only for the optifine/secretrooms mod blocks. If theres a way I can hijack the render code and replace the IBlockAccess with the one i need then just let me know

Wyn-Price commented 6 years ago

I was thinking replacing Minecraft#world but im not sure when i replace it, and when I would put it back to normal

sp614x commented 6 years ago

Forge defines this in RenderChunk:

    /**
     * Creates a new RegionRenderCache instance.<br>
     * Extending classes can change the behavior of the cache, allowing to visually change
     * blocks (schematics etc).
     *
     * @see RegionRenderCache
     * @param world The world to cache.
     * @param from The starting position of the chunk minus one on each axis.
     * @param to The ending position of the chunk plus one on each axis.
     * @param subtract Padding used internally by the RegionRenderCache constructor to make
     *                 the cache a 20x20x20 cube, for a total of 8000 states in the cache.
     * @return new RegionRenderCache instance
     */
    protected ChunkCache createRegionRenderCache(World world, BlockPos from, BlockPos to, int subtract)
    {
      return new ChunkCache(world, from, to, subtract);
    }

it could be use make a proxy ChunkCache.

sp614x commented 6 years ago

In RenderGlobal there is a field IRenderChunkFactory renderChunkFactory; that could be used to make custom RenderChunk's with custom createRegionRenderCache(). I am not sure if some other mods (or Forge) are already using it.

Wyn-Price commented 6 years ago

So would I have to use reflect to change the IRenderChunkFactory renderChunkFactory value to a custom IRenderChunkFactory ? That would make sense

sp614x commented 6 years ago

It would also be nice to pipeline the ChunkCache creation in case someone has already installed a custom factory. To do it, keep the old factory and when creating a new RenderChunk give it a RenderChunk created by the old factory (old RenderChunk). Then the method createRegionRenderCache() should first call the createRegionRenderCache() on the old RenderChunk() an then make a proxy on top of it. In this way if someone has already replaced the factory with custom one his changes would be preserved.

Wyn-Price commented 6 years ago

Ok theres an issue that ive found with what ive done so far. So ive set it up so the IBlockAccess is replaces with my IBlockAccess and it all works, however theres a problem. Because ive set it so the IBlockAccess gets the block, the TileEntitySpecialRenderer.java isnt being called, as before it searches for that, the code effectively does IBlockAccess#getBlockState#getblock#hasTileEntity . Now the issue thats happening is that the IBlockAccess#getBlockState is returning the mirroredState, so if that block dosnt have a tileentity, the TileEntitySpecialRenderer will never be called. All I need to be able to do, is return the normal secretroomsmod blockstate when RenderChunk#L-160 is called, but because Optifine seemingly changes things around im not sure how I can do this

Wyn-Price commented 6 years ago

I might just have to replace the whole of the RenderChunk class

Wyn-Price commented 6 years ago

Would you be able to help on this. im not sure if theres any way to do it. I did what you said and made the whole new IBlockAccess system. The only problem with it now, is that it gets the mirrored state, and renders that. Ive tried alot of diffrent things, and none of them really work. All I need is to be able to change the blockstate that is taken in RenderChunk#rebuildChunk. In ChunkCacheOF.java, I noticed you had this bit of code

IBlockState iblockstate = this.blockStates[i];

if (iblockstate == null)
{
     iblockstate = this.chunkCache.getBlockState(pos);
     this.blockStates[i] = iblockstate;
}

return iblockstate;

and what I think is happening, is that the mirrored state, and the non-mirrored state are getting mixed up, as my ChunkCache#getBlockState is not being called to get the state for rendering (in RenderChunk#L-160), but is being called by various diffrent reasons, like working out the light. The only way I can think of working around this, is to use reflect to change the list of blockstates you have and removing them when its a SecretRoomsMod block. I dont want to do this as im not sure what is relied on there and I dont want to break any of the code, but it might have to be somthing I need to do if I cant find another way.

sp614x commented 6 years ago

This is getting complicated. I am not sure how it can be fixed without having performance problems.

Wyn-Price commented 6 years ago

Why are you storing the blockstates, and also what is the ArrayCache used for. Ive found a way to hijack the class but both of those things will be gone

Wyn-Price commented 6 years ago

Ok ive managed to do somthing here. From testing it dosnt break the code at all, but I just wanted to make sure that there wasnt anything breaking. FakeArrayCache.java