Batch shroud cell changes #7723
Conversation
|
Travis failed, just a style error though:
|
|
Haven't tested this thoroughly for regressions yet (though I didn't spot any in my short test), but with massive number of migs, this improves performance approximately as much as an update delay of 15-20 in #7669, without the drawbacks. So a preliminary :+0.5: for the performance improvement. |
| @@ -515,6 +515,9 @@ static void RenderTick() | |||
| } | |||
| } | |||
|
|
|||
| foreach (var player in Game.worldRenderer.World.Players) | |||
| player.Shroud.ResetChangedCells(); | |||
There was a problem hiding this comment.
I suspect there will be a better place to run this from.
There was a problem hiding this comment.
I can't do the reset until the widgets have drawn, since the radar widget also needs this list.
Unless you just mean calling this as a static method of ShroudRenderer or something?
RoosterDragon
force-pushed
the
batch-shroud-cell-changes
branch
from
303b66a
to
79078b1
Compare
|
OH YES. 👍 |
RoosterDragon
force-pushed
the
batch-shroud-cell-changes
branch
from
79078b1
to
3737ea3
Compare
|
After a bit of IRC discussion, I've reworked the caching slightly. Instead of the shroud maintaining a copy of changed cells until the next render, the shroud renderer and radar widget now each maintain their own copy.
This means performance isn't quite as good as we need to maintain two sets instead of one, but it does mean a cleaner design. |
|
Spotted no regressions and performance is still a lot better than without this pr ( with migs worth ~200 batches, ~25ms vs. ~45ms). |
| cellsAndNeighborsDirty.Add(cell + new CVec(1, 0)); | ||
| cellsAndNeighborsDirty.Add(cell + new CVec(-1, 1)); | ||
| cellsAndNeighborsDirty.Add(cell + new CVec(0, 1)); | ||
| cellsAndNeighborsDirty.Add(cell + new CVec(1, 1)); |
There was a problem hiding this comment.
You may be able to use CVec.Directions here to ease readability.
By maintaining a set of changed cells we can avoid repeating work for cells that change multiple times before being rendered. The shroud renderer and radar widget now delay their work until they must render, and thus process each changed cell only once. This avoids significant repetition that was causing major slowdown when many actors were in the world.
This reduces us to one conversion per cell rather than two or three.
RoosterDragon
force-pushed
the
batch-shroud-cell-changes
branch
from
3737ea3
to
d44830c
Compare
|
No regressions for playing field and radar widget visible. 👍 |
|
Thanks! |
Significantly improves the perf issue noted in #7669 when you have many moving actors causing a lot of shroud changes.
This change reduces repeated work when actor visibilities overlap and when units move. Moving individual units should be slightly less costly in terms of performance now, and moving large groups of units together should be significantly less costly.
This shouldn't affect any in-game functionality - in particular when testing you should verify the radar and the shroud/fog still act correctly.