Page MenuHomeWildfire Games

Move all data from to cinema manager to the cinema component / fix OOS with non-visual replay

Authored by vladislavbelov on Mar 28 2017, 5:25 PM.


  • All data (CinemaPaths, settings) are moved to the CCmpCinemaManager
  • Some methods are const now
  • All operations with path are controlled by the component now.
Test Plan
  • Use a Cinema Demo map to test, that cut scene is rendering the same.
  • Open replay of the map or another game with cinema paths in non-visual mode.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Mar 28 2017, 5:25 PM
vladislavbelov set the repository for this revision to rP 0 A.D. Public Repository.
elexis edited reviewers, added: leper; removed: Yves.Mar 29 2017, 12:12 PM
elexis edited edge metadata.

(Yves said he is currently out of order)
Fix for rP17594

leper edited reviewers, added: Itms; removed: leper.Mar 29 2017, 1:12 PM
leper edited edge metadata.

Currently considering whether the nuclear option for putting out oil well fires isn't more likely to get the point across. (Also my motivation to review this patch isn't huge given that I've had to complain about this for what feels like years.)

elexis requested changes to this revision.Mar 30 2017, 12:26 PM
elexis removed a reviewer: Itms.
  • Unrelated: When someone rejoins while a path is played, there is no OOS, but the path should be played for the rejoined client too, at the same state like for the other clients
  • Unrelated: It doesn't consider gamespeed. If it is 20x, the sim time advances by 20x while the path is played at regular speed and aborts after 2 seconds.
  • Unrelated: The cinematic path doesn't play if the window doesn't have focus. This is good for singleplayer but probably bad for multiplayer.

(A combination of one of these points means that in MP, the paths aren't played exactly at the same time, which should likely be the case.)

66 ↗(On Diff #982)

We had the same issue in rP18613, not being able to open more GUI pages sounds like an ugly workaround limiting our capabilities.
The proposed fix (last 2 lines of the comment) should be removed and discussed in a ticket or diff proposal.

72 ↗(On Diff #982)

Replace these two comments with "// Hide session GUI while playing the path" ?

78 ↗(On Diff #982)


200 ↗(On Diff #982)

missing negation

57 ↗(On Diff #982)

that comment doesn't really state something that isn't self-evident from the function name, same above

59 ↗(On Diff #982)

compiler complaining about pCinema being unused, so lets make that UNUSED until the code actually does refer to it

75 ↗(On Diff #982)

use the actual type instead of auto

295 ↗(On Diff #982)

This entirely crashes everything in non-visual replay mode (at least here on linux).
Just needs an if (CRenderer::IsInitialised()).
It didn't crash the previous code as that was only executed if we had a renderer.

Also the comment seems unneeded.

Also this call is (still) bugged because it will enable silouettes after finishing the path, even if the user option is to disable silhouettes. Hoping to get a fix for that afterwards.

135 ↗(On Diff #982)

Don't understand this TODO

164 ↗(On Diff #982)

manager unused now, delete

This revision now requires changes to proceed.Mar 30 2017, 12:26 PM
vladislavbelov edited edge metadata.
vladislavbelov marked 10 inline comments as done.

Fixed elexis's notes.

elexis requested changes to this revision.Apr 5 2017, 5:39 AM

Things wrong in this patch:

  • The code moves the disabling of unit silhouettes in the renderer to the simulation component, but should remain in the graphics class.
  • Three m_Paused:

The graphics m_Paused is just a striaght copy from the g_Game one and we can just check for the latter.
The simulation m_Paused is unused / never set to true and shouldn't be used in the future either. Explicit pausing will most likely never be part of the simulation (as players should be able add a pause even if other players paused already) and the sim must treat implicit pauses (lag) equally well.
Having three different variables for that is bad as it means they could have different states if not managed correctly and it means that the reader has to figure out the differences and prove the correctness too.

  • m_Enabled graphics copy

m_Enabled in the simulation is true if the path is currently playing or paused.
m_Enabled in the graphics class is solely used as a cache to prevent calling sn->SetSetting("hidden", L"true"); on each call.
However we are much better off with checking for GUI<bool>::GetSetting(sn, "hidden", hidden); instead of adding new member variable that is a copy of the simulation variable.

  • More readability if logically independent things would be moved to helper functions like UpdateSessionVisibility, UpdateSilhouettesVisibility and DrawPaths instead of combining it in one function.

The reader can first read the structure and then only read the code if he really wants to. Also allows early returns instead of big if-scopes.

Things wrong in rP17594 which this patch could have addressed:

  • Hotkey TODO:

    Not changed by the patch but still in the hunks: // TODO: implement skip with some empty code which also seems wrong. That TODO shouldn't have been in the code in the first place. trac is the place for a wish list. The empty code shell shouldn't have been added. Also the code shell is probably wrong because it must be a simulation command, not a GUI check, so it probably must go to misc.xml. TODO features must be designed in the proper place with dev feedback before implemented partially.
  • Searching for the uses of the removed member variables above revealed CameraCtrlHandlers.cpp should check for IsEnabled, not IsPlaying because we shouldn't be able to move the camera while a cinematic path is paused too
  • GetPathsDrawing and SetPathsDrawing are unused and should not have been introduced, they just confuse the reader. Introduce things only when they are needed or when a de is guaranteed to commit the user of that code right afterwards. Not removing it now because you said you will use it soon.

*Things which will remain wrong after this diff*

  • OOS on rejoin while a path is played, because the path won't be started for a rejoining client if it had started before (should be implemented) and thus the reveal map flag will not be reset
  • Cinematic camera path animation is not synchronized but only depends on renderer, thus ignores gamespeed, network and renderer lag. We must guarantee that all players finished the path simultaneously and that if a map/path author wants to show a given entity at a given time, that the path actually follows that determination.
58 ↗(On Diff #1008)

Pointless: Checks whether g_Game exists after having it used already

175 ↗(On Diff #1008)

Should add a comment why we return handled here

30 ↗(On Diff #1008)

Pointless comment

52 ↗(On Diff #1008)

Those three lines should be one until extended IMO

69 ↗(On Diff #1008)

What information do these thee dots provide that an empty function doesn't imply?

336 ↗(On Diff #1008)

Each member on an own line

72 ↗(On Diff #1008)


This revision now requires changes to proceed.Apr 5 2017, 5:39 AM
elexis accepted this revision.Apr 5 2017, 5:48 AM

Luckily there is such a thing called IRC in which we can discuss things before posting a final review, making it actually faster to review.
I've changed the things in the first two parts in a github branch and split it to 13 commits.
The diffs were reviewed by Vladislav, 8 accepted as is, 2 requested changes and 2 denied.
Here the resulting 12 commits

which will be committed in two svn commits.

So there is only one more thing to say.
Thanks, Finally ! Keep up the good work on this.
Once we have fixed the remaining serialization issue and the atlas controls were implemented, this will become a very popular feature and used in a number of scenario/campaign maps!

This revision is now accepted and ready to land.Apr 5 2017, 5:48 AM
This revision was automatically updated to reflect the committed changes.