AndrewScheidecker / BrickGame

A demo of Minecraft-style voxel rendering in UE4
399 stars 145 forks source link

Translucency correctly implemented. #24

Closed miguelemosreverte closed 8 years ago

AndrewScheidecker commented 8 years ago

I think the way this should work is by looking at whether the surface material for a brick is translucent or not:

That avoids the need for the hard-coded translucent material index, and makes it work automatically for one or many translucent materials.

miguelemosreverte commented 8 years ago

Indeed, this hard-coded translucent material index was a placeholder at the time for a list of translucent material indexes. Will get onto it.

miguelemosreverte commented 8 years ago

Done! Added some blueprint to BrickGrid ConstructionScript so that it fills the Translucent Materials List on the fly.

image

image

AndrewScheidecker commented 8 years ago

What I meant was that you can make UBrickRenderComponent::CreateSceneProxy find which materials are translucent by calling UMaterialInterface::GetBlendMode on each, without an explicit translucent flag in FBrickMaterial. That avoids the need for any setup other than assigning a translucent material to a brick.

Also, I think it's also too slow to call TArray::Contains to test whether a material index is translucent or not. Just create a TArray or FBitArray with one flag/material index, and read from it with the brick's material index to determine if it's translucent.

Finally, I think your code is producing more vertices than necessary. It's adding any vertex that has an adjacent translucent brick to the vertex buffer, even though those vertices may never be used by a face. This code should be doing something like this:

if((HasAdjacentTranslucentBricks && HasAdjacentEmptyBricks)
|| (HasAdjacentOpaqueBricks && HasAdjacentEmptyBricks)
|| (HasAdjacentOpaqueBricks && HasAdjacentTranslucentBricks))

It boils down to classifying brick materials as opaque, translucent, or empty, and then producing vertices that are shared by bricks of more than a single class.

Putting that together with the previous point, I think you should:

enum class EBrickClass
{
    Empty = 0,
    Translucent = 1,
    Opaque = 2,
    Count = 3
};
if((HasAdjacentBrickOfClass[EBrickClass::Opaque]
+   HasAdjacentBrickOfClass[EBrickClass::Translucent]
+   HasAdjacentBrickOfClass[EBrickClass::Empty]) > 1)

Thanks for taking the time to work on this!

miguelemosreverte commented 8 years ago

Thanks to you! I have said it before, will repeat it now, this project of yours is a fantastic piece of code to learn good coding practices, and most important, the hunger for optimization.

Will get onto it immediatly!

miguelemosreverte commented 8 years ago

Right now if when selecting which faces to render, the old line `

if (FrontBrickMaterial == EmptyMaterialIndex || (!Grid->Parameters.TranslucentMaterialsIndexes.Contains(BrickMaterial) && Grid->Parameters.TranslucentMaterialsIndexes.Contains(FrontBrickMaterial)))`

is changed to

if ((BrickClassByMaterial[BrickMaterial]) > (BrickClassByMaterial[FrontBrickMaterial])) or if (((int32)BrickClassByMaterial[BrickMaterial]) > ((int32)BrickClassByMaterial[FrontBrickMaterial]))

No faces are rendered at all. Will fix this soon, must go to class.

miguelemosreverte commented 8 years ago

Ha, after getting back from school looking at the culprit is like a bad joke, yet an hilarious one:

if ((BrickClassByMaterial[BrickMaterial]) > (BrickClassByMaterial[FrontBrickMaterial]))

was never going to work. Its so obvious!

Here

... to avoid crashes the iterator was set to start at 1. And EmptyMaterialIndex is 0 by default.

Now is working! image

AndrewScheidecker commented 8 years ago

Awesome, just a few small things (I'll add those comments in line) and then I think this is ready to merge.

miguelemosreverte commented 8 years ago

Done!

AndrewScheidecker commented 8 years ago

Merged. Thank you!