Page MenuHomeWildfire Games
Differential D5001 Diff 22557 source/graphics/MapGenerator.h

Changeset View

Standalone View

View Options

source/graphics/MapGenerator.h

Show All 12 Lines
* *
* You should have received a copy of the GNU General Public License * You should have received a copy of the GNU General Public License
* along with 0!A.D. If not, see <http://www.gnu.org/licenses/>. * along with 0!A.D. If not, see <http://www.gnu.org/licenses/>.
*/ */
#ifndef INCLUDED_MAPGENERATOR #ifndef INCLUDED_MAPGENERATOR
#define INCLUDED_MAPGENERATOR #define INCLUDED_MAPGENERATOR
#include "ps/FileIo.h" #include "lib/file/vfs/vfs_path.h"
#include "ps/Future.h"
#include "ps/TemplateLoader.h"
#include "scriptinterface/StructuredClone.h" #include "scriptinterface/StructuredClone.h"
#include <atomic> #include <atomic>
#include <boost/random/linear_congruential.hpp>
#include <mutex>
#include <set>
#include <string> #include <string>
class CMapGeneratorWorker;
/**
* Random map generator interface. Initialized by CMapReader and then checked
* periodically during loading, until it's finished (progress value is 0).
*
* The actual work is performed by CMapGeneratorWorker in a separate thread.
*/
class CMapGenerator
{
NONCOPYABLE(CMapGenerator);
public:
CMapGenerator();
~CMapGenerator();
/**
* Start the map generator thread
*
* @param scriptFile The VFS path for the script, e.g. "maps/random/latium.js"
* @param settings JSON string containing settings for the map generator
*/
void GenerateMap(const VfsPath& scriptFile, const std::string& settings);
/**
* Get status of the map generator thread
*
* @return Progress percentage 1-100 if active, 0 when finished, or -1 on error
*/
int GetProgress() const;
/** /**
wraitiiUnsubmitted
Done
Inline Actions

I'd call it TestGenerateMap

wraitii: I'd call it TestGenerateMap
phositAuthorUnsubmitted
Done
Inline Actions

It's also (indirectly) called by non test code.

phosit: It's also (indirectly) called by non test code.
wraitiiUnsubmitted
Done
Inline Actions

Right, wondered that.
Then perhaps just change the comment to 'Intended for tests & inner usage'

wraitii: Right, wondered that. Then perhaps just change the comment to 'Intended for tests & inner usage'
vladislavbelovUnsubmitted
Done
Inline Actions

Private methods/fields should be after public ones.

vladislavbelov: Private methods/fields should be after public ones.
vladislavbelovUnsubmitted
Done
Inline Actions

That should be in the private section.

vladislavbelov: That should be in the `private` section.
phositAuthorUnsubmitted
Done
Inline Actions

IDK

phosit: IDK
vladislavbelovUnsubmitted
Done
Inline Actions

It's an implementation detail

vladislavbelov: It's an implementation detail
phositAuthorUnsubmitted
Done
Inline Actions

It's already in the private section.
Most (when not all) classes who have a friend declaration have it at the top.

phosit: It's already in the private section. Most (when not all) classes who have a `friend`…
vladislavbelovUnsubmitted
Done
Inline Actions

The private friend should be after the public since it's an implementation/particular relationship detail. Definitely not all, I'd say about 50/50 or 40/60.

vladislavbelov: The private friend should be after the public since it's an implementation/particular…
vladislavbelovUnsubmitted
Done
Inline Actions

CMapGenerator.

vladislavbelov: `CMapGenerator`.
* Get random map data, according to this format: * Generate the map. This does take a long time.
* http://trac.wildfiregames.com/wiki/Random_Map_Generator_Internals#Dataformat
* *
* @return StructuredClone containing map data * @param progress Destination to write the function progress to. You must not
*/ * write to it while `RunMapGenerationScript` is running.
Script::StructuredClone GetResults(); * @param script The VFS path for the script, e.g. "maps/random/latium.js".
* @param settings JSON string containing settings for the map generator.
private: * @param flags With thous flags the engine functions get registered
CMapGeneratorWorker* m_Worker; * `g_MapSettings` also respects this flags.
* @return If there is an error `nullptr` is returned. Otherwise random map
}; * data, according to this format:
* https://trac.wildfiregames.com/wiki/Random_Map_Generator_Internals#Dataformat
/** */
* Random map generator worker thread. Script::StructuredClone RunMapGenerationScript(std::atomic<int>& progress,
wraitiiUnsubmitted
Done
Inline Actions

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.

wraitii: Feels like it'd be better to abstract the core functionality in a common path, and make the…
phositAuthorUnsubmitted
Done
Inline Actions

I'll take a look at that.

phosit: I'll take a look at that.
wraitiiUnsubmitted
Done
Inline Actions

This takes a long time maybe

wraitii: `This takes a long time` maybe
phositAuthorUnsubmitted
Done
Inline Actions

I'll do that

phosit: I'll do that
vladislavbelovUnsubmitted
Done
Inline Actions

The style doesn't seem like a constant, I'd use ERROR_CONSTANT or at least ErrorConstant. Also Constant suffix seems redundant, we don't use such suffixes.

vladislavbelov: The style doesn't seem like a constant, I'd use `ERROR_CONSTANT` or at least `ErrorConstant`.
phositAuthorUnsubmitted
Done
Inline Actions

You would use ERROR_CONSTANT but without the "constant" suffix. So only ERROR?

phosit: You would use `ERROR_CONSTANT` but without the "constant" suffix. So only `ERROR`?
vladislavbelovUnsubmitted
Done
Inline Actions

ERROR_CONSTANT was an example of the style. I'd use INVALID_PROGRESS here.

vladislavbelov: `ERROR_CONSTANT` was an example of the style. I'd use `INVALID_PROGRESS` here.
vladislavbelovUnsubmitted
Done
Inline Actions

A subject is missing.

vladislavbelov: A subject is missing.
phositAuthorUnsubmitted
Done
Inline Actions

Yes. Can you propose a better formulation?

phosit: Yes. Can you propose a better formulation?
vladislavbelovUnsubmitted
Done
Inline Actions

I'd use @param progress Destination to write the function progress to.. or @param progress The function writes its progress to the parameter..

vladislavbelov: I'd use `@param progress Destination to write the function progress to.`. or `@param progress…
* (This is run in a thread so that the GUI remains responsive while loading) ScriptInterface& scriptInterface, const VfsPath& script, const std::string& settings,
* const u16 flags = JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
phositAuthorUnsubmitted
Done
Inline Actions

I don't like the name of this function RunMapGenerationScript is more expressive.

phosit: I don't like the name of this function `RunMapGenerationScript` is more expressive.
* Thread-safety:
* - Initialize and constructor/destructor must be called from the main thread.
* - ScriptInterface created and destroyed by thread
* - StructuredClone used to return JS map data - JS:Values can't be used across threads/contexts.
*/
class CMapGeneratorWorker
{
public:
CMapGeneratorWorker(ScriptInterface* scriptInterface);
~CMapGeneratorWorker();
/**
* Start the map generator thread
*
* @param scriptFile The VFS path for the script, e.g. "maps/random/latium.js"
* @param settings JSON string containing settings for the map generator
*/
void Initialize(const VfsPath& scriptFile, const std::string& settings);
/**
* Get status of the map generator thread
*
* @return Progress percentage 1-100 if active, 0 when finished, or -1 on error
*/
int GetProgress() const;
/**
* Get random map data, according to this format:
* http://trac.wildfiregames.com/wiki/Random_Map_Generator_Internals#Dataformat
*
* @return StructuredClone containing map data
*/
Script::StructuredClone GetResults();
/**
* Set initial seed, callback data.
* Expose functions, globals and classes defined in this class relevant to the map and test scripts.
*/
void InitScriptInterface(const u32 seed);
private:
/**
* Expose functions defined in this class that are relevant to mapscripts but not the tests.
*/
void RegisterScriptFunctions_MapGenerator();
/**
* Load all scripts of the given library
*
* @param libraryName VfsPath specifying name of the library (subfolder of ../maps/random/)
* @return true if all scripts ran successfully, false if there's an error
*/
bool LoadScripts(const VfsPath& libraryName);
/**
* Finalize map generation and pass results from the script to the engine.
*/
void ExportMap(JS::HandleValue data);
/**
* Load an image file and return it as a height array.
*/
JS::Value LoadHeightmap(const VfsPath& src);
/**
* Load an Atlas terrain file (PMP) returning textures and heightmap.
*/
JS::Value LoadMapTerrain(const VfsPath& filename);
/**
* Sets the map generation progress, which is one of multiple stages determining the loading screen progress.
*/
void SetProgress(int progress);
/**
* Microseconds since the epoch.
*/
double GetMicroseconds();
/**
* Return the template data of the given template name.
*/
CParamNode GetTemplate(const std::string& templateName);
/**
* Check whether the given template exists.
*/
bool TemplateExists(const std::string& templateName);
/**
* Returns all template names of simulation entity templates.
*/
std::vector<std::string> FindTemplates(const std::string& path, bool includeSubdirectories);
/**
* Returns all template names of actors.
*/
std::vector<std::string> FindActorTemplates(const std::string& path, bool includeSubdirectories);
/**
* Perform the map generation.
*/
bool Run();
/**
* Currently loaded script librarynames.
*/
std::set<VfsPath> m_LoadedLibraries;
/**
* Result of the mapscript generation including terrain, entities and environment settings.
*/
Script::StructuredClone m_MapData;
/**
* Deterministic random number generator.
*/
boost::rand48 m_MapGenRNG;
/**
* Current map generation progress.
* Initialize to `-1`. If something happens before we start, that's a
* failure.
*/
std::atomic<int> m_Progress{-1};
/**
* Provides the script context.
*/
ScriptInterface* m_ScriptInterface;
/**
* Map generation script to run.
*/
VfsPath m_ScriptPath;
/**
* Map and simulation settings chosen in the gamesetup stage.
*/
std::string m_Settings;
/**
* Backend to loading template data.
*/
CTemplateLoader m_TemplateLoader;
/**
* Holds the completion result of the asynchronous map generation.
* TODO: this whole class could really be a future on its own.
*/
Future<void> m_WorkerThread;
/**
* Avoids thread synchronization issues.
*/
std::mutex m_WorkerMutex;
};
#endif //INCLUDED_MAPGENERATOR #endif //INCLUDED_MAPGENERATOR
Wildfire Games · Phabricator