Page MenuHomeWildfire Games

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

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

Details

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 8580
Build 14047: Vulcan BuildJenkins
Build 14046: arc lint + arc unit

Event Timeline

wraitii created this revision.Jun 30 2019, 6:58 PM
Owners added a subscriber: Restricted Owners Package.Jun 30 2019, 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.Jun 30 2019, 7:39 PM
source/main.cpp
425

why the scope ?

Angen added a comment.Jun 30 2019, 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
425

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.Jul 4 2019, 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.Jul 4 2019, 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.Jul 4 2019, 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.Jul 20 2019, 2:53 PM
Angen accepted this revision.EditedJul 21 2019, 1:14 PM

I cannot agree on that we swapped buffers when we did not render
As you can see here https://code.wildfiregames.com/D1495?vs=on&id=7570#toc
If we were minimised we would not swap buffers, but other than that I have no strong opinion on this part

This revision is now accepted and ready to land.Jul 21 2019, 1:14 PM
wraitii updated this revision to Diff 9102.Jul 24 2019, 5:45 PM

@Angen is correct that we only swapped buffers if we renderer - do that again.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/223/display/redirect

Angen requested changes to this revision.Jul 24 2019, 6:05 PM
Angen added inline comments.
source/ps/GameSetup/GameSetup.cpp
203

!g_VideoMode.IsInFullscreen()

This revision now requires changes to proceed.Jul 24 2019, 6:05 PM
Angen added inline comments.Jul 24 2019, 6:05 PM
source/main.cpp
414–423

inline this

wraitii planned changes to this revision.Jul 24 2019, 6:09 PM
wraitii added inline comments.
source/main.cpp
414–423

oh yeah, why didn't i do that :p

source/ps/GameSetup/GameSetup.cpp
203

ah, you're right, I misunderstood this condition.

wraitii updated this revision to Diff 9104.Jul 24 2019, 6:17 PM

what angen said

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/224/display/redirect

Angen accepted this revision.Jul 24 2019, 6:30 PM

Build on windows is green, no crashes related to glsl or postprocessing, sound plays in atlas as should.

This revision is now accepted and ready to land.Jul 24 2019, 6:30 PM
Angen added inline comments.Jul 24 2019, 6:35 PM
source/ps/Game.cpp
1

2019

source/ps/GameSetup/GameSetup.h
1

2019

Yeah Jenkins is failing at compiling test_root.cpp somehow :/

Tests are passing for me too, I'll commit this, thanks for reviewing.

This revision was landed with ongoing or failed builds.Jul 24 2019, 6:40 PM
This revision was automatically updated to reflect the committed changes.