SuperMartijn642 / ConnectedGlass

6 stars 9 forks source link

[Feature] Appearance API support #46

Closed XFactHD closed 7 months ago

XFactHD commented 8 months ago

Is your feature request related to a problem? Please describe. At the moment ConnectedGlass takes the block for whom the connection state is being requested and the blocks surrounding it and compares them directly. This leads to issues like this one in FramedBlocks where glass blocks on this kind of "mimic" block don't connect to actual glass blocks and, in the case of my implementation, glass blocks on this kind of mimic block unintentionally connect to mimic blocks without glass on them. The reason for the latter is that I am passing my own blockstate to the source model's getModelData() instead of the source state in order to have better control over the behaviour of the appearance API.

Describe the solution you'd like To fix this, the connection calculation in SideConnections#isSameBlock() needs to invoke IForgeBlock#getAppearance() for both the "local" and the neighboring blocks, check that the results aren't air and then compare them. The reason for requesting the appearance of the local block for each neighbor is that I need to know from which position and, ideally, also with which block a connection is being attempted to be able to properly handle connections to "double blocks" which may have multiple different textures on a single face. The block being passed as the "query block" should be the one before the appearance query in both cases. The reason for checking for air is that I need to be able to say "whatever the querying block is, it cannot connect".

Fabric provides the exact same API via IFabricBlock#getAppearance().

Side note To be able to fully support your connected textures on my blocks, I need to have access to your model's model data and therefore need the ModelProperty of that data. I can either add such an integration myself where I retrieve your ModelProperty, which of course has the downside of reaching for internals, or you can provide the property to me via my add_ct_property IMC method. The latter wouldn't even require any checks for FramedBlocks being loaded, you can just blindly throw the IMC message into the void in InterModEnqueueEvent and if FramedBlocks is not loaded it will just be ignored.

SuperMartijn642 commented 7 months ago

I've updated Fusion to support the Forge/Fabric appearance APIs and I've updated Connected Glass to use Fusion, so I don't have to keep duplicating code between the two.

To fix this, the connection calculation in SideConnections#isSameBlock() needs to invoke IForgeBlock#getAppearance() for both the "local" and the neighboring blocks, check that the results aren't air and then compare them.

I adjusted the connection checks to also rerequest the appearance of the block itself for each neighbor and made it not connect when either the block itself or the neighbor is air.

With that, I now get the following result: image Both full blocks are just the block itself and the half blocks are Framed Panels from FramedBlocks.

It seems for the sides and the back of the panels, it still returns air when requesting the appearance, thus those would not connect. However, even besides that, only the front face of the panel receives the model data. The sides and back always receive empty model data.

or you can provide the property to me via my add_ct_property IMC method.

I added the following line which I believe passes the model data property as intended, but with or without it, it seems to make no difference. (I checked with a breakpoint and the event listener does get called)

FMLJavaModLoadingContext.get().getModEventBus().addListener((Consumer<InterModEnqueueEvent>)event -> InterModComms.sendTo("framedblocks", "add_ct_property", () -> ConnectingBakedModel.SURROUNDING_BLOCK_DATA_MODEL_PROPERTY));

This seems to be as far as I can get as I am not really sure where I am going wrong. The relevant parts should be visible in this commit: https://github.com/SuperMartijn642/Fusion/commit/26bdfe3a9e91cc2acb027c42b60c36574540bd2b Do you know what might be wrong?

XFactHD commented 7 months ago

Had to clone your repo before remembering again what's going on😅: It's not your fault that it doesn't work on the two structures on the left, it's caused by a default in my config. Due to the CT support still being slightly unstable, the conTexMode setting in the FramedBlocks client config defaults to FULL_FACE, which limits it to what you are seeing. If you set that to DETAILED, then it'll work in all test cases. Looking at the commit you linked, the implementation looks good to me :shipit:

SuperMartijn642 commented 7 months ago

If you set that to DETAILED, then it'll work in all test cases.

Great that did the trick, it all connects properly now. Damn that appearance API is really cool used like this 🙂

I released it as an update for Fusion for Forge 1.19.2 - 1.20.2 and Fabric 1.18 - 1.20.2. So both Connected Glass and Rechiseled should now connect properly. Thank you for the excellent explanation and report, and for taking the time to look into it!

XFactHD commented 7 months ago

Awesome, thank you for the quick resolution.