avoid extra loops when terrain does not exist

[zhangyiatmicrosoft]

Launch Checklist

placeSymbol is a very hot code path if not the hottest. For most screen sizes, this function is called 1000 to 2000 times.
When terrain does not exist, it is still doing a loop on 4-element array ['textBox', 'verticalTextBox', 'iconBox', 'verticalIconBox'] which means 4000 to 8000 extra loops.

Please review --- before my change, textBox had elevation of 0, and now it would be undefined.

However, I cannot find anywhere the elevation of textBox is used for placement or collision. The previous code was added in #1022, @HarelM @prozessor13, can you share more insights regarding where and how it is used?
There is no UT coverage either.

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.

[zhangyiatmicrosoft]

@HarelM any suggestions?
Giving the fact that this code does not have test coverage, it is very likely something prepared for future.
Can we merge this change? Seems like a very simple and healthy one.

[HarelM]

I don't know what this was for unfortunately, maybe @prozessor13 remembers.
In any case, a single unit test to cover this scenario would be helpful to cover this case and allow merging.
I know there are missing unit tests around this area, which was related to the effort around 3D terrain, but I still think this is needed.

[prozessor13]

After a quick look into the code, because i cannot remember, i think the whole getElevtaion logic at this place is no longer needed. I think it is a leftover from the days, where all elevation is calculated in CPU not in shader.
Originally i think it was needed to place the collision-box at correct height, today the get_elevation shader function is used for this.

Regards.

[HarelM]

Thanks for the quick response @prozessor13!
Do we have a way to validate this assumption? If we remove this code, what do we need to check to make sure it's not needed?

[prozessor13]

I think its sufficient to switch on collision-boxes, and visually check if they have the right positions. On the other side there exists render-tests which would fail if something goes wrong.

[HarelM]

@zhangyiatmicrosoft I think the best approach is to do what @prozessor13 suggested.
There's a debug page with terrain that I think you can use to try out things, render-test should also help us avoid breaking things as @prozessor13 mentioned.

[zhangyiatmicrosoft]

@zhangyiatmicrosoft I think the best approach is to do what @prozessor13 suggested. There's a debug page with terrain that I think you can use to try out things, render-test should also help us avoid breaking things as @prozessor13 mentioned.

Thank you all. Deleted the block.
getElevation function is still needed later but it does not depend on placeSymbol function so I moved it outside to have better performance.