[PATCH] graphics/Terrain.cpp overflow

[sanderd17]

graphics/Terrain.cpp function CalcPositionFixed gives an overflow when the final height is over 44-ish meter

The following is a quick patch that solves the problem. But maybe not the best for performance

[historic_bruno]

It's not a good idea to use floats in the simulation, since e.g. compiler optimizations can lead to different results on different systems. How are you noticing this problem?

[sanderd17]

When calculating the normal (used in #1988), and units arrive around the height of 44. The normal is completely faced he wrong way.

You can test it by applying #1988, and in Death canyon driving a ram upwards from the canyon. Around half the height, the ram will flip around once (on the tile where he meets that jump point).

It also happens on some mountains in the Acropolis maps. There the critical height seems to be exactly around the height of the mountain between the two polises. So driving around a ram there (or even a cavalry unit) makes them flip like hell.

I don't know if it's so bad that it can lead to different results. As far as I know, the normals are never stored for later use. And they're only used for visual things, not for any damage calculation. On the other hand, this could change in the future. So it might indeed be better to have it fixed.

[sanderd17]

I did a bit more investigation, and it looks like someone made a calculation mistake.

It happens when the height variable is exactly 2¹⁵ (=32768). At that point, the FromInt method returns a negative fixed. While the maximum terrain height you can get is 2¹⁶-1 (=65535), the maximum value of u16.

So I've made a better patch, where everything is kept integer. But height is divided by 2 to keep it under 2¹⁵.

[historic_bruno]

terrain.diff​ looks good, why does the other patch use an unsigned int?

[sanderd17]

terrain.2.diff was a mistake. I changed it to unsigned int for testing if the u16 height was overflowing, or the fixed pos.Y. It wasn't meant to stay that way.

And because I forgot to check the "replace original files" mark, it's still there.

It can be deleted. Btw, is there a way to automatically check that checkmark?

[ben]

In 13499:

Fixes possible overflow in terrain position/normal calculation, based on patch by sanderd17. Fixes #1986

[historic_bruno]

Thanks for the patch.

[scroogie]

Wouldn't it be better to prevent larger values instead of dividing by 2 for each and every lookup?

[sanderd17]

The heightmap gets used in a lot more places, while that was the only point where if was converted to a fixed directly.

It was the most simple solution.