Remove duplicated call in void TerrainOverlay::RenderTile
ClosedPublic
Actions

Authored by vinhig on Jun 12 2020, 4:29 PM.

Details

Reviewers

wraitii

Commits

rP23767: Remove duplicated call in tile rendering for TerrainOverlay.

Summary

m_Terrain->CalcPosition was called 12 times in that method but we just need 4 different calls.

Edit:
It's called just 6 times

Test Plan

Start Atlas editor and use the brush to modify terrain. If it doesn't crash and if 9 wonderful green squares are shown, it means that my little modfications work well :)

Need someone to PROFILE2 this, I didn't manage to have it working...

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vinhig created this revision.Jun 12 2020, 4:29 PM

Owners added a subscriber: Restricted Owners Package.Jun 12 2020, 4:29 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/renderer/TerrainOverlay.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2405/display/redirect

Harbormaster completed remote builds in B12060: Diff 12259.Jun 12 2020, 4:37 PM

I forgot to add vinhig as a contributor and to change the date at the beginning of the file.

Thanks and congratulations on your first patch :)
The code was not actually calling CalcPosition 12 times but 6 times, since only one branch of GetTriangulationDir is taken each time. However, you're still cutting it down from 6 to 4 (I checked the assembly).

It's a little frustrating that this 0-initialises the 4 vectors as CalcPosition takes the actors by argument, but the cost of 3 new 0-initialisations is very likely more than compensated by the 2 fewer calls to CalcPosition.

I would suggest using pos_i0_j0 and pos_i1_j1 instead of ip1 as that makes them the same length and it's probably more readable. I also don't think you really need the 'precompute positions' comment.

vladislavbelov added a subscriber: vladislavbelov.Jun 12 2020, 4:56 PM

vladislavbelov added inline comments.

source/renderer/TerrainOverlay.cpp
188 ↗	(On Diff #12261)	I'd suggest to do something like: CVector3D pos[2][2]; for (int dx = 0; dx < 2; ++dx) for (int dy = 0; dy < 2; ++dy) m_Terrain->CalcPosition(i + dx, j + dy, pos[dx][dy]);

I applied all of your comments ^^

Looks good, straightforward change. Feel free to update the diff with the inline cleanups, otherwise I'll do it when committing.

source/renderer/TerrainOverlay.cpp
165 ↗	(On Diff #12267)	You would add PROFILE2("TO::RenderTile"); here for profiling usually, but since this is atlas only it seems you won't really be able to use it as the profiler doesn't appear to work correctly with atlas :/ No buggy for this diff though.
189 ↗	(On Diff #12267)	In general, we don't use `{}` if we don't have to, and when we do we use Allman style: if (something) { } In this instance, you can drop the {}