Page MenuHomeWildfire Games

Alt+tab fix for rP22314 - call IdleTask in Atlas and swap buffers only once
Needs ReviewPublic

Authored by wraitii on Sun, Jun 30, 6:58 PM.

Details

Reviewers
Stan
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

As reported by Angen:

  • rP22314 moved SwapBuffers inside Render - this makes Atlas call it twice in a row which is possibly broken and certainly wasteful.
  • rP22314 moved IdleTask from the sound manager outside of Render. I did not realise this would mean Atlas apparently never called it any more. Trough some weird fluke, it was called also in Game->Update but only in the specific path Atlas doesn't take.

Calling IdleTask every frame in Atlas fixes the sound issue stan reported, I think what happens there is that the buffers, being never cleaned, got overrun and new sounds couldn't start.

I'm not sure about the reports that we must only call IdleTask iff Render is called - needs some investigation.
I'm also not sure if we should call IdleTask when minimised, but I don't see why not as it would appear to be necessary for playlist and music doesn't stop when the application is minimised

Test Plan

Check sound in Atlas, review changes for code flow, debate the doubts in the above Summary

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
temp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8127
Build 13232: Vulcan BuildJenkins
Build 13231: arc lint + arc unit

Event Timeline

wraitii created this revision.Sun, Jun 30, 6:58 PM
Owners added a subscriber: Restricted Owners Package.Sun, Jun 30, 6:58 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Game.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/tools/atlas/GameInterface/View.cpp
| 211| »   »   //·not·in·every·call·to·g_Game->Update
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1855/display/redirect

Stan added inline comments.Sun, Jun 30, 7:39 PM
source/main.cpp
416

why the scope ?

Angen added a comment.Sun, Jun 30, 8:02 PM

Issue with atlas is resolved.
Builds release and debug.
Tests pass release and debug.

Sound now keeps playing even game is minimised or alt+tab-ed but maybe not so bad.

The one thing I am not sure is now we swap buffers even we do not render, however it did not break game ( for me on windows with nvidia, intel) , I do not think it is good.

needs some investigation.

Have you some results? Did you reproduce Stan's issue?

source/main.cpp
416

Because of PROFILE3("swap buffers"), it measure the current block.

source/ps/Game.cpp
317

Do we need to add swap here?

Also we had a sound update here, now it's missed. Probably it should present?

Angen added a comment.Thu, Jul 4, 6:57 AM

The point call idleTask iff render was mainly about there is no point call it when one does not see whats happening (minimised, tabbed) and to keep previous behaviour mostly.

source/ps/Game.cpp
317

Swap was not present here originally so no.
Game has not really start yet, so I think sound is not needed, but maybe there is now small silent window (I did notice)

wraitii added inline comments.Thu, Jul 4, 9:02 AM
source/ps/Game.cpp
317

I think in an ideal world this would not render at all, rather just begin loading assets, so swapping or adding IdleTask sounds not wanted.

Angen added inline comments.Thu, Jul 4, 9:21 AM
source/ps/Game.cpp
317

Edit: (did not notice)

Possibly related to #5471

In D2029#84525, @Angen wrote:

The one thing I am not sure is now we swap buffers even we do not render, however it did not break game ( for me on windows with nvidia, intel) , I do not think it is good.

This was actually the case before, so it seems to be fine.

elexis retitled this revision from Fix for rP22314 - call IdleTask in Atlas and swap buffers only once to Alt+tab fix for rP22314 - call IdleTask in Atlas and swap buffers only once.Sat, Jul 20, 2:53 PM