Put the CMapGeneratorWorker completely inside the task
Needs ReviewPublic
Actions

Authored by phosit on Sat, May 20, 7:59 PM.

Details

Reviewers

wraitii

Summary

It was weirdly split which parts the task does edit and which parts the main thread does.

CMapGeneratorWorker is now unaware of threads and synchronisation (Except for the progress). There is no explicit std::mutex anymore.
The only purpose of CMapGeneratorCallbackData (the old CMapGeneratorWorker) is to provide the callbacks to the script (hence the renaming).

The internal API is easyer: The MapGenerator does only have to construct the MapGeneratorCallbackData and call Execute on it.
The external API is easyer: MapGenerator::GetResult could have ben called when the generation wasn't finished (leading to UB). Now It does wait for the result.

Likely performance is improved but that's a secondary-effect.

Test Plan

Generate some maps.

Diff Detail

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

phosit requested review of this revision.Sat, May 20, 7:59 PM

phosit created this revision.

phosit added a parent revision: D3676: Removes explicit synchronizations from MapGenerator.

phosit retitled this revision from But the CMapGeneratorWorker complitly inside the task to Put the CMapGeneratorWorker completely inside the task.

I think you should merge the two classes at this point, and / or go Pimpl .

I think the function for tests can be improved to remove this ugly 'don't use this' and also the comments need to be improved either way.

source/graphics/MapGenerator.h
47–48	Feels like it'd be better to abstract the core functionality in a common path, and make the Test class a friend, so that we don't have any specific code for this.