Skip to content

Fix some issues with the 1.19 version of CTM #204

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix some issues with the 1.19 version of CTM #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daaa326
Bump min forge version to 43.1.24 and fix stripping ambient occlusion…
pupnewfster Mar 24, 2023
f02d418
Fix calculating possible render types for baked ctm models always inc…
pupnewfster Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
mod_version=1.1.7
minecraft_version=1.19.2
forge_version=43.1.3
forge_version=43.2.8

curse_type=beta
projectId=267602
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/team/chisel/ctm/api/model/IModelCTM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.Collection;

import java.util.Set;
import javax.annotation.Nullable;

import net.minecraft.client.renderer.RenderType;
Expand All @@ -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.


@Nullable
TextureAtlasSprite getOverrideSprite(int tintIndex);

Expand Down
19 changes: 6 additions & 13 deletions src/main/java/team/chisel/ctm/client/model/AbstractCTMBakedModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,17 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumMap;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import javax.annotation.ParametersAreNonnullByDefault;

import net.minecraftforge.client.RenderTypeHelper;
import org.jetbrains.annotations.NotNull;

import com.google.common.base.Throwables;
Expand All @@ -31,7 +30,6 @@
import lombok.ToString;
import net.minecraft.client.Minecraft;
import net.minecraft.client.multiplayer.ClientLevel;
import net.minecraft.client.renderer.ItemBlockRenderTypes;
import net.minecraft.client.renderer.RenderType;
import net.minecraft.client.renderer.block.model.BakedQuad;
import net.minecraft.client.renderer.block.model.ItemOverrides;
Expand Down Expand Up @@ -229,26 +227,21 @@ public ChunkRenderTypeSet getRenderTypes(@NotNull BlockState state, @NotNull Ran
if (this.layer != null) {
return ChunkRenderTypeSet.union(ChunkRenderTypeSet.of(layer), getParent(rand).getRenderTypes(state, rand, data));
} else {
List<RenderType> ret = new ArrayList<>();
for (RenderType type : ChunkRenderTypeSet.all()) {
if (this.getModel().canRenderInLayer(state, type)) {
ret.add(type);
}
}
return ChunkRenderTypeSet.union(ChunkRenderTypeSet.of(ret), getParent(rand).getRenderTypes(state, rand, data));
ChunkRenderTypeSet extraTypes = ChunkRenderTypeSet.of(this.getModel().getExtraLayers(state));
return ChunkRenderTypeSet.union(extraTypes, getParent(rand).getRenderTypes(state, rand, data));
}
}

@Override
public List<RenderType> getRenderTypes(ItemStack itemStack, boolean fabulous) {
List<RenderType> ret = new ArrayList<>();
ret.addAll(this.getParent().getRenderTypes(itemStack, fabulous));
List<RenderType> ret = new ArrayList<>(this.getParent().getRenderTypes(itemStack, fabulous));
if (this.layer != null) {
if (!ret.contains(layer)) {
ret.add(layer);
}
} else {
var type = ItemBlockRenderTypes.getRenderType(itemStack, false);
//Note: Uses this model as opposed to parent so that any layers added by CTM can be checked as well
var type = RenderTypeHelper.getFallbackItemRenderType(itemStack, this, false);
if (!ret.contains(type)) {
ret.add(type);
}
Expand Down
13 changes: 9 additions & 4 deletions src/main/java/team/chisel/ctm/client/model/ModelBakedCTM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@
import net.minecraft.core.Direction;
import net.minecraft.util.RandomSource;
import net.minecraft.world.level.block.state.BlockState;
import net.minecraftforge.client.ChunkRenderTypeSet;
import net.minecraftforge.client.model.data.ModelData;
import net.minecraftforge.common.util.Lazy;
import team.chisel.ctm.api.model.IModelCTM;
import team.chisel.ctm.api.texture.ICTMTexture;
import team.chisel.ctm.api.texture.ITextureContext;
import team.chisel.ctm.api.util.RenderContextList;
import team.chisel.ctm.client.util.BakedQuadRetextured;
import team.chisel.ctm.client.util.CTMPackReloadListener;

@ParametersAreNonnullByDefault
public class ModelBakedCTM extends AbstractCTMBakedModel {
Expand All @@ -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.

AbstractCTMBakedModel ret = new ModelBakedCTM(model, parent, layer);
for (Direction facing : FACINGS) {
List<BakedQuad> parentQuads = parent.getQuads(state, facing, rand, data, null); // NOTE: We pass null here so that all quads are always returned, layer filtering is done below
Expand Down Expand Up @@ -77,10 +81,11 @@ protected AbstractCTMBakedModel createModel(@Nullable BlockState state, IModelCT
// Explore optimizations to quad goal (detecting overlaps??)
int quadGoal = ctx == null ? 1 : texturemap.values().stream().mapToInt(tex -> tex.getType().getQuadsPerSide()).max().orElse(1);
for (Entry<BakedQuad, ICTMTexture<?>> e : texturemap.entrySet()) {
ICTMTexture<?> texture = e.getValue();
// If the layer is null, this is a wrapped vanilla texture, so passthrough the layer check to the block
if (layer == null || (e.getValue().getLayer() != null && e.getValue().getLayer().getRenderType() == layer) || (e.getValue().getLayer() == null && (state == null || CTMPackReloadListener.canRenderInLayerFallback(state, layer)))) {
ITextureContext tcx = ctx == null ? null : ctx.getRenderContext(e.getValue());
quads.addAll(e.getValue().transformQuad(e.getKey(), tcx, quadGoal));
if (layer == null || (texture.getLayer() != null && texture.getLayer().getRenderType() == layer) || (texture.getLayer() == null && (state == null || lazyParentTypes.get().contains(layer)))) {
ITextureContext tcx = ctx == null ? null : ctx.getRenderContext(texture);
quads.addAll(texture.transformQuad(e.getKey(), tcx, quadGoal));
}
}
}
Expand Down
26 changes: 23 additions & 3 deletions src/main/java/team/chisel/ctm/client/model/ModelCTM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -66,7 +67,9 @@ public class ModelCTM implements IModelCTM {
protected Map<Pair<Integer, ResourceLocation>, ICTMTexture<?>> textureOverrides;

private final Collection<ResourceLocation> textureDependencies;


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?

private transient byte layers;

private Map<ResourceLocation, ICTMTexture<?>> textures = new HashMap<>();
Expand Down Expand Up @@ -162,7 +165,13 @@ public void initializeTextures(ModelBakery bakery, Function<Material, TextureAtl
} else {
tex = meta.get().makeTexture(sprite, spriteGetter);
}
layers |= 1 << (tex.getLayer() == null ? 7 : tex.getLayer().ordinal());
BlockRenderLayer renderLayer = tex.getLayer();
if (renderLayer != null) {
extraLayers.add(renderLayer.getRenderType());
layers |= 1 << renderLayer.ordinal();
} else {
layers |= 1 << 7;
}
return tex;
});
}
Expand Down Expand Up @@ -190,7 +199,13 @@ public void initializeTextures(ModelBakery bakery, Function<Material, TextureAtl
sprite = spriteGetter.apply(new Material(TextureAtlas.LOCATION_BLOCKS, texLoc));
}
ICTMTexture<?> tex = e.getValue().makeTexture(sprite, spriteGetter);
layers |= 1 << (tex.getLayer() == null ? 7 : tex.getLayer().ordinal());
BlockRenderLayer renderLayer = tex.getLayer();
if (renderLayer != null) {
extraLayers.add(renderLayer.getRenderType());
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

textureOverrides.put(Pair.of(e.getIntKey(), texLoc), tex);
}
}
Expand All @@ -216,6 +231,11 @@ public boolean canRenderInLayer(BlockState state, RenderType layer) {
return (layers < 0 && CTMPackReloadListener.canRenderInLayerFallback(state, layer)) || ((layers >> BlockRenderLayer.fromType(layer).ordinal()) & 1) == 1;
}

@Override
public Set<RenderType> getExtraLayers(BlockState state) {
return extraLayersView;
}

@Override
@Nullable
public TextureAtlasSprite getOverrideSprite(int tintIndex) {
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/team/chisel/ctm/client/util/Quad.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ public BakedQuad rebake() {
builder.setDirection(this.builder.quadOrientation);
builder.setTintIndex(this.builder.quadTint);
builder.setShade(this.builder.applyDiffuseLighting);
builder.setHasAmbientOcclusion(this.builder.applyAmbientOcclusion);
builder.setSprite(this.uvs.getSprite());
var format = DefaultVertexFormat.BLOCK;

Expand Down Expand Up @@ -508,6 +509,8 @@ public static class Builder {

@Setter
private boolean applyDiffuseLighting;
@Setter
private boolean applyAmbientOcclusion;

private final float[][] positions = new float[4][];
private final float[][] uvs = new float[4][];
Expand All @@ -519,6 +522,7 @@ public void copyFrom(BakedQuad baked) {
setQuadTint(baked.getTintIndex());
setQuadOrientation(baked.getDirection());
setApplyDiffuseLighting(baked.isShade());
setApplyAmbientOcclusion(baked.hasAmbientOcclusion());
var vertices = baked.getVertices();
for (int i = 0; i < 4; i++) {
int offset = i * STRIDE;
Expand Down
4 changes: 2 additions & 2 deletions src/main/resources/META-INF/mods.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
modLoader="javafml"
loaderVersion="[38,)"
loaderVersion="[43,)"
issueTrackerURL="https://github.com/Chisel-Team/ConnectedTexturesMod/issues/"
license="GPL-2.0"

Expand All @@ -17,6 +17,6 @@ license="GPL-2.0"
[[dependencies.ctm]]
modId="forge"
mandatory=true
versionRange="[38.0.17,)"
versionRange="[43.1.24,)"
ordering="NONE"
side="BOTH"