Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some issues with the 1.19 version of CTM #204

Closed
wants to merge 2 commits into from

Conversation

pupnewfster
Copy link
Contributor

Changes:

  • Bumps dev version of forge to latest (43.2.8)
  • Bumps min version of forge to 43.1.24 and ensures that ambient occlusion is not stripped when unbaking and rebaking quads (CTM 1.19.2 makes Mekanism Multiblock blocks render incorrectly #202)
  • Fixed always (except for leaves) including solid as a render type for baked CTM models which was causing the sugar cane CTM test resources to render with black boxes as opposed to respecting cutout
  • Fixed using default render types instead of model based types for rendering the item variants of baked CTM models which was causing translucent based blocks to render incorrectly as can be seen by the pictures of the item variants in CTM 1.19.2 makes Mekanism Multiblock blocks render incorrectly #202
  • Respect model render types for fallback in baked CTM model rather than using the broken canRenderInLayerFallback method

Questions/comments:

  • Given I am having to bump the forge version anyway, should I bump it to 43.1.56 (as it is still before the latest RB) instead so that Quad can make use of the helper QuadBakingVertexConsumer.Buffered class instead of having to manually handle the single element Quad array for baking purposes?
  • In ModelCTM the formatting displays incorrectly for one of the changes as that section of code uses a mix of tabs and spaces so I am not quite sure what formatting it is supposed to have so if you have any input let me know and I can fix it.
  • I am not fully positive about the desired rendering of items for CTM so I am basically having it do what it was which is defaulting to how the base block version would render, but figured I should ask if this should use the new getExtraLayers method directly or not
  • How should I handle the breaking change I did in IModelCTM? In theory I could default the new method to calling canRenderInLayer for each of the render types in BlockRenderLayer, but that would potentially have the same issue for implementers as existed for sugar cane depending on implementation. Though I am guessing it is worthwhile to do this just to avoid the breaking change? Also should I even be passing the block state parameter to it (it is unused)
  • Following the previous comment/question should I deprecate the canRenderInLayer method for removal? As it is no longer really used anywhere and has been superseded by the new getExtraLayers method
  • Should I delete a good portion of CTMPackReloadListener as the layer hacks aren't used, and the only place the layer fallback check is used now is in ModelCTM#canRenderInLayer, which could potentially just turn that method into: Minecraft.getInstance().getModelManager().getBlockModelShaper().getBlockModel(state).getRenderTypes(state, RandomSource.create(42), ModelData.EMPTY).contains(type) as CTM no longer overrides the actual set render types

I am opening this as a draft PR for now to make sure the questions and comments can be addressed before this gets merged.

…luding solid except for leaves and returning improper results for items
layers |= 1 << renderLayer.ordinal();
} else {
layers |= 1 << 7;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This identical code should be extracted to a helper

@@ -42,6 +43,9 @@ protected AbstractCTMBakedModel createModel(@Nullable BlockState state, IModelCT
parent = castParent.getParent(rand);
}

BakedModel finalParent = parent;
//Only use this if state is not null
Lazy<ChunkRenderTypeSet> lazyParentTypes = Lazy.of(() -> finalParent.getRenderTypes(state, rand, data));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a benefit in this being lazily calculated? Is there a situation in which it fails or takes an inordinate amount of time to calculate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should not be any cases where this fails, but depending on how mods implement things in custom models there is a chance that this might be expensive if calling for every quad as depending on the implementation there is a good chance the other mod doesn't cache their returned result and instead calculates it every single time. Now are those models also used in combination with CTM I have no idea but I figured better safe than sorry.



private final Set<RenderType> extraLayers = new HashSet<>();
private final Set<RenderType> extraLayersView = Collections.unmodifiableSet(extraLayers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a purpose to keeping the old byte field? It was an optimization over exactly this sort of data structure, but since canRenderInLayer is no longer used...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because I was unsure about how the implementation to canRenderInLayer would change and figured it might as well stay until that may be removed, but thinking about it I can remove the byte field and change the unused canRenderInLayer to be implemented as such:

if (getExtraLayers(state).contains(layer)) {
    return true;
}
BakedModel bakedModel = Minecraft.getInstance().getModelManager().getBlockModelShaper().getBlockModel(state);
return bakedModel.getRenderTypes(state, RandomSource.create(42), ModelData.EMPTY).contains(layer);

and that way it would at least be returning mostly accurate results (in theory replacing the byte with a boolean so that it can keep track of if there are some layers with no custom render lay would make it even more accurate but probably isn't needed. Would you like me to make the proposed change to the implementation of canRenderInLayer so that I can remove the byte field and also have it so that it returns the proper result for cases it currently does not?

@@ -21,6 +22,8 @@ public interface IModelCTM extends IUnbakedGeometry<IModelCTM> {

boolean canRenderInLayer(BlockState state, RenderType layer);

Set<RenderType> getExtraLayers(BlockState state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should canRenderInLayer be deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, in fact I think it should be deprecated as for removal, but I held off on it as I figured it could be addressed when the question I asked in the PR description about defaulting the new getExtraLayers method and also how you want to handle breaking changes. There is also the fact that it returns a faulty value for purposes of things like sugar cane in your test pack as canRenderInLayer returns true for solid there.

@tterrag1098
Copy link
Member

Not backporting this at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants