Besides the pathfinder OOS if the water level was changed by the simulation, it was reported by rejoining players that the graphical water height is not correct after deserialization.
This is because the Deserialize function of CCmpWaterManager.cpp is currently empty besides restoring the simulation setting.
This is correct for all maps that never change the water height with a simulation script (in which case we don't need to recompute the water vertices).
Details
- Reviewers
- None
- Group Reviewers
Contributors Restricted Project - Commits
- rP19862: Recompute water graphics when changing the water level and upon deserialization…
- Trac Tickets
- #4596
Apply this patch that:
- sets the water rise time to 0 seconds in extinct_volcano_triggers.js and a big height increase to make it easily noticeable whther the waterheight is correct or not
- shows whether it recomputes the graphical data upon deserialization
Then
- Start hosting a game on extinct volcano. Notice the water level rose immediately. Pause the game.
- Join as a late observer in a second window and check the waterrendering for height and completeness.
Notice that without the fix applied, only some areas of the map were updated coincidentally with the correct height:
Notice that the result meets the expectation if the patch has been applied.
Read the debug warnings and notice that the recomputation is called if and only if the simulation has changed the waterheight.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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/1554/ 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/195/ for more details.
Minimap Bug:
As seen on the screenshot, the minimap isn't updated.
That's because g_Renderer.GetWaterManager()->m_WaterHeight isn't updated from
Waterheight Bug: (Fixed)
Of the RecomputeWaterData call, only the GetSimContext().GetTerrain().MakeDirty(RENDERDATA_UPDATE_VERTICES); call updates the graphical water height, i.e. the CPatch types which use cmpWaterManager->GetExactWaterLevel(), so they're not affected by the renderer waterheight being outdated (the CTexturedLineRData::Update function in TexturedLineRData.cpp is the one noticeable ingame).
Effect Bug:
The other calls in RecomputeWaterData are RecomputeBlurredNormalMap, RecomputeDistanceHeightmap, RecomputeWindStrength, CreateWaveMeshes and still compute the data for the outdated waterheight.
But since it's outdated for both clients, both compute the same blurred normal map, distance heightmap, wind strength and wave meshes.
Camera Bug:
The camera height only checks for the terrain and completely ignores the waterheight. Hence it becomes more likely to have the camera below the waterheight with rising water.
It's also relevant to fix for maps that don't change the waterheight later, because if the difference between the terrain and water level is great enough, then the camera will drop off unexpectedly when moving it over a river for example.
Fix the other three bugs reported above too.
Remove duplication.
Those early returns are needed to prevent SEGFAULTS when calling the graphical updates in SetWaterLevel from the MapLoader before the terrain was initialized.
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/1556/ 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/197/ for more details.
The test for if (m_WaterHeight.ToFloat() != g_Renderer.GetWaterManager()->m_WaterHeight) when deserializing in non-visual mode (rejointest) is broken.
(Remove the check that doesn't work if there is no renderer, hence) always CCmpWaterManager::RecomputeWaterData upon CCmpWaterManager::Deserialize (even if the waterheight didn't change).
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/1664/ 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/265/ for more details.
source/renderer/WaterManager.cpp | ||
---|---|---|
949 ↗ | (On Diff #2757) | This seems broken, at least considering that GetTerrain can be a nullptr, which RecomputeDistanceHeightmap handles. However theis function is called before that in RecomputeWaterData, so if GetTerrain returns nullptr we will crash here. You might want to move the actual check closer to the assignment of terrain (or vice versa). |
source/renderer/WaterManager.cpp | ||
---|---|---|
949 ↗ | (On Diff #2757) | Thus ending up with the exact same early return in all three of these functions. |
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/268/ for more details.
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/1668/ for more details.
Tested with atlas and it's slow to change the waterheight often by moving the slider slowly. But it's not noticeably slower than before.
The "Reset Water Data" button in atlas introduced by rP14514 is (at least now) obsolete and should be removed (the commit also contains a comment saying the button is "not extremely nice").