Rewrite facade rendering #945
Conversation
|
This could be improved quite a bit if there was a map of chunks that had facades; but adding that right now seems potentially out of scope. |
|
I've added a chunk map, fixing the above issue. It will check if the chunk has any facades, skipping the expensive lookup step. |
There was a problem hiding this comment.
I think most of this is good, just some small nitpicks. I do however think an alternative path to not render anything (or a replacement transparent overlay) when shaders are active is needed. This should be linked to a config as wel IMO (I don't mind doing this myself if you haven't worked with the neo config system yet). Thanks for the PR!
| @@ -79,7 +83,8 @@ public class ConduitBundleBlockEntity extends EnderBlockEntity { | |||
| public static final String CONDUIT_INV_KEY = "ConduitInv"; | |||
|
|
|||
| @UseOnly(LogicalSide.CLIENT) | |||
| public static final Map<BlockPos, BlockState> FACADES = new HashMap<>(); | |||
| public static final Long2ObjectMap<BlockState> FACADES = new Long2ObjectOpenHashMap<>(); | |||
| public static final Long2ObjectMap<Set<BlockPos>> CHUNK_FACADES = new Long2ObjectOpenHashMap<>(); | |||
There was a problem hiding this comment.
Just a small thing, can this be a LongSet instead of Set<BlockPos>? We do need to convert it back to a blockpos, but it may be a bit faster?
| @@ -217,7 +228,8 @@ public void onChunkUnloaded() { | |||
| ConduitSavedData savedData = ConduitSavedData.get(serverLevel); | |||
| bundle.getConduits().forEach(type -> onChunkUnloaded(savedData, type)); | |||
| } else { | |||
| FACADES.remove(worldPosition); | |||
There was a problem hiding this comment.
just a heads up, I added another remove in this file a bit after your PR, so there's a small conflict you'll have to solve.
| import org.spongepowered.asm.mixin.Mixin; | ||
| import org.spongepowered.asm.mixin.injection.At; | ||
|
|
||
| @Mixin(BlockRenderDispatcher.class) |
There was a problem hiding this comment.
Since we don't have many mixins in enderio, do you mind writing a short explanation what this does/fixes? I think this is for the breaking overlay, but want to make sure.
| model.getModelData(level, entry.getKey(), entry.getValue(), ModelData.EMPTY), | ||
| renderType); | ||
| .tesselateBlock(context.getRegion(), model, state, pos, context.getPoseStack(), consumer, | ||
| true, random, 42L, OverlayTexture.NO_OVERLAY, ModelData.EMPTY, renderType); |
There was a problem hiding this comment.
Here (and above as well) ModelData.EMPTY should not be used, but the correct ModelData should be fetched from the model and position. This is needed for some blocks to render properly.
| static void renderFacade(RenderLevelStageEvent event) { | ||
| if (event.getStage() != RenderLevelStageEvent.Stage.AFTER_TRIPWIRE_BLOCKS || FacadeHelper.areFacadesVisible()) { | ||
| static void renderFacade(AddSectionGeometryEvent event) { | ||
| Map<BlockPos, BlockState> facades = new Object2ObjectOpenHashMap<>(); |
There was a problem hiding this comment.
The map allocation should be moved below the if (blockList == null) check to avoid allocating a redundant map for sections without facades (i.e. most sections, including empty ones).
| renderTypes = ChunkRenderTypeSet.union(renderTypes, facadeRenderTypes); | ||
| } | ||
| return renderTypes; | ||
| return ChunkRenderTypeSet.of(RenderType.cutout()); |
There was a problem hiding this comment.
This should be cached in a static field to avoid allocation.
Description
This rewrites facade rendering to always use the additional chunk buffering system. Additionally, bugs have been fixed with shaders and mods, and the block breaking overlay now correctly shows when breaking a facade.
Checklist