Fix rendering of UP face for waterlogged blocks with translucent sides#3775
Fix rendering of UP face for waterlogged blocks with translucent sides#3775Auth0x78 wants to merge 4 commits into
Conversation
…t side was not rendered
There was a problem hiding this comment.
I think it would be more appropriate if we used the voxel shape that we construct based on the fluid height for culling the fluid against the block that contains it. This voxel shape is used to cull the fluid against other blocks, and we could also use it to clear the fluid against its own block. Passing the fluid height into the shape comparison goes against the idea of the shape comparison system.
I appreciate your effort in resolving this issue, but I think there's a more appropriate solution than this.
|
I have done the necessary changes. I get that passing fluidHeight is redundant and defeats the whole point of shape comparision. So removed it from func parameter and so now before calling the main function for checking visibility of fluid with self we construct a VoxelShape that better represents the fluid block. Thank you for the feedback. |
| if (fluidHeight >= 1.0F) { | ||
| fluidShape = Shapes.block(); | ||
| } else { | ||
| fluidShape = Shapes.box(0.0D, 0.0D, 0.0D, 1.0D, fluidHeight, 1.0D); |
There was a problem hiding this comment.
my understanding is that we already have this same code elsewhere in this class. It seems to me like some restructuring may be necessary.
There was a problem hiding this comment.
Ya I saw it somewhere in the code and also as a precautinory measure if somehow we end up with height greater than 1.0f
There was a problem hiding this comment.
I think we should not duplicate the construction of the fluid shape.
There was a problem hiding this comment.
Then maybe we can maybe abstract it out into the render function?
It we can use the same new constructed fluidShape for all other face's visibility check too
There was a problem hiding this comment.
No need for the above comment too, actually found out an even better way to do it.
| if (ShapeComparisonCache.isFullShape(selfShape) && ShapeComparisonCache.isFullShape(fluidShape)) { | ||
| return false; | ||
| if (ShapeComparisonCache.isFullShape(selfShape)) { | ||
| return !ShapeComparisonCache.isFullShape(fluidShape); |
There was a problem hiding this comment.
what is the reason for this change? I think this changes the behavior.
There was a problem hiding this comment.
The main reason for this was even when the ShapeComparisonCache.isFullShape(fluidShape) returned FALSE, we fall through to the line return this.occlusionCache.get().lookup(fluidShape, selfShape)
We check in occlusion cache, but the issue is that during first render, the cache is empty (no block data as 1st render), so the lookup() in cache create a new entry and the default creation value is FALSE.
Hence the function returns false and the UP face of the fluid is not visible.
There was a problem hiding this comment.
can we simplify this logic to simply return false when ShapeComparisonCache.isFullShape(selfShape)? The check for ShapeComparisonCache.isFullShape(fluidShape) seems redundant to me when the full self shape already culls everything.
There was a problem hiding this comment.
We have to keep it as in the case of the issue that this resolves there might be the case that the where ShapeComparisonCache.isFullShape(selfShape) returns true, but as the sides of the selfBlock are transparent we need to render the UP face. So in that aspect we need to keep the ShapeComparisonCache.isFullShape(fluidShape) there.
There was a problem hiding this comment.
Either we check the isFullShape(fluidShape) or other way is we can check each side wall of the block and see if its full shape or not, but it will require us to loop over North, South, East and West facing wall surfaces of the current block, which might be less optimal than the current approach although a bit more accurate.
Sorry for the delayed response.
There was a problem hiding this comment.
The culling shape of a block should match what the actual model represents. If the model says it's full, then we should be allowed to rely on that fact to cull anything within it as determined by the culling shape.
There was a problem hiding this comment.
But here var selfShape = selfBlockState.getFaceOcclusionShape(facing); as we can see that the selfShape is a VoxelShape of the surface in the direction of facing and not the entire block's VoxelShape.
|
@douira any other changes, I will make them |
|
I found that we dont require to manually calculate height and then construct VoxelShape for accurate fluid shape, we can simply query |
|
@douira can you review the new changes and if any issue ill try to fix them. |
2 participants
Occlude the UP face only when the waterlogged fluid height is below 1.0f, as the side faces may be translucent.
Issue resolved:
#3197