perf(radar): Reduce cost of radar pixel draw in PartitionManager::refreshShroudForLocalPlayer by 96% #2072
Conversation
xezon
added
Minor
|
Generals fails to compile until replicated. |
Core/GameEngineDevice/Source/W3DDevice/Common/System/W3DRadar.cpp
Outdated
Show resolved
Hide resolved
| RECT rect; | ||
| rect.left = min.I; | ||
| rect.top = min.J; | ||
| rect.right = max.I; | ||
| rect.bottom = max.J; |
There was a problem hiding this comment.
| RECT rect; | |
| rect.left = min.I; | |
| rect.top = min.J; | |
| rect.right = max.I; | |
| rect.bottom = max.J; | |
| RECT rect = { min.I, min.J, max.I, max.J }; |
There was a problem hiding this comment.
Did you try it? VC6 seems to compile for me.
Core/GameEngineDevice/Source/W3DDevice/Common/System/W3DRadar.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| const Int playerIndex = rts::getObservedOrLocalPlayer()->getPlayerIndex(); | ||
| for (int i = 0; i < m_totalCellCount; ++i) | ||
| if (m_totalCellCount != 0) |
There was a problem hiding this comment.
Maybe invert to reduce nesting?
if (m_totalCellCount == 0)
return;
There was a problem hiding this comment.
Prefer not, because it is not at the top of the function.
There was a problem hiding this comment.
Why? A few lines down is still at the top of the function.
There was a problem hiding this comment.
I am not so much a fan of returns unless they are at the start of the function or the function has a single purpose and will never change (e.g. findSomethingInContainer)
|
|
||
| Int psize; | ||
| psize=PixelSize(desc); | ||
| psize=Get_Bytes_Per_Pixel(desc.Format); |
There was a problem hiding this comment.
Combine declaration and assignment?
const Int psize=Get_Bytes_Per_Pixel(desc.Format);There was a problem hiding this comment.
Could do, but for simplicity sake the refactor is enough as is.
There was a problem hiding this comment.
Okay. I had assumed minor refactors were on the table due to the changes to Core/Libraries/Source/WWVegas/WW3D2/ww3dformat.cpp.
There was a problem hiding this comment.
Yes sure but can be separate refactor then. Keeping it extra simple :)
| m_shroudSurface = m_shroudTexture->Get_Surface_Level(); | ||
| DEBUG_ASSERTCRASH( m_shroudSurface != NULL, ("W3DRadar::beginSetShroudLevel: Can't get surface for Shroud texture") ); | ||
|
|
||
| if (surfaceRegion != NULL) |
There was a problem hiding this comment.
In what situation is surfaceRegion not NULL? The only invocation I found was TheRadar->beginSetShroudLevel()
There was a problem hiding this comment.
I provided the option to lock a subregion only. But it is unused, because refreshShroudForLocalPlayer needs the full region locked to touch every pixel.
| switch (m_shroudSurfacePixelSize) | ||
| { | ||
| case 1: *column = (UnsignedByte)color; break; | ||
| case 2: *reinterpret_cast<UnsignedShort*>(column) = (UnsignedShort)color; break; |
There was a problem hiding this comment.
Is it valid to cast Color like this? Color is AARRGGBB and casting to 2 bytes would give you GGBB and casting to 1 byte gives BB which are both zero in this case
There was a problem hiding this comment.
Indeed it looks strange. I copied this style from SurfaceClass::DrawPixel, so if this is wrong then it is original game bug (out the scope of this change).
|
Perhaps we can combine this fix with jmarshall2323@9b1c399, which makes a lot of sense too. |
3 participants
Merge with Rebase
This optimization is needed to make
HeightMapRenderObjClass::oversizeTerrainfaster, which is relevant later for terrain size extension on low camera pitch.The first commit consolidates some duplicated code.
The second commit implements the performance optimization.
Cost was measured and reduced by 96%.
TODO