Fix CinemaManager code that broke some hotkeys
AbandonedPublic
Actions

Authored by elexis on Jun 1 2017, 6:43 AM.

Details

Reviewers

Sandarac
vladislavbelov

Summary

I'm not sure if this is the best way to fix this, but the cinema manager code in rP19375 (or maybe earlier, I'm not sure) broke the hotkeys that toggle unit silhouettes (default "Alt+S"), and the visibility of session GUI (default "Alt+G") (mentioned by @Imarok in D548). This is because CinemaManager.Update is called constantly in Update in GameView.cpp, even though currently the CinemaManager is not used often, and in any case, it seems it only needs to be updated when it is actually playing (in Atlas it's handled differently). Whenever Update is called, the CinemaManager calls two functions that set the visibility of session GUI and sets the config option for unit silhouettes, which prevents the hotkeys from working.
(Notice that before rP19375 CinemaManager.Update would only be called if it was actually enabled, so this diff does that.)

Another option might be to do the check (if the CinemaManager is actually enabled) directly in CCinemaManager::Update.

Test Plan

Test that the two hotkeys work again, and that the "Cinema Demo" scenario map runs as expected.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 2076
Build 3750: Vulcan Build (Windows)	Jenkins
Build 3749: Vulcan Build	Jenkins
Build 3365: Vulcan Build	Jenkins
Build 3355: Vulcan Build	Jenkins

Event Timeline

Sandarac created this revision.Jun 1 2017, 6:43 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/1427/ for more details.

Harbormaster completed remote builds in B2076: Diff 2345.Jun 1 2017, 7:30 AM

Update should be called everytime. I'll update it later. The current patch breaks cinematics.

SVN is broken, because there's no saving of current state (as was in my patch), just updating every frame.

So probably solutions are: save toggled state by hotkets, 2) get current state of visibility in cinematics.

vladislavbelov planned changes to this revision.Jun 1 2017, 12:01 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/1430/ for more details.

Harbormaster completed remote builds in B2076: Diff 2345.Jun 1 2017, 1:06 PM

Sandarac mentioned this in rP19375: Move most cinematic path simulation data and control from the graphics class to….Jun 1 2017, 11:20 PM

In D584#24080, @vladislavbelov wrote:

Update should be called everytime. I'll update it later. The current patch breaks cinematics.

SVN is broken, because there's no saving of current state (as was in my patch), just updating every frame.

So probably solutions are: save toggled state by hotkets, 2) get current state of visibility in cinematics.

IMO the hotkeys should toggle a new state somewhere instead of doing cinematic updates less often and then breaking the hotkey functionality less often.
The cinematics code should be agnostic of hotkeys and not save that state.
The silhouettes hotkey could actually trigger the config value.
The session visibility should be true upon each gamestart, so it using the config system would be wrong.
Having a global somewhere for toggling the 2D panel visibility might also allow us to hide all menus altogether and remove the hack.

As mentioned by Vladislav, the patch is broken because not calling the cinema update function after the path ended (!enabled) means not showing the GUI anymore.
Reproduce by applying the patch and starting the cinema demo map and noticing we don't have a GUI anymore after the path ended.

Since I forgot that there was an attempt to fix it before, I've created D631 which should also fix the underlying code design flaw.

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/1544/ 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/184/ for more details.

Harbormaster completed remote builds in B2076: Diff 2345.Jun 12 2017, 8:33 PM