[PATCH] Camera height

[frapell]

There's like a "ceiling" where the camera cannot zoom out anymore.

This ceiling appears to be related to the terrain, so in maps like "Death Canyon", when you go over the canyon itself, the camera zooms in, and when you go back to your camp, you are really close to the units, and need to zoom out, which gets very annoying.

I think there should be a "ceiling", but how deep or tall the terrain is, shouldn't affect it.

[Philip Taylor]

There's a min/max limit on the distance from the camera to the point of terrain that's at the center of the screen. The idea was that it should be independent of absolute height - the zoom range when looking at a low plateau should be the same as when looking at a high plateau. If it was height-independent then you'd be able to see much more at once of the low plateau, and we'd probably need to make maps flatter to keep the view more consistent. Maybe that's better than the current slightly weird behaviour, though.

[frapell]

Instead of making the maps flatter, i think a better approach would be having the "zoom level" something not global, but local.

Say, you are 5 "units" away from the ground, no matter if the ground is high or low, you should always be 5 "units" away from it... it should zoom in and zoom out, automatically, and you always should be at the same height (to the ground).

Having to manually change the zoom every time you move the camera through the terrain gets really annoying.

[Philip Taylor]

I think the problem with that approach is the camera will seem very bumpy when you're panning across uneven ground, or very jerky if you pan onto/off a cliff, if it's trying to keep a constant distance between the camera and the point at the center of the screen.

(Hmm, I wonder if it'd work to make a smoothed/blurred version of the terrain heightmap and base the camera on distance to that, to avoid those problems with sudden height fluctuations...)

[Matthias Cullmann]

another approach to avoid bumpy panning would be a timeout: adjust zoom x msecs after last panning

[Jonathan Waller]

I have had a look at this. I am not very experienced with C++ so this isn't a full review. There are two things I found.

First there was a compile error on linux 64 bit with lines 59-63 needing an explicit cast for the 0 argument so it looks like (ssize_t)0.

Secondly with your new definition of the zoom the zooming in and out has been reversed so the shortcuts need appropriate modification. The affected lines are 809, 811, 1089, 1094.

In general it looked pretty good to me, it is a nice feature.

[Philip Taylor]

More detailed review:

• Looks like a good design - the camera movement seems to work very nicely.
• Disabling "restrict camera" (press alt-d to open dev controls) should allow the camera to freely move through the terrain, but now it gets restricted when zooming.
• Assertion failure when I run Atlas (add "-editor" to command-line arguments), with call stack

  #6  0x00000000009f28a0 in CHeightMipmap::Update (this=0x7fffbc0af588, mapSize=257, ptr=0x0, left=0, top=0, right=257, bottom=257) at ../../../source/graphics/HeightMipmap.cpp:55
  #7  0x00000000009f280b in CHeightMipmap::Update (this=0x7fffbc0af588, mapSize=257, ptr=0x0) at ../../../source/graphics/HeightMipmap.cpp:50
  #8  0x00000000009f2c6c in CHeightMipmap::Initialize (this=0x7fffbc0af588, mapSize=257, ptr=0x0) at ../../../source/graphics/HeightMipmap.cpp:86
  #9  0x00000000008f65d5 in CTerrain::Initialize (this=0x7fffbc0af560, patchesPerSide=16, data=0x0) at ../../../source/graphics/Terrain.cpp:95
  #10 0x0000000000906a18 in CXMLReader::ReadTerrain (this=0x2ee34f0, parent=...) at ../../../source/graphics/MapReader.cpp:525
  #11 0x0000000000909eb2 in CXMLReader::ProgressiveRead (this=0x2ee34f0) at ../../../source/graphics/MapReader.cpp:973
  #12 0x000000000090a3f3 in CMapReader::ReadXML (this=0x7fffbc2863f0) at ../../../source/graphics/MapReader.cpp:1061
  

• The initial CHeightMipmap::Update seems to take about 2ms on an Oasis-sized map (on Core 2 2.16GHz). Update performance is only really a concern when editing maps in Atlas, and it looks like that'll do incremental updates for the usual terrain editing tools so it should be okay, though it'd be nice to test that.

GameView.cpp

• "To prevent accessive zoom" - should be "excessive".
• Instead of repeating

  CVector3D pivot = targetCam.m_Orientation.GetTranslation() + targetCam.m_Orientation.GetIn() * m->Zoom.GetSmoothedValue();
  

  so many times, there should probably be a "GetPivot(m)" or similar.

HeightMipmap.cpp

• (0,0,0) is the bottom-left corner of the map in the default camera view, so minimum Z is conventionally 'bottom'. The top/bottom parameters in this code seem to be the other way round.
• Should use lib/bits.h round_up_to_pow2 to compute mipmapSize (instead of adding log2).
• GCC warnings:

  HeightMipmap.cpp: In member function ‘void CHeightMipmap::HalfResizeUpdate(SMipmap&, ssize_t, const uint16_t*, size_t, size_t, size_t, size_t)’:
  HeightMipmap.cpp:135: warning: comparison of unsigned expression >= 0 is always true
  HeightMipmap.cpp:135: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp:136: warning: comparison of unsigned expression >= 0 is always true
  HeightMipmap.cpp:136: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp:137: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp:138: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp: In member function ‘void CHeightMipmap::BilinearUpdate(SMipmap&, ssize_t, const uint16_t*, size_t, size_t, size_t, size_t)’:
  HeightMipmap.cpp:162: warning: comparison of unsigned expression >= 0 is always true
  HeightMipmap.cpp:162: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp:163: warning: comparison of unsigned expression >= 0 is always true
  HeightMipmap.cpp:163: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp:164: warning: comparison between signed and unsigned integer expressions
  HeightMipmap.cpp:165: warning: comparison between signed and unsigned integer expressions
  

• Should use ENSURE instead of ASSERT, since it helps debug crashes in release builds. (ASSERT is only needed in performance-critical code where the test is measurably expensive.)
• Should follow the standard formatting described in Coding_Conventions - whitespace around brackets, brace placement, etc.

HeightMipmap.h

• Should use

  class CHeightMipmap
  {
      NONCOPYABLE(CHeightMipmap);
  public:
      ...
  

  to prevent copies at compile-time, instead of overriding operator=.
• "Describes ground via heightmap mimpaps" - typo.

[Kieran P]

@Dietger Hey. Thanks for your patch. See the ticket for some notes from Philip regarding some changes required. Would you be interested in finishing off this patch? Hopefully we can get it into Alpha 10.

[Dietger]

Makes camera zoom a local distance to trilinear filtered heightmap. Filter radius is proportional to zoom distance. Uses a mipmap for fast filtering.

[Dietger]

I updated the patch, incorporating the comments from Quantumstate and Philip.

[Philip Taylor]

Thanks for this, and sorry for slow response again...

I see an assertion failure when running tests - it gets to the call stack

#0  CHeightMipmap::Update (this=0x7fffffffd668, ptr=0x125ee00, left=0, bottom=0, right=64, top=64) at source/graphics/HeightMipmap.cpp:52
#1  CTerrain::FlattenArea (this=0x7fffffffd640, x0=-1000, x1=1000, z0=-1000, z1=1000) at source/graphics/Terrain.cpp:618
#2  TestTerrain::test_FlattenArea (this=0x1225be0) at source/graphics/tests/test_Terrain.h:205

with CHeightMipmap::m_MapSize == 65. The first loop iteration in Update gets left == right == top == bottom == 0, which makes BilinearUpdate unhappy.

But we don't actually use FlattenArea, so I deleted it (r11540). But the height mipmap needs updating when you edit terrain in Atlas, so I added a call from CTerrain::MakeDirty, which triggers the same problem. Seems to fail because "right = clamp(... (right / mapSize) ...)" is doing integer division and will usually become 0. I changed it to do float division, and then it seems to work better. I'm not totally convinced these bounds are precisely correct, and that it won't occasionally forget to update a few pixels around the edges of the changed area, but I haven't seen any visible errors so I'll assume it's alright :-)

I also added a command to dump the mipmap data as a PNG file ("Engine.DumpTerrainMipmap()" from the in-game console or from the JS console in Atlas), mainly to help debugging the issues with updating the mipmaps in Atlas.

Found a few other things to change:

• Pressing 'H' (should reset the camera to default zoom/orientation) seems to zoom in the wrong direction - if you're close to the terrain it zooms in, if you're further away then it zooms out. (Particularly noticeable when "restrict camera" is disabled). I added a negation in CGameView::ResetCameraAngleZoom which seems to fix that.
• VS2008 doesn't have hypot; seems easiest to stick with sqrtf(x*x+y*y).
• In FocusHeight, I sometimes see diff alternating between -0.000008 and 0.000008 forever. I changed the "if (!diff)" to "if (fabsf(diff) < 0.0001f)" to try to avoid that floating-point-precision issue.
• If you disable "restrict camera", zoom the camera in so its Y is negative, then re-enable restrict camera, the Y coordinate becomes NaN and it's impossible to get the camera back to valid coordinates. Looks like that's because GetTrilinearGroundLevel gets radius <= 0, so logf gives NaN, and clamp(NaN, 0, size) == NaN. I added a check for non-positive radiuses to avoid that.
• BilinearFilter/BilinearUpdate clamped coordinates to the range [0, mapSize-1] and then accessed m_Heightmap[(z+1)*mapSize + (x+1)], which I think means it can exceed the array bounds. I changed them to clamp to [0, mapSize-2] instead.
• Tidied up a bit of whitespace.

Apart from that, looks good, so I'll commit with those changes.

[philip]

In 11556:

Compute camera height and zoom limits based on smoothed terrain heightmap. Fixes #794, based on patch by Dietger.

[elexis]

In r19896:

Ensure the camera position being above the water level (without smoothing the water level with the terrain).

Differential Revision: ​https://code.wildfiregames.com/D700
Fixes #3892, ​Phab:D229, ​Phab:D638