-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add 3D layer offset as style property #378
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In favour of merging this feature, but I am having trouble understanding the changes a bit. I think some comments or better variable names or refactoring would help.
@@ -387,6 +387,7 @@ class HexagonGame | |||
void drawTrailParticles(); | |||
void drawSwapParticles(); | |||
void drawImguiLuaConsole(); | |||
void drawMainLayer(const auto getRenderStates); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True I tried implementing this in another way initially but ended up not using it and forgot to remove it from the header.
render(wallQuads3DTop, getRenderStates(RenderStage::WallQuads3D)); | ||
render(pivotQuads3DTop, getRenderStates(RenderStage::PivotQuads3D)); | ||
render(playerTris3DTop, getRenderStates(RenderStage::PlayerTris3D)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this also be wrapped in if (renderAbove) { ... }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
for(std::size_t k = jAdj * numPlayerTris; | ||
k < (jAdj + 1) * numPlayerTris; ++k) | ||
{ | ||
playerTris3D[k].position += newPos; | ||
playerTris3D[k].color = overrideColor; | ||
if(renderingAbove) | ||
{ | ||
playerTris3DTop[k].position += newPos; | ||
playerTris3DTop[k].color = overrideColor; | ||
} | ||
else | ||
{ | ||
playerTris3D[k].position += newPos; | ||
playerTris3D[k].color = overrideColor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull the if
statement outside of the loop, so that we have if (renderAbove) { for{...} } else { for{...} }
.
for(std::size_t k = jAdj * numWallQuads; | ||
k < (jAdj + 1) * numWallQuads; ++k) | ||
{ | ||
wallQuads3D[k].position += newPos; | ||
wallQuads3D[k].color = overrideColor; | ||
if(renderingAbove) | ||
{ | ||
wallQuads3DTop[k].position += newPos; | ||
wallQuads3DTop[k].color = overrideColor; | ||
} | ||
else | ||
{ | ||
wallQuads3D[k].position += newPos; | ||
wallQuads3D[k].color = overrideColor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull the if
statement outside of the loop, so that we have if (renderAbove) { for{...} } else { for{...} }
.
for(std::size_t k = jAdj * numPivotQuads; | ||
k < (jAdj + 1) * numPivotQuads; ++k) | ||
{ | ||
pivotQuads3D[k].position += newPos; | ||
pivotQuads3D[k].color = overrideColor; | ||
if(renderingAbove) | ||
{ | ||
pivotQuads3DTop[k].position += newPos; | ||
pivotQuads3DTop[k].color = overrideColor; | ||
} | ||
else | ||
{ | ||
pivotQuads3D[k].position += newPos; | ||
pivotQuads3D[k].color = overrideColor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull the if
statement outside of the loop, so that we have if (renderAbove) { for{...} } else { for{...} }
.
const float aboveMain(-styleData._3dLayerOffset - 1); | ||
const bool renderAbove(aboveMain > 0 && !styleData._3dMainOnTop); | ||
const float layerOffset(renderAbove ? -1.f : styleData._3dLayerOffset); | ||
const float depth(styleData._3dDepth - renderAbove * aboveMain); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments would be appreciated here, I don't find this code obvious, especially what aboveMain
means and the role of layerOffset
. Also maybe rename the variables a bit to be more meaningful, like mustRender3DAboveMain
instead of renderAbove
?
playerTris3DTop.reserve(numPlayerTris * aboveMain); | ||
} | ||
|
||
if(depth > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have a comment explaining why this only happens when depth > 0
.
(styleData._3dAlphaMirror ? std::abs(i + 1) : i) * | ||
styleData._3dAlphaFalloff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, this expression is pretty complicated. Either comments or maybe splitting it up would help.
|
||
c.a = Utils::componentClamp(newAlpha); | ||
}; | ||
|
||
for(int j(0); j < static_cast<int>(depth); ++j) | ||
for(int j(0); | ||
j < static_cast<int>(renderAbove ? depth + aboveMain : depth); ++j) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract static_cast<int>(renderAbove ? depth + aboveMain : depth)
to a const
variable outside of the loop.
const bool renderingAbove(j >= depth && renderAbove); | ||
const float jAdj(j - depth * renderingAbove * (depth >= 0)); | ||
const float i(depth - j - 1 + layerOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, especially confused about jAdj
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit confusing because i didn't want to split it into two loops, it's so it can switch to the next vertex container in the same loop. It's probably a better idea to just split it into two loops.
Originally I made this to shift 3D layers up by one for 1.92 parity (for the level converter), but it turned out that quite a lot of fun effects can be done with this, so I also added the ability to render 3D layers above the main layer and a way to mirror the alpha falloff as well as the option to render the main layer on top of all 3D layers.