beyond-all-reason / spring

A powerful free cross-platform RTS game engine
https://beyond-all-reason.github.io/spring/
Other
219 stars 101 forks source link

Slopes speed modifier miscalculation #828

Open tyggerr opened 1 year ago

tyggerr commented 1 year ago

MoveMath uses average slopes for the speed modifier, but individual triangles' normals to check for downward/upward.

This means that on a steep upward slope a unit could get a movespeed increases if a single triangle is downward

https://github.com/beyond-all-reason/spring/blob/d69fb498bed99f075f37df480ea970ceb3c7c686/rts/Sim/MoveTypes/MoveMath/MoveMath.cpp#L82

https://github.com/beyond-all-reason/spring/blob/d69fb498bed99f075f37df480ea970ceb3c7c686/rts/Sim/MoveTypes/MoveMath/MoveMath.cpp#L86

sprunk commented 1 year ago

Can there actually be a downward triangle on an upward slope? (Toy example?)

lhog commented 1 year ago

Yes, @tyggerr please be more specific. A triangle should never actually face down.

tyggerr commented 1 year ago

Slopes are determined by averaging a 2x2 square grid. Each square being composed of two triangles. So each cell of the pathfinding is built out of 4x4 triangles. However, the code deciding whether a unit is moving upward or downward uses a single triangle normal.

So a (side view) setup looking like this : image

Would result in a slope of ~0.5. But the unit traversing it would get a movement speed increase between the # 1 and # 2 points, because that triangle is facing downward.

I realise that this is a minor detail, but I wanted to note that different metric sizes were used in the same checks (4 triangles wide vs 1 triangle wide)

lhog commented 1 year ago

Your description is not 100% accurate. A single triangle normal is never used to determine dirSlopeMod.

Roughly speaking the slope is an average of Y component of 8 triangle normal vectors, making up 4 squares (one the unit stays at + 3 positively adjacent ones).

A cell in centerNormalsSynced on the other hand is calculated by averaging normal vectors of 2 triangles, making up a square the unit stays at. centerNormals2D is constructed similarly, except the normalization is done with for only XZ plane, Y component is nullified. Quick search didn't show any other use of center normals (2D or 3D), other than for the purpose of dirSlopeMod calculation, so we could reduce their resolution in half effectively making it operate on the same "level of details" as slopes array.

We could, but I'm not sure we should.

lhog commented 1 year ago

If you feel like it go ahead and remove centerNormalsSynced, centerNormalsUnsynced, sharedCenterNormals, all references to them (hopefully they are indeed unused); rename (or keep the name) centerNormals2D and change the calculation such that it 2D normalizes 8 normal vectors instead (the same normal vectors as the one used for slope calculation).

Then test if it makes any perceptual differences in unit movement. My bet you won't find much difference, but at least that approach should reduce memory consumption and compute expenses a little bit.