Paths

Table of Contentst

Differential D2029

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

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

Details

Reviewers

Stan
Silier

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP22544: Alt+tab fix for rP22314 - call sound IdleTask in Atlas and swap buffers only…
rP22314: Fix a crash on some system when Alt-tabbing during game setup.

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 8578
Build 14044: Vulcan Build	Jenkins
Build 14043: 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

wraitii added a commit: rP22314: Fix a crash on some system when Alt-tabbing during game setup..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

Harbormaster completed remote builds in B8127: Diff 8659.Jun 30 2019, 7:19 PM

Stan added inline comments.Jun 30 2019, 7:39 PM

source/main.cpp
426	why the scope ?

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
426	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?

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.

Silier 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

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

@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

Harbormaster failed remote builds in B8578: Diff 9102!Jul 24 2019, 5:47 PM

Silier requested changes to this revision.Jul 24 2019, 6:05 PM

Silier added inline comments.