Fix rmgen quit / rP21264 on OS X
ClosedPublic
Actions

Authored by elexis on May 5 2018, 9:19 PM.

Details

Reviewers

vladislavbelov
wraitii

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP21818: Fix OSX not starting matches following rP21264.
rP21264: Stop the random map generation upon quit request (Alt+F4), refs #4822.

Summary

As said on rP21264, OSX doesn't allow polling events from secondary threads. Thus when the interrupt gets triggered, things go bad.
The same issue was fixed in Atlas in rP19160.

From https://wiki.libsdl.org/SDL_PumpEvents:

/!\ WARNING: This should only be run in the thread that initialized the video subsystem, and for extra safety, you should consider only doing those things on the main thread in any case.

Test Plan

On Windows, linux and OSX:

Start a random map and make sure it actually starts (Currently doesn't on OSX).
Start a random map, hit Alt+F4. Notice in the terminal that the rmgen stage was active and that the process ended before rmgen finished.
Generate a random map in atlas.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.May 5 2018, 9:19 PM

Owners added a subscriber: Restricted Owners Package.May 5 2018, 9:19 PM

wraitii added a commit: rP21264: Stop the random map generation upon quit request (Alt+F4), refs #4822..May 5 2018, 9:19 PM

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

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

Harbormaster completed remote builds in B6026: Diff 6512.May 5 2018, 9:23 PM

After tests, I have a variant, how to solve it, P120 - simple way, use a static variable in the same translation unit.

Also, should we stop all workers for different map generators, only for the current one?

source/graphics/MapGenerator.cpp
61 ↗	(On Diff #6512)	`CMapGeneratorWorker::` is not needed.
source/graphics/MapGenerator.h
124 ↗	(On Diff #6512)	We should avoid internal members to be public.

vladislavbelov requested changes to this revision.May 6 2018, 12:15 AM

This revision now requires changes to proceed.May 6 2018, 12:15 AM

I have to stack a change request, this patch and Vladislavs variant both do not work.

The problem that this commit addressed is that the JS code is executed without interruption, so the MapReader.cpp hunk inserted here is executed only after the JS mapgen finished already.
But it should be exeuted meanwhile and that's why we have added the SpiderMonkey interrupt function which is called periodically (see D1304).

So MapGeneratorInterruptCallback would have to call into the mainthread to update the SDL_QuitRequested state.

I'm ok with reverting the concerned commit if none of you knows a blatant solution in the next hours.

In D1484#60697, @elexis wrote:

I have to stack a change request, this patch and Vladislavs variant both do not work.

The problem that this commit addressed is that the JS code is executed without interruption, so the MapReader.cpp hunk inserted here is executed only after the JS mapgen finished already.
But it should be exeuted meanwhile and that's why we have added the SpiderMonkey interrupt function which is called periodically (see D1304).

So MapGeneratorInterruptCallback would have to call into the mainthread to update the SDL_QuitRequested state.

I'm ok with reverting the concerned commit if none of you knows a blatant solution in the next hours.

Mh, I don't understand, MapReader is in a separate thread from the worker one though?
If it doesn't work anyways, I think we should either revert or if_def that to "not OS X". As I've said, force-quitting on OSX does force quit without waiting on RMGen. Default-quitting indeed may not work that well though.

In D1484#60701, @wraitii wrote:

Mh, I don't understand, MapReader is in a separate thread from the worker one though?

CMapReader::GenerateMap is not executed during JS rmgen code. GetProgress is, but there still seem to be nicer alternatives, one of which is proposed in the updated revision.

What I find worrying is that not a single person tried running the game on OSX since february.

Provide an actually working patch, at least for linux.
Could there be any race condition?

elexis retitled this revision from Fix rP21264 on OS X to Fix rmgen quit / rP21264 on OS X.May 6 2018, 3:32 PM

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

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

Harbormaster completed remote builds in B6030: Diff 6516.May 6 2018, 3:37 PM

Fix atlas doing its own thing

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

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

Harbormaster completed remote builds in B6031: Diff 6517.May 7 2018, 1:42 AM

elexis edited the summary of this revision. (Show Details)May 7 2018, 1:46 PM

elexis edited the test plan for this revision. (Show Details)