Ticket #472 (closed task: fixed)
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
Change History
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.)
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?
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
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
