Page MenuHomeWildfire Games

Camera should not submerge under water
ClosedPublic

Authored by elexis on Jul 1 2017, 6:45 AM.

Details

Summary

As reported in #3892 and especially noticeable on Extinct Volcano after an hour of gametime, the camera can move underwater as the clamping only considers terrain.
rP11556 introduced the camera terrain clamping to smoothed terrain.

Test Plan

Change waterRiseStartTime, waterIncreaseTime and waterLevelIncreaseHeight or build a custom map with huge water height in atlas.

Event Timeline

elexis created this revision.Jul 1 2017, 6:45 AM
Vulcan added a subscriber: Vulcan.Jul 1 2017, 8:13 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1676/ for more details.

Vulcan added a comment.Jul 1 2017, 8:14 AM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/275/ for more details.

wraitii requested changes to this revision.Jul 1 2017, 1:39 PM
wraitii added a subscriber: wraitii.

I'd rather you used "GetWaterHeight" or however it's called, would make multiple-water-plane transition easier if/when we get there.

This revision now requires changes to proceed.Jul 1 2017, 1:39 PM
elexis added a comment.Jul 1 2017, 1:42 PM

I do agree that this variable should rather be deleted, but I don't think the naming is crucial for this bugfix.
(I'd like to discuss with the filtering and clamping with Philip later though, the other heights look like they should be clamped too)

elexis requested review of this revision.Jul 5 2017, 10:10 AM
elexis edited edge metadata.

I can just assume that you mean GetWaterLevel and GetExactWaterLevel from the simulation component which is not accessed in this context yet, so it would probably add a significant performance impact. (Notice even those two functions return the simulation copy of m_WaterHeight)
Don't fancy implementing multiple water plane support in one area of the code when not implementing multiple water plane support.

When adding a WaterManager.h getter, function and using it only here, it would still mean that that we have 20-30 reads to the graphics copies (minimap has one too) of the waterheight.
So a patch implementing that getter must address all occurances and not only 4%.

Did you look at the actual logic change?

elexis removed a reviewer: wraitii.Jul 5 2017, 10:11 AM
elexis updated this revision to Diff 2811.Jul 5 2017, 10:19 AM

Clamp (the only use of) filtered_near_ground and filtered_pivot_ground too as mentioned above and also noticed by Philip.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1696/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/287/ for more details.

Imarok added a subscriber: Imarok.Jul 5 2017, 12:03 PM
In D700#28185, @elexis wrote:

When adding a WaterManager.h getter, function and using it only here, it would still mean that that we have 20-30 reads to the graphics copies (minimap has one too) of the waterheight.
So a patch implementing that getter must address all occurances and not only 4%.

Isn't 4% multiple water plane support better than none?
(We have to start somewhere)

In D700#28201, @Imarok wrote:
In D700#28185, @elexis wrote:

When adding a WaterManager.h getter, function and using it only here, it would still mean that that we have 20-30 reads to the graphics copies (minimap has one too) of the waterheight.
So a patch implementing that getter must address all occurances and not only 4%.

Isn't 4% multiple water plane support better than none?
(We have to start somewhere)

Were not adding any water plane support by using a getter. Either change all occurances to a getter or none. If we change all occurrances to a getter, then there shouldn't be a logic change like the one proposed in that patch.

Besides that a getter doesn't add water plane support, a GetWaterHeight(x, y) { return m_WaterHeight; } with unused X, Y doesn't help either. If you want multiple water plane support, start with the design of that patch in a ticket about that feature.

About the actual topic:

The very brief discussion with Philip was on 2017-07-01-QuakeNet-#0ad-dev.log and since I didn't point this out in this differential revision proposal:
The clamping of the water height is done _after_ GetFilteredGroundLevel, so we don't have a smooth transition if there are significant differences between the waterheight and the territory near the water (I guess cycladic archipelago would be an example).
The patch that originally introduced the code fixed #794 and it was an important requirement to filter the terrain, as the camera height would change too frequently when moving the camera over bumps terrain.
So this can be considered a disadvantage (a bigger one than not using a getter helper function that doesn't change anything).
However, if we account for the water height in the Terrain filtering function, then this function would be the only one that takes that into consideration, while the other GetFooTerrainLevel function don't account for the water; which sounds a lot like it would cause issues in the future (even if that filter function is only used here for now).
But if we consider the fact that water is always at one level (unless we use multi water planes in which case we have to rewrite a lot of logic, not only using helper functions) and that having a huge bump near the water would still cause a huge bump in a filtered version, the issue shouldn't be really relevant.

This revision was automatically updated to reflect the committed changes.