Skip to content

Comments

Re-implement ActiveVariantBlockBakedModel#1522

Merged
TechLord22 merged 11 commits intoGregTechCEu:masterfrom
Tictim:avbbm
Mar 3, 2023
Merged

Re-implement ActiveVariantBlockBakedModel#1522
TechLord22 merged 11 commits intoGregTechCEu:masterfrom
Tictim:avbbm

Conversation

@Tictim
Copy link
Contributor

@Tictim Tictim commented Feb 21, 2023

What

This PR re-implements ActiveVariantBlockBakedModel to account for various issues present in current implementation.

Implementation Details

Previous iteration of AVBBM used ThreadLocals to cache model particle texture for later use. Since this model is shared between many variations of VariantActiveBlocks, desync between particle texture and actual tile occurred very frequently.

To address this issue, the AVBBM is redesigned from ground to be registered for each variation of VABs.

  • AVBBM holds two model locations, inactive model and active model.
  • Additionally, AVBBM holds reference to VariantActiveBlock#isBloomEnabled method, which can be overridden from block to enable or disable blooms in certain conditions; more on later.
  • In AVBBM#getQuads, unlisted property active is checked to determine whether to render inactive model or active model. This is same as previous iteration.
  • Bloom handling is modified to account for "no-bloom" rendering mode; that is, emissive textures being turned off. If bloom is turned off, the bloom texture is rendered on CUTOUT layer instead.

Because of the rendering change, VariantActiveBlock#canRenderInLayer implementation is changed yet again.

Outcome

In addition to the fix documented above, this PR contains a behavior change on config casingsActiveEmissiveTextures and machinesEmissiveTextures. Previously, all AVBBM bloom effects are handled by the config casingsActiveEmissiveTextures, probably due to the limitations of the approach. This created some inconsistencies on visuals, most notably in active firebox casings where the firebox blocks are affected by casingsActiveEmissiveTextures and hatches with firebox skin were affected by machinesEmissiveTextures. This created an inconsistent bloom effect which rather looked like a visual glitch.

In this PR, casingsActiveEmissiveTextures is modified to only affect bloom effects of coils. Bloom effects of all other casings are now controlled by machinesEmissiveTextures config.

Potential Compatibility Issues

Like in #1521, this PR poses a risk of breaking mod-added blocks.

@ALongStringOfNumbers ALongStringOfNumbers added the type: feature New feature or request label Feb 21, 2023
@ALongStringOfNumbers
Copy link
Contributor

I am noticing an issue that was already present in the mod still being present here. If you can try and fix it with this refactor, that would be nice, otherwise it should not block this refactor.

The issue is that particles for the different Variant Active blocks do not update for which block they are performed on, and stick with the particles that were displayed for the first time in that world.

For example, Using Tritanium coils, Fusion Glass, and Assembly Line casing. If I cause the particles that are present by falling onto the block from a distance, using the Tritanium coils, the other two blocks will also have the Tritanium coil particles when you fall onto them from a distance.

Or the particles for breaking a block, the particles from the first block you break will also be applied to the other blocks.

This seems to be somewhat chunk related, as when I crossed chunk boundaries and repeated things, the blocks got new particles, but only near the chunk boundaries.

Like I said, this is a minor thing and so should not block this PR, but it would be nice if it could be fixed in this refactor as well.

@Tictim
Copy link
Contributor Author

Tictim commented Feb 23, 2023

The particle issue comes from ThreadLocals being used to cache the particle texture. Patching the issue would probably need a bigger scale of refactor.

@Tictim
Copy link
Contributor Author

Tictim commented Feb 24, 2023

With the last change, AVBBM was re-implemented to address number of shortcomings of previous approach. As a result the preface is currently obsolete and no longer represents this PR's implementation details.

A new behavior change is made in the bloom config. Previously, all VABs are afrected by casingsActiveEmissiveTextures. The new implementation makes machinesEmissiveTextures control bloom effects of all VAB instances, except coils which still uses casingsActiveEmissiveTextures as toggle config. This change was made to match the original intention of the config, as well as to solve a minor inconsistency of casing blooms only appearing on hatches when the bloom is toggled off. I think the config name casingsActiveEmissiveTextures should be changed as well to better represent the new function, but I'll leave it as-is for now.

The code has been tested with/without CTM, but compatibility with Optifine shaders should be tested as well. (I don't mean blooms should work with Optifine shaders; we need to see whether the block models correctly render "no bloom" fallback state.)

ThreadLocal particle cache is deployed in other places as well, so all of them could benefit from reimplementation such as this. Perhaps a generified solution could be devised; but this PR should be sufficient for now.

@Tictim
Copy link
Contributor Author

Tictim commented Feb 24, 2023

Hey uh how do I request review on mobile web???

@TechLord22 TechLord22 added this to the 2.5.3 milestone Feb 27, 2023
@Tictim
Copy link
Contributor Author

Tictim commented Feb 27, 2023

I've updated the description to reflect latest changes.

@Tictim Tictim changed the title ActiveVariantBlockBakedModel refactor & cleanup Re-implement ActiveVariantBlockBakedModel Feb 27, 2023
@TechLord22 TechLord22 added type: refactor Suggestion to refactor a section of code and removed type: feature New feature or request labels Feb 27, 2023
@ALongStringOfNumbers
Copy link
Contributor

Please update the config descriptions for the multiblock and the machine config options. The multiblock should just mention the EBF coils now, and the machines should mention multiblock parts

@TechLord22 TechLord22 merged commit eed7cd2 into GregTechCEu:master Mar 3, 2023
@Tictim Tictim deleted the avbbm branch March 3, 2023 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: refactor Suggestion to refactor a section of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants