Page MenuHomeWildfire Games

Put the CMapGeneratorWorker completely inside the task
Needs ReviewPublic

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



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 Skipped
Unit Tests Skipped

Event Timeline

phosit requested review of this revision.Sat, May 20, 7:59 PM
phosit created this revision.
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.


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.

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

No, I'm doing quite the opposit. It was Pimpl before (although the implementation also in the header).
The two classes have distinct purposes.


I'll take a look at that.

phosit updated this revision to Diff 21818.Sun, May 21, 7:42 PM

Moved the MapGeneratorCallbackData to the cpp file.
Moved the MapGenerator to the MapReader.cpp and named it GeneratorState.