Ticket #472 (closed task: fixed)

Opened 3 years ago

Last modified 13 months ago

Fixed-point CTerrain::GetExactGroundLevel

Reported by: Philip Owned by: nobody
Priority: Should Have Milestone: Pre-Alpha 2
Component: UI & Simulation Keywords: simple review
Cc:

Description

Need to reimplement CTerrain::GetExactGroundLevel using fixed-point instead of floating-point, so it can be safely used by CCmpTerrain::GetGroundLevel.

Attachments

472.patch (2.7 KB) - added by JubJub 3 years ago.
472_test.patch (3.5 KB) - added by JubJub 3 years ago.

Change History

comment:1 Changed 3 years ago by Philip

  • Keywords simple added

comment:2 Changed 3 years ago by Philip

  • Status changed from new to assigned

comment:3 Changed 3 years ago by Philip

  • Owner changed from Philip to nobody
  • Status changed from assigned to new

(Um, I don't mean to have this assigned to myself.)

Changed 3 years ago by JubJub

comment:4 Changed 3 years ago by JubJub

  • Keywords review added

My first little contribution :). To me its working fine, the new method has only little difference (less than 0.2 in my tests) compared to the old one which i think is ok, is it?

comment:5 Changed 3 years ago by Philip

Looks fine to me! The CFixed_23_8 is a bit inaccurate for small numbers, so that kind of error seems inevitable - it'd probably be nice to add something like a CFixed_15_16 in the future, but this is okay for now.

Before committing this patch, it would be good to have tests (particularly for edge cases like very high terrain). graphics/tests/test_Terrain.h has test_CalcNormalFixed etc which is the same basic idea, so could you add something similar for the new method?

Changed 3 years ago by JubJub

comment:6 Changed 3 years ago by JubJub

I added a few tests, especially high terrain. I'm not sure about other edge cases. It looks like it only differs a little in high terrain which is suprising if CFixed_23_8 is inaccurate with small numbers only - or is it float which is inaccurate? Besides there is much less difference than i expected, far less than i noticed ingame. I have to check again later.

comment:7 Changed 3 years ago by philip

  • Status changed from new to closed
  • Resolution set to fixed

(In [7443]) Fix #472 (fixed-point CTerrain::GetExactGroundLevel?), based on patch from JubJub?

comment:8 Changed 3 years ago by Philip

Thanks! Do you have a name and/or public email address I could add to the contributors list?

The CFixed can only represent multiples of 1/256, so if xf etc aren't nice numbers like 0.5 then that could probably cause a ~1% error overall. But I just realised this function's linear interpolation won't actually match the rendered terrain anyway, because the terrain is flat triangles rather than a curved quad, and #169 makes it impossible to know what the triangles are. So this will look a bit inaccurate, but it's fine for now.

comment:9 Changed 3 years ago by JubJub

Sorry for the delay - have been busy this week. My name is Sebastian Vetter and email is jubjub@…. I'm feeling a little proud now being an official contributor to this amazing project :)

Does the inner state of the terrain really depend on the rendering? I think there should be the height defined in terrain with heightmap and algorithms and rendering should just display it correct - not the other way round.

comment:10 Changed 3 years ago by philip

(In [7454]) Update source contributors list (see #472)

comment:11 Changed 3 years ago by Philip

Sorry, I don't think I explained it well. To try again: The terrain values used by the gameplay code shouldn't depend on the rendering, but they should match each other. The heightmap defines the vertex positions but there isn't an unambiguously correct way to compute the heights between vertices. GetExactGroundLevel uses one way (bilinear interpolation). The renderer uses a different way (split each tile into two flat triangles, which is the only way it can render non-flat squares). But the renderer can split in one of two different directions and currently the OpenGL drivers decide, which is bad - we ought to make a single consistent choice ourselves (as in #169). Once we know what the renderer is doing, then we can update GetExactGroundLevel to do the same, so that they match.

comment:12 Changed 3 years ago by anonymous

  • Milestone Unclassified deleted

Milestone Unclassified deleted

comment:13 Changed 13 months ago by historic_bruno

  • Milestone set to Pre-Alpha 2
Note: See TracTickets for help on using tickets.