Details
- Reviewers
- None
- Commits
- rP19896: Ensure the camera position being above the water level (without smoothing the…
- Trac Tickets
- #3892
Change waterRiseStartTime, waterIncreaseTime and waterLevelIncreaseHeight or build a custom map with huge water height in atlas.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2486 Build 4190: Vulcan Build (Windows) Jenkins Build 4189: Vulcan Build Jenkins Build 4188: arc lint + arc unit
Event Timeline
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.
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.
I'd rather you used "GetWaterHeight" or however it's called, would make multiple-water-plane transition easier if/when we get there.
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)
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?
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.
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.