Page MenuHomeWildfire Games

Update graphical water level after deserialization
ClosedPublic

Authored by elexis on Jun 13 2017, 9:53 PM.

Details

Reviewers
None
Group Reviewers
Contributors
Restricted Project
Commits
rP19862: Recompute water graphics when changing the water level and upon deserialization…
Trac Tickets
#4596
Summary

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).

Test Plan

Apply this patch that:

  1. 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
  2. shows whether it recomputes the graphical data upon deserialization

Then

  1. Start hosting a game on extinct volcano. Notice the water level rose immediately. Pause the game.
  2. 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.

Event Timeline

elexis created this revision.Jun 13 2017, 9:53 PM
elexis edited the test plan for this revision. (Show Details)Jun 13 2017, 9:55 PM
elexis edited the test plan for this revision. (Show Details)Jun 13 2017, 9:57 PM
Vulcan added a subscriber: Vulcan.Jun 13 2017, 10:41 PM

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.

elexis updated the Trac tickets for this revision.Jun 13 2017, 11:07 PM
elexis planned changes to this revision.Jun 14 2017, 4:28 AM

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.

elexis updated this revision to Diff 2555.Jun 14 2017, 4:30 AM

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.

elexis planned changes to this revision.Jun 14 2017, 4:35 PM

The test for if (m_WaterHeight.ToFloat() != g_Renderer.GetWaterManager()->m_WaterHeight) when deserializing in non-visual mode (rejointest) is broken.

elexis updated this revision to Diff 2757.Jun 29 2017, 6:54 PM

(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).

In D638#25957, @elexis wrote:

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.

This is the issue described in #3892, and so I guess this diff fixes that.

This is the issue described in #3892, and so I guess this diff fixes that.

Ah, thanks

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.

leper added inline comments.
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).

elexis added reviewers: Contributors, Restricted Project.Jun 29 2017, 9:03 PM
elexis added inline comments.
source/renderer/WaterManager.cpp
949 ↗(On Diff #2757)

Thus ending up with the exact same early return in all three of these functions.
CreateWaveMeshes looks like it could use such a check too.
(The 4 duplicate checks still shouldn't be merged in an early return at WaterManager::RecomputeWaterData, because we still need the terrain variable, thus we can just as well check and allow engine modders to only call some of these 4 functions selectively.)

elexis updated this revision to Diff 2763.Jun 29 2017, 9:18 PM

Add !terrain check, so that all four functions have the same early return.

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.

elexis added inline comments.Jul 1 2017, 5:22 AM
source/graphics/GameView.cpp
586 ↗(On Diff #2763)

Splitting this into a separate diff as it's related to rP11556 and independent of the WaterManager failures

elexis added a comment.Jul 1 2017, 6:13 AM

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").

This revision was automatically updated to reflect the committed changes.