[1.10] CTM not working
Tiavor opened this issue ยท 14 comments
Using Chisel - MC1.9.4-0.0.6.53 for MC1.10.2
Optifine 1.12.2_HD_U_D1
Texturepack; Conquest for 1.9 (essentially has only connected textures)
Connected textures aren't working when Chisel-mod is active
Those scrubs should be using our CTM system /joke
Interesting. @tterrag1098?
Have the same issue that vanilla textures aren't CTM anymore like the glass block etc. Chisel blocks are fine.
Basically Chisel is ASM patching (aaargh!) Block.getExtendedState() to return a wrapper ChiselExtendedState, which is not a subclass of BlockStateBase. This is done for all blocks, not only for Chisel blocks (why?).
CTM needs the block state to be an instance of BlockStateBase in order to work properly.
ASM patching is only bad when we do it, I suppose?
Why does it specifically need that impl of IBlockState? Ours was carefully made to be interchangeable. If you just call getClean() it should even return the underlying state. There's no reason it should not work with our version.
ASM patching is generally problematic as it can generate hard to find bugs. Even simple access transformers can wreak havoc (nice example for a 3-way mod dependancy).
The BlockStateBase adds a cache for some values that CTM needs in order to have acceptable performance.
Adding the wrapper only for Chisel blocks could fix this, as the Chisel blocks are not relevant to the OptiFine CTM.
BlockStateBase has no caches that I can see. Tell me what exactly you need to exist, and I can make sure our state object behaves the same. Though as I said, calling getClean() should get you what you want. I did not add this on a whim, it exists to allow resource packs to add chisel rendering to any block. Maybe instead of accusing me of doing something wrong, we could work together to solve the issue?
The caches in BlockStateBase are added by OptiFine. Adding the caches in ChiselExtendedState is not going to help as there is no common class (interface) to access them, so I will have to use reflection which is too slow.
Calling getClean() is not simple as I do not have access to the IExtendedState class (OptiFine also has to work without Forge) and additionally the Forge ExtendedStateImplementation.getClean() is too slow as it uses a HashMap.
There is also the problem that some blocks may have both OptiFine and Chisel CTM configurations and using getClean() may cause OptiFine CTM to override Chisel.
For me the best solution would be the ChiselExtendedState to be added only to blocks which have a Chisel CTM configuration. This will solve several problems at once:
- OptiFine CTM will start working for the non-chisel blocks
- OptiFine CTM will be automatically disabled for blocks with Chisel CTM
- other mods may use custom block state implementations without conflicting with Chisel
Adding a wrapper to all block state implementations is quite invasive, it would be better if it is limited to the ones that need it.
The caches in BlockStateBase are added by OptiFine.
So essentially this boils down to "your hack breaks my hack." You didn't mention you had any patches in the first place.
Calling getClean() is not simple as I do not have access to the IExtendedState class
If you do not have access to IExtendedState how are you calling getExtendedState in the first place?
For me the best solution would be the ChiselExtendedState to be added only to blocks which have a Chisel CTM configuration.
I don't think you read my last comment.
OptiFine changes (overwrites) many base classes, this is how it works.
The changed classes are made to be Forge compatible and the Forge methods are called by reflection where needed.
Is it possible for Chisel to add the wrapper only for blocks which currently have CTM configuration?
That would add a lot of overhead, getExtendedState is a hotspot and adding any extra logic would be bad.
My question is, if you don't need the extended state, why are you bothering with it at all? Just use the normal state for CTM.
Forge is calling Block.getExtendedState() and passing the state to BlockModelRenderer.renderModel() where CTM have to work with it.
@sp614x Could we solve this issue by simply extending BlockStateBase? Because that can easily be done.