Page MenuHomeWildfire Games
Differential D5001 Diff 22028 source/graphics/MapGenerator.cpp

Changeset View

Standalone View

View Options

source/graphics/MapGenerator.cpp

Show All 24 Lines
#include "lib/external_libraries/libsdl.h" #include "lib/external_libraries/libsdl.h"
#include "lib/status.h" #include "lib/status.h"
#include "lib/timer.h" #include "lib/timer.h"
#include "lib/file/vfs/vfs_path.h" #include "lib/file/vfs/vfs_path.h"
#include "maths/MathUtil.h" #include "maths/MathUtil.h"
#include "ps/CLogger.h" #include "ps/CLogger.h"
#include "ps/FileIo.h" #include "ps/FileIo.h"
#include "ps/Profile.h" #include "ps/Profile.h"
#include "ps/TaskManager.h"
#include "ps/scripting/JSInterface_VFS.h" #include "ps/scripting/JSInterface_VFS.h"
#include "ps/TemplateLoader.h"
#include "scriptinterface/FunctionWrapper.h" #include "scriptinterface/FunctionWrapper.h"
#include "scriptinterface/ScriptContext.h" #include "scriptinterface/ScriptContext.h"
#include "scriptinterface/ScriptConversions.h" #include "scriptinterface/ScriptConversions.h"
#include "scriptinterface/ScriptInterface.h" #include "scriptinterface/ScriptInterface.h"
#include "scriptinterface/JSON.h" #include "scriptinterface/JSON.h"
#include "simulation2/helpers/MapEdgeTiles.h" #include "simulation2/helpers/MapEdgeTiles.h"
#include <boost/random/linear_congruential.hpp>
#include <set>
#include <string> #include <string>
#include <vector> #include <vector>
// TODO: Maybe this should be optimized depending on the map size. // TODO: Maybe this should be optimized depending on the map size.
constexpr int RMS_CONTEXT_SIZE = 96 * 1024 * 1024; constexpr int RMS_CONTEXT_SIZE = 96 * 1024 * 1024;
extern bool IsQuitRequested(); extern bool IsQuitRequested();
static bool namespace
MapGeneratorInterruptCallback(JSContext* UNUSED(cx)) {
bool MapGeneratorInterruptCallback(JSContext* UNUSED(cx))
{ {
// This may not use SDL_IsQuitRequested(), because it runs in a thread separate to SDL, see SDL_PumpEvents // This may not use SDL_IsQuitRequested(), because it runs in a thread separate to SDL, see SDL_PumpEvents
if (IsQuitRequested()) if (IsQuitRequested())
{ {
LOGWARNING("Quit requested!"); LOGWARNING("Quit requested!");
return false; return false;
} }
return true; return true;
} }
CMapGeneratorWorker::CMapGeneratorWorker(ScriptInterface* scriptInterface) : /**
m_ScriptInterface(scriptInterface) * Provides callback's for the JavaScript.
{} */
class CMapGeneratorCallbackData
vladislavbelovUnsubmitted
Done
Inline Actions

It doesn't seem like a struct.

vladislavbelov: It doesn't seem like a `struct`.
CMapGeneratorWorker::~CMapGeneratorWorker()
{ {
// Cancel or wait for the task to end. public:
m_WorkerThread.CancelOrWait(); CMapGeneratorCallbackData(ScriptInterface& scriptInterface, std::atomic<int>& progress) :
} m_ScriptInterface{scriptInterface},
m_Progress{progress}
void CMapGeneratorWorker::Initialize(const VfsPath& scriptFile, const std::string& settings)
{ {
std::lock_guard<std::mutex> lock(m_WorkerMutex); m_ScriptInterface.SetCallbackData(static_cast<void*>(this));
// Set progress to positive value
m_Progress.store(1);
m_ScriptPath = scriptFile;
m_Settings = settings;
// Start generating the map asynchronously.
m_WorkerThread = Threading::TaskManager::Instance().PushTask([this]() {
PROFILE2("Map Generation");
std::shared_ptr<ScriptContext> mapgenContext = ScriptContext::CreateContext(RMS_CONTEXT_SIZE);
// Enable the script to be aborted // Enable the script to be aborted
JS_AddInterruptCallback(mapgenContext->GetGeneralJSContext(), MapGeneratorInterruptCallback); JS_AddInterruptCallback(m_ScriptInterface.GetGeneralJSContext(), &MapGeneratorInterruptCallback);
m_ScriptInterface = new ScriptInterface("Engine", "MapGenerator", mapgenContext);
// Run map generation scripts
if (!Run() || m_Progress.load() > 0)
{
// Don't leave progress in an unknown state, if generator failed, set it to -1
m_Progress.store(-1);
}
SAFE_DELETE(m_ScriptInterface);
// At this point the random map scripts are done running, so the thread has no further purpose
// and can die. The data will be stored in m_MapData already if successful, or m_Progress
// will contain an error value on failure.
});
}
bool CMapGeneratorWorker::Run()
{
ScriptRequest rq(m_ScriptInterface);
// Parse settings
JS::RootedValue settingsVal(rq.cx);
if (!Script::ParseJSON(rq, m_Settings, &settingsVal) && settingsVal.isUndefined())
{
LOGERROR("CMapGeneratorWorker::Run: Failed to parse settings");
return false;
}
// Prevent unintentional modifications to the settings object by random map scripts
if (!Script::FreezeObject(rq, settingsVal, true))
{
LOGERROR("CMapGeneratorWorker::Run: Failed to deepfreeze settings");
return false;
}
// Init RNG seed
u32 seed = 0;
if (!Script::HasProperty(rq, settingsVal, "Seed") ||
!Script::GetProperty(rq, settingsVal, "Seed", seed))
LOGWARNING("CMapGeneratorWorker::Run: No seed value specified - using 0");
InitScriptInterface(seed);
RegisterScriptFunctions_MapGenerator();
// Copy settings to global variable
JS::RootedValue global(rq.cx, rq.globalValue());
if (!Script::SetProperty(rq, global, "g_MapSettings", settingsVal, true, true))
{
LOGERROR("CMapGeneratorWorker::Run: Failed to define g_MapSettings");
return false;
} }
vladislavbelovUnsubmitted
Done
Inline Actions

I was thinking and I'm not sure about the name. CallbackData sounds like we store only data for callbacks but we also provides functions and other data. Maybe CMapGeneratorCallbackContext?

vladislavbelov: I was thinking and I'm not sure about the name. `CallbackData` sounds like we store only data…
phositAuthorUnsubmitted
Done
Inline Actions

Context sounds js related. How about only CMapGeneratorCallback or CMapGeneratorCallbacks?

phosit: `Context` sounds js related. How about only `CMapGeneratorCallback` or `CMapGeneratorCallbacks`?
vladislavbelovUnsubmitted
Done
Inline Actions

CMapGeneratorCallbacks seems fine.

vladislavbelov: `CMapGeneratorCallbacks` seems fine.
// Load RMS ~CMapGeneratorCallbackData()
LOGMESSAGE("Loading RMS '%s'", m_ScriptPath.string8());
if (!m_ScriptInterface->LoadGlobalScriptFile(m_ScriptPath))
{ {
LOGERROR("CMapGeneratorWorker::Run: Failed to load RMS '%s'", m_ScriptPath.string8()); JS_AddInterruptCallback(m_ScriptInterface.GetGeneralJSContext(), nullptr);
return false; m_ScriptInterface.SetCallbackData(nullptr);
} }
return true; /**
} * Load all scripts of the given library
*
#define REGISTER_MAPGEN_FUNC(func) \ * @param libraryName VfsPath specifying name of the library (subfolder of ../maps/random/)
ScriptFunction::Register<&CMapGeneratorWorker::func, ScriptInterface::ObjectFromCBData<CMapGeneratorWorker>>(rq, #func); * @return true if all scripts ran successfully, false if there's an error
#define REGISTER_MAPGEN_FUNC_NAME(func, name) \ */
ScriptFunction::Register<&CMapGeneratorWorker::func, ScriptInterface::ObjectFromCBData<CMapGeneratorWorker>>(rq, name); bool LoadLibrary(const VfsPath& libraryName)
void CMapGeneratorWorker::InitScriptInterface(const u32 seed)
{
m_ScriptInterface->SetCallbackData(static_cast<void*>(this));
m_ScriptInterface->ReplaceNondeterministicRNG(m_MapGenRNG);
m_MapGenRNG.seed(seed);
// VFS
JSI_VFS::RegisterScriptFunctions_ReadOnlySimulationMaps(*m_ScriptInterface);
// Globalscripts may use VFS script functions
m_ScriptInterface->LoadGlobalScripts();
// File loading
ScriptRequest rq(m_ScriptInterface);
REGISTER_MAPGEN_FUNC_NAME(LoadScripts, "LoadLibrary");
REGISTER_MAPGEN_FUNC_NAME(LoadHeightmap, "LoadHeightmapImage");
REGISTER_MAPGEN_FUNC(LoadMapTerrain);
// Engine constants
// Length of one tile of the terrain grid in metres.
// Useful to transform footprint sizes to the tilegrid coordinate system.
m_ScriptInterface->SetGlobal("TERRAIN_TILE_SIZE", static_cast<int>(TERRAIN_TILE_SIZE));
// Number of impassable tiles at the map border
m_ScriptInterface->SetGlobal("MAP_BORDER_WIDTH", static_cast<int>(MAP_EDGE_TILES));
}
void CMapGeneratorWorker::RegisterScriptFunctions_MapGenerator()
{
ScriptRequest rq(m_ScriptInterface);
// Template functions
REGISTER_MAPGEN_FUNC(GetTemplate);
REGISTER_MAPGEN_FUNC(TemplateExists);
REGISTER_MAPGEN_FUNC(FindTemplates);
REGISTER_MAPGEN_FUNC(FindActorTemplates);
// Progression and profiling
REGISTER_MAPGEN_FUNC(SetProgress);
REGISTER_MAPGEN_FUNC(GetMicroseconds);
REGISTER_MAPGEN_FUNC(ExportMap);
}
#undef REGISTER_MAPGEN_FUNC
#undef REGISTER_MAPGEN_FUNC_NAME
int CMapGeneratorWorker::GetProgress() const
{
return m_Progress.load();
}
double CMapGeneratorWorker::GetMicroseconds()
{
return JS_Now();
}
Script::StructuredClone CMapGeneratorWorker::GetResults()
{
std::lock_guard<std::mutex> lock(m_WorkerMutex);
return m_MapData;
}
void CMapGeneratorWorker::ExportMap(JS::HandleValue data)
{
{
// Copy results
std::lock_guard<std::mutex> lock(m_WorkerMutex);
m_MapData = Script::WriteStructuredClone(ScriptRequest(m_ScriptInterface), data);
}
m_Progress.store(0);
}
void CMapGeneratorWorker::SetProgress(int progress)
{
// When the task is started, `m_Progress` is only mutated by this thread.
const int currentProgress = m_Progress.load();
if (progress >= currentProgress)
m_Progress.store(progress);
else
LOGWARNING("The random map script tried to reduce the loading progress from %d to %d",
currentProgress, progress);
}
CParamNode CMapGeneratorWorker::GetTemplate(const std::string& templateName)
{
const CParamNode& templateRoot = m_TemplateLoader.GetTemplateFileData(templateName).GetOnlyChild();
if (!templateRoot.IsOk())
LOGERROR("Invalid template found for '%s'", templateName.c_str());
return templateRoot;
}
bool CMapGeneratorWorker::TemplateExists(const std::string& templateName)
{
return m_TemplateLoader.TemplateExists(templateName);
}
std::vector<std::string> CMapGeneratorWorker::FindTemplates(const std::string& path, bool includeSubdirectories)
{
return m_TemplateLoader.FindTemplates(path, includeSubdirectories, SIMULATION_TEMPLATES);
}
std::vector<std::string> CMapGeneratorWorker::FindActorTemplates(const std::string& path, bool includeSubdirectories)
{
return m_TemplateLoader.FindTemplates(path, includeSubdirectories, ACTOR_TEMPLATES);
}
bool CMapGeneratorWorker::LoadScripts(const VfsPath& libraryName)
{ {
// Ignore libraries that are already loaded // Ignore libraries that are already loaded
if (m_LoadedLibraries.find(libraryName) != m_LoadedLibraries.end()) if (m_LoadedLibraries.find(libraryName) != m_LoadedLibraries.end())
return true; return true;
// Mark this as loaded, to prevent it recursively loading itself // Mark this as loaded, to prevent it recursively loading itself
m_LoadedLibraries.insert(libraryName); m_LoadedLibraries.insert(libraryName);
VfsPath path = VfsPath(L"maps/random/") / libraryName / VfsPath(); VfsPath path = VfsPath(L"maps/random/") / libraryName / VfsPath();
VfsPaths pathnames; VfsPaths pathnames;
// Load all scripts in mapgen directory // Load all scripts in mapgen directory
Status ret = vfs::GetPathnames(g_VFS, path, L"*.js", pathnames); Status ret = vfs::GetPathnames(g_VFS, path, L"*.js", pathnames);
if (ret == INFO::OK) if (ret == INFO::OK)
{ {
for (const VfsPath& p : pathnames) for (const VfsPath& p : pathnames)
{ {
LOGMESSAGE("Loading map generator script '%s'", p.string8()); LOGMESSAGE("Loading map generator script '%s'", p.string8());
if (!m_ScriptInterface->LoadGlobalScriptFile(p)) if (!m_ScriptInterface.LoadGlobalScriptFile(p))
{ {
LOGERROR("CMapGeneratorWorker::LoadScripts: Failed to load script '%s'", p.string8()); LOGERROR("CMapGeneratorWorker::LoadScripts: Failed to load script '%s'", p.string8());
return false; return false;
} }
} }
} }
else else
{ {
// Some error reading directory // Some error reading directory
wchar_t error[200]; wchar_t error[200];
LOGERROR("CMapGeneratorWorker::LoadScripts: Error reading scripts in directory '%s': %s", path.string8(), utf8_from_wstring(StatusDescription(ret, error, ARRAY_SIZE(error)))); LOGERROR("CMapGeneratorWorker::LoadScripts: Error reading scripts in directory '%s': %s", path.string8(), utf8_from_wstring(StatusDescription(ret, error, ARRAY_SIZE(error))));
return false; return false;
} }
return true; return true;
} }
JS::Value CMapGeneratorWorker::LoadHeightmap(const VfsPath& filename) /**
* Finalize map generation and pass results from the script to the engine.
* The `data` has to be according to this format:
* https://trac.wildfiregames.com/wiki/Random_Map_Generator_Internals#Dataformat
*/
void ExportMap(JS::HandleValue data)
{
// Copy results
m_MapData = Script::WriteStructuredClone(ScriptRequest(m_ScriptInterface), data);
m_Progress.store(0);
}
vladislavbelovUnsubmitted
Done
Inline Actions

Might want to add a comment what should be in data.

vladislavbelov: Might want to add a comment what should be in `data`.
/**
* Load an image file and return it as a height array.
*/
JS::Value LoadHeightmapImage(const VfsPath& filename)
{ {
std::vector<u16> heightmap; std::vector<u16> heightmap;
if (LoadHeightmapImageVfs(filename, heightmap) != INFO::OK) if (LoadHeightmapImageVfs(filename, heightmap) != INFO::OK)
{ {
LOGERROR("Could not load heightmap file '%s'", filename.string8()); LOGERROR("Could not load heightmap file '%s'", filename.string8());
return JS::UndefinedValue(); return JS::UndefinedValue();
} }
ScriptRequest rq(m_ScriptInterface); ScriptRequest rq(m_ScriptInterface);
JS::RootedValue returnValue(rq.cx); JS::RootedValue returnValue(rq.cx);
Script::ToJSVal(rq, &returnValue, heightmap); Script::ToJSVal(rq, &returnValue, heightmap);
return returnValue; return returnValue;
} }
// See CMapReader::UnpackTerrain, CMapReader::ParseTerrain for the reordering /**
JS::Value CMapGeneratorWorker::LoadMapTerrain(const VfsPath& filename) * Load an Atlas terrain file (PMP) returning textures and heightmap.
*
* See CMapReader::UnpackTerrain, CMapReader::ParseTerrain for the reordering
*/
JS::Value LoadMapTerrain(const VfsPath& filename)
{ {
ScriptRequest rq(m_ScriptInterface); ScriptRequest rq(m_ScriptInterface);
if (!VfsFileExists(filename)) if (!VfsFileExists(filename))
{ {
ScriptException::Raise(rq, "Terrain file \"%s\" does not exist!", filename.string8().c_str()); ScriptException::Raise(rq, "Terrain file \"%s\" does not exist!", filename.string8().c_str());
return JS::UndefinedValue(); return JS::UndefinedValue();
} }
CFileUnpacker unpacker; CFileUnpacker unpacker;
unpacker.Read(filename, "PSMP"); unpacker.Read(filename, "PSMP");
if (unpacker.GetVersion() < CMapIO::FILE_READ_VERSION) if (unpacker.GetVersion() < CMapIO::FILE_READ_VERSION)
{ {
ScriptException::Raise(rq, "Could not load terrain file \"%s\" too old version!", filename.string8().c_str()); ScriptException::Raise(rq, "Could not load terrain file \"%s\" too old version!", filename.string8().c_str());
return JS::UndefinedValue(); return JS::UndefinedValue();
} }
// unpack size // unpack size
ssize_t patchesPerSide = (ssize_t)unpacker.UnpackSize(); ssize_t patchesPerSide = (ssize_t)unpacker.UnpackSize();
size_t verticesPerSide = patchesPerSide * PATCH_SIZE + 1; size_t verticesPerSide = patchesPerSide * PATCH_SIZE + 1;
// unpack heightmap // unpack heightmap
std::vector<u16> heightmap; std::vector<u16> heightmap;
heightmap.resize(SQR(verticesPerSide)); heightmap.resize(SQR(verticesPerSide));
unpacker.UnpackRaw(&heightmap[0], SQR(verticesPerSide) * sizeof(u16)); unpacker.UnpackRaw(&heightmap[0], SQR(verticesPerSide) * sizeof(u16));
// unpack texture names // unpack texture names
size_t textureCount = unpacker.UnpackSize(); size_t textureCount = unpacker.UnpackSize();
std::vector<std::string> textureNames; std::vector<std::string> textureNames;
textureNames.reserve(textureCount); textureNames.reserve(textureCount);
for (size_t i = 0; i < textureCount; ++i) for (size_t i = 0; i < textureCount; ++i)
{ {
CStr texturename; CStr texturename;
unpacker.UnpackString(texturename); unpacker.UnpackString(texturename);
textureNames.push_back(texturename); textureNames.push_back(texturename);
} }
// unpack texture IDs per tile // unpack texture IDs per tile
ssize_t tilesPerSide = patchesPerSide * PATCH_SIZE; ssize_t tilesPerSide = patchesPerSide * PATCH_SIZE;
std::vector<CMapIO::STileDesc> tiles; std::vector<CMapIO::STileDesc> tiles;
tiles.resize(size_t(SQR(tilesPerSide))); tiles.resize(size_t(SQR(tilesPerSide)));
unpacker.UnpackRaw(&tiles[0], sizeof(CMapIO::STileDesc) * tiles.size()); unpacker.UnpackRaw(&tiles[0], sizeof(CMapIO::STileDesc) * tiles.size());
// reorder by patches and store and save texture IDs per tile // reorder by patches and store and save texture IDs per tile
std::vector<u16> textureIDs; std::vector<u16> textureIDs;
for (ssize_t x = 0; x < tilesPerSide; ++x) for (ssize_t x = 0; x < tilesPerSide; ++x)
{ {
size_t patchX = x / PATCH_SIZE; size_t patchX = x / PATCH_SIZE;
size_t offX = x % PATCH_SIZE; size_t offX = x % PATCH_SIZE;
for (ssize_t y = 0; y < tilesPerSide; ++y) for (ssize_t y = 0; y < tilesPerSide; ++y)
{ {
size_t patchY = y / PATCH_SIZE; size_t patchY = y / PATCH_SIZE;
size_t offY = y % PATCH_SIZE; size_t offY = y % PATCH_SIZE;
// m_Priority and m_Tex2Index unused // m_Priority and m_Tex2Index unused
textureIDs.push_back(tiles[(patchY * patchesPerSide + patchX) * SQR(PATCH_SIZE) + (offY * PATCH_SIZE + offX)].m_Tex1Index); textureIDs.push_back(tiles[(patchY * patchesPerSide + patchX) * SQR(PATCH_SIZE) +
(offY * PATCH_SIZE + offX)].m_Tex1Index);
} }
} }
JS::RootedValue returnValue(rq.cx); JS::RootedValue returnValue(rq.cx);
Script::CreateObject( Script::CreateObject(
rq, rq,
&returnValue, &returnValue,
"height", heightmap, "height", heightmap,
"textureNames", textureNames, "textureNames", textureNames,
"textureIDs", textureIDs); "textureIDs", textureIDs);
return returnValue; return returnValue;
} }
////////////////////////////////////////////////////////////////////////////////// /**
////////////////////////////////////////////////////////////////////////////////// * Sets the map generation progress, which is one of multiple stages determining the loading screen progress.
*/
void SetProgress(int progress)
{
// When the task is started, `m_Progress` is only mutated by this thread.
const int currentProgress = m_Progress.load();
if (progress >= currentProgress)
m_Progress.store(progress);
else
LOGWARNING("The random map script tried to reduce the loading progress from %d to %d",
currentProgress, progress);
}
/**
* Microseconds since the epoch.
*/
double GetMicroseconds() const
vladislavbelovUnsubmitted
Done
Inline Actions

double GetMicroseconds() const?

vladislavbelov: `double GetMicroseconds() const`?
{
return JS_Now();
}
/**
* Return the template data of the given template name.
*/
CParamNode GetTemplate(const std::string& templateName)
vladislavbelovUnsubmitted
Done
Inline Actions

CParamNode GetTemplate(const std::string& templateName) const?

vladislavbelov: `CParamNode GetTemplate(const std::string& templateName) const`?
phositAuthorUnsubmitted
Done
Inline Actions

Can't be const CTemplateLoader::GetTemplateFileData isn't const.

phosit: Can't be const `CTemplateLoader::GetTemplateFileData` isn't const.
{
const CParamNode& templateRoot =
m_TemplateLoader.GetTemplateFileData(templateName).GetOnlyChild();
if (!templateRoot.IsOk())
LOGERROR("Invalid template found for '%s'", templateName.c_str());
return templateRoot;
}
/**
vladislavbelovUnsubmitted
Done
Inline Actions

bool TemplateExists(const std::string& templateName) const?

vladislavbelov: `bool TemplateExists(const std::string& templateName) const`?
* Check whether the given template exists.
*/
bool TemplateExists(const std::string& templateName) const
{
return m_TemplateLoader.TemplateExists(templateName);
}
/**
* Returns all template names of simulation entity templates.
*/
std::vector<std::string> FindTemplates(const std::string& path, bool includeSubdirectories)
{
return m_TemplateLoader.FindTemplates(path, includeSubdirectories, SIMULATION_TEMPLATES);
}
/**
* Returns all template names of actors.
*/
std::vector<std::string> FindActorTemplates(const std::string& path, bool includeSubdirectories)
{
return m_TemplateLoader.FindTemplates(path, includeSubdirectories, ACTOR_TEMPLATES);
}
/**
* Provides the script context.
*/
ScriptInterface& m_ScriptInterface;
wraitiiUnsubmitted
Done
Inline Actions

This one really sounds like it should be in the anon namespace, and kinda same below

wraitii: This one really sounds like it should be in the anon namespace, and kinda same below
phositAuthorUnsubmitted
Done
Inline Actions

Meh, a unnamed namespace has the same efect as static

phosit: Meh, a unnamed namespace has the same efect as `static`
/**
* Result of the mapscript generation including terrain, entities and environment settings.
*/
Script::StructuredClone m_MapData;
private:
/**
* Currently loaded script librarynames.
*/
std::set<VfsPath> m_LoadedLibraries;
/**
* Current map generation progress.
* Initialize to `-1`. If something happens before we start, that's a
vladislavbelovUnsubmitted
Done
Inline Actions

MapGenerator::INVALID_PROGRESS.

vladislavbelov: `MapGenerator::INVALID_PROGRESS`.
* failure.
*/
std::atomic<int>& m_Progress;
/**
* Backend to loading template data.
*/
CTemplateLoader m_TemplateLoader;
};
#define REGISTER_MAPGEN_FUNC(func) \
ScriptFunction::Register<&CMapGeneratorCallbackData::func, ScriptInterface::ObjectFromCBData<CMapGeneratorCallbackData>>(rq, #func);
/**
* Set initial seed, callback data.
* Expose functions, globals and classes defined in this class relevant to the map and test scripts.
*/
void InitScriptInterface(CMapGeneratorCallbackData& callbackData)
vladislavbelovUnsubmitted
Done
Inline Actions

Why it's not a method if it needs to access private members?

vladislavbelov: Why it's not a method if it needs to access private members?
phositAuthorUnsubmitted
Done
Inline Actions

There isn't a big difference between public and private since everything has internal linkage.
IMO it should be a struct. JS-callbacks have to be methods.

phosit: There isn't a big difference between `public` and `private` since everything has internal…
vladislavbelovUnsubmitted
Done
Inline Actions

There is a semantic difference. It's not a POD and it has some logic when accessing members. So external modification is less safe.

vladislavbelov: There is a semantic difference. It's not a POD and it has some logic when accessing members. So…
phositAuthorUnsubmitted
Done
Inline Actions

Yes it's not a POD. Does it have to be?
The "scope" of CMapGeneratorCallbackData isn't that big. So the bit of code which may modify it should know what it does. (The most distand code is JS and JS has to have access to private members through member functions.)

phosit: Yes it's not a POD. Does it have to be? The "scope" of `CMapGeneratorCallbackData` isn't that…
vladislavbelovUnsubmitted
Done
Inline Actions

In that case it's not critical.

Ideally I think we should use a) POD with possibility of an external modification or b) non-POD without external modification.

vladislavbelov: In that case it's not critical. Ideally I think we should use a) POD with possibility of an…
phositAuthorUnsubmitted
Done
Inline Actions

Why do you have such a strong oppinion about POD-idity? (It's deprecated in C++20 ;P)

The coreguidline is even stricter then you... I'll make all member-variables private.

phosit: Why do you have such a strong oppinion about POD-idity? (It's deprecated in C++20 ;P) The…
{
// VFS
JSI_VFS::RegisterScriptFunctions_ReadOnlySimulationMaps(callbackData.m_ScriptInterface);
// Globalscripts may use VFS script functions
callbackData.m_ScriptInterface.LoadGlobalScripts();
// File loading
ScriptRequest rq(callbackData.m_ScriptInterface);
vladislavbelovUnsubmitted
Done
Inline Actions

_ might be removed to follow CC.

vladislavbelov: `_` might be removed to follow CC.
phositAuthorUnsubmitted
Done
Inline Actions

What do you think about removing the whole _MapGenerator suffix? This whole file is about map generation.

phosit: What do you think about removing the whole `_MapGenerator` suffix? This whole file is about map…
vladislavbelovUnsubmitted
Done
Inline Actions

Yeah, it's ok.

vladislavbelov: Yeah, it's ok.
REGISTER_MAPGEN_FUNC(LoadLibrary);
REGISTER_MAPGEN_FUNC(LoadHeightmapImage);
REGISTER_MAPGEN_FUNC(LoadMapTerrain);
// Engine constants
// Length of one tile of the terrain grid in metres.
// Useful to transform footprint sizes to the tilegrid coordinate system.
callbackData.m_ScriptInterface.SetGlobal("TERRAIN_TILE_SIZE", static_cast<int>(TERRAIN_TILE_SIZE));
// Number of impassable tiles at the map border
callbackData.m_ScriptInterface.SetGlobal("MAP_BORDER_WIDTH", static_cast<int>(MAP_EDGE_TILES));
}
/**
* Expose functions defined in this class that are relevant to mapscripts but not the tests.
*/
void RegisterScriptFunctions_MapGenerator(CMapGeneratorCallbackData& callbackData)
{
ScriptRequest rq(callbackData.m_ScriptInterface);
// Template functions
REGISTER_MAPGEN_FUNC(GetTemplate);
REGISTER_MAPGEN_FUNC(TemplateExists);
REGISTER_MAPGEN_FUNC(FindTemplates);
REGISTER_MAPGEN_FUNC(FindActorTemplates);
CMapGenerator::CMapGenerator() : m_Worker(new CMapGeneratorWorker(nullptr)) // Progression and profiling
REGISTER_MAPGEN_FUNC(SetProgress);
REGISTER_MAPGEN_FUNC(GetMicroseconds);
REGISTER_MAPGEN_FUNC(ExportMap);
}
#undef REGISTER_MAPGEN_FUNC
}
Script::StructuredClone MapGenerator::GenerateMap(std::atomic<int>& progress,
ScriptInterface& scriptInterface, const VfsPath& script, const std::string& settings,
const bool isTest)
{
ScriptRequest rq(scriptInterface);
// Parse settings
vladislavbelovUnsubmitted
Done
Inline Actions

We don't use unnamed namespace comment, anonymous namespace or empty.

vladislavbelov: We don't use `unnamed namespace` comment, `anonymous namespace` or empty.
JS::RootedValue settingsVal(rq.cx);
if (!Script::ParseJSON(rq, settings, &settingsVal) && settingsVal.isUndefined())
{ {
LOGERROR("CMapGeneratorWorker::Run: Failed to parse settings");
vladislavbelovUnsubmitted
Done
Inline Actions

Outdated messages here and below.

vladislavbelov: Outdated messages here and below.
progress.store(MapGenerator::INVALID_PROGRESS);
return nullptr;
} }
CMapGenerator::~CMapGenerator() // Prevent unintentional modifications to the settings object by random map scripts
if (!Script::FreezeObject(rq, settingsVal, true))
{ {
delete m_Worker; LOGERROR("CMapGeneratorWorker::Run: Failed to deepfreeze settings");
progress.store(MapGenerator::INVALID_PROGRESS);
vladislavbelovUnsubmitted
Done
Inline Actions

Too many repeats of -1, might be a named constant.

vladislavbelov: Too many repeats of `-1`, might be a named constant.
phositAuthorUnsubmitted
Done
Inline Actions

Idealy errors are handled using exceptions.

phosit: Idealy errors are handled using exceptions.
return nullptr;
} }
void CMapGenerator::GenerateMap(const VfsPath& scriptFile, const std::string& settings) // Init RNG seed
u32 seed = 0;
if (!Script::HasProperty(rq, settingsVal, "Seed") ||
!Script::GetProperty(rq, settingsVal, "Seed", seed))
LOGWARNING("CMapGeneratorWorker::Run: No seed value specified - using 0");
boost::rand48 mapGenRNG{seed};
scriptInterface.ReplaceNondeterministicRNG(mapGenRNG);
CMapGeneratorCallbackData callbackData{scriptInterface, progress};
InitScriptInterface(callbackData);
if (!isTest)
{
RegisterScriptFunctions_MapGenerator(callbackData);
// Copy settings to global variable
JS::RootedValue global(rq.cx, rq.globalValue());
if (!Script::SetProperty(rq, global, "g_MapSettings", settingsVal, true, true))
{ {
m_Worker->Initialize(scriptFile, settings); LOGERROR("CMapGeneratorWorker::Run: Failed to define g_MapSettings");
progress.store(-1);
vladislavbelovUnsubmitted
Done
Inline Actions

MapGenerator::INVALID_PROGRESS.

vladislavbelov: `MapGenerator::INVALID_PROGRESS`.
return nullptr;
}
} }
vladislavbelovUnsubmitted
Done
Inline Actions

Why it can't work in tests?

vladislavbelov: Why it can't work in tests?
phositAuthorUnsubmitted
Done
Inline Actions

I'm not sure. The tests use mock functions and Engine.GetTemplate can't be overwritten.

phosit: I'm not sure. The tests use mock functions and `Engine.GetTemplate` can't be overwritten.
vladislavbelovUnsubmitted
Done
Inline Actions

Why it was working before the patch?

vladislavbelov: Why it was working before the patch?
phositAuthorUnsubmitted
Done
Inline Actions

Before the patch the test also didn't executed that.

phosit: Before the patch the test also didn't executed that.
int CMapGenerator::GetProgress() const // Load RMS
vladislavbelovUnsubmitted
Done
Inline Actions

Why it should fail in tests?

vladislavbelov: Why it should fail in tests?
phositAuthorUnsubmitted
Done
Inline Actions

I don't understand this question: No it should not fail in tests...
The tests (js-scripts) define Engine.GetTemplate and g_MapSettings. If we define it from the outside it can't be overwritten.

phosit: I don't understand this question: No it should not fail in tests... The tests (js-scripts)…
vladislavbelovUnsubmitted
Done
Inline Actions

I'm worried about having production code under if section which depends on tests environment.

vladislavbelov: I'm worried about having production code under `if` section which depends on tests environment.
phositAuthorUnsubmitted
Done
Inline Actions

The other approach would be to split up this function and expose multiple functions in the .h file. Which would make the interface more complicated again. (CMapGeneratorCallbacks would also have to be exposed.)

A different design might be to pass a function (function-ptr or std::function) which could act on the rq to set mock-functions or the production-functions. This is very similar to the if solution but allows more customization.

phosit: The other approach would be to split up this function and expose multiple functions in the .h…
LOGMESSAGE("Loading RMS '%s'", script.string8());
vladislavbelovUnsubmitted
Done
Inline Actions

InitScriptInterface is called without isTest then RegisterScriptFunctions should be also called without it.

vladislavbelov: `InitScriptInterface` is called without `isTest` then `RegisterScriptFunctions` should be also…
phositAuthorUnsubmitted
Done
Inline Actions

I don't know excactly which functions the test-scripts use. I kept it as before.

phosit: I don't know excactly which functions the test-scripts use. I kept it as before.
vladislavbelovUnsubmitted
Not Done
Inline Actions

I don't know excactly which functions the test-scripts use.

It contradicts to your comment:

// The test script don't call `ExportMap` so `GenerateMap` allways returns `nullptr`.
vladislavbelov: > I don't know excactly which functions the test-scripts use. It contradicts to your comment…
phositAuthorUnsubmitted
Done
Inline Actions

ExportMap is a function which is called in the test-scripts directly.

I don't know exactly which functions the test-scripts use.

... Because they use some functions indirectly.

The thing I know for sure is that some test-scripts don't call ExportMap.

phosit: `ExportMap` is a function which is called in the test-scripts directly. > I don't know exactly…
vladislavbelovUnsubmitted
Not Done
Inline Actions

We can create g_MapSettings in tests (also rq might be passed as a parameter).

vladislavbelov: We can create `g_MapSettings` in tests (also `rq` might be passed as a parameter).
phositAuthorUnsubmitted
Done
Inline Actions

No, the test scripts define g_MapSettings. Also g_MapSettings is "frozen".

phosit: No, the test scripts define `g_MapSettings`. Also `g_MapSettings` is "frozen".
vladislavbelovUnsubmitted
Not Done
Inline Actions

I'd remove g_MapSettings from test_*.js and add the size parameter to the settings parameter: "{\"Seed\": 0, \"Size\": 512}", else the settings parameter is passed but unused by tests.

vladislavbelov: I'd remove `g_MapSettings` from `test_*.js` and add the size parameter to the `settings`…
phositAuthorUnsubmitted
Done
Inline Actions

test_DiskPlacer.js uses multiple different g_MapSettings. {"Seed": 0, "Size": 512, "CircularMap": true} should work for all tests.

phosit: test_DiskPlacer.js uses multiple different `g_MapSettings`. `{"Seed": 0, "Size": 512…
vladislavbelovUnsubmitted
Not Done
Inline Actions

Hmm. Maybe we need additional test environment. But it seems like a lot of work. For example test_*.js files don't have code in the global space but rather define test_* functions/test cases (like in C++ tests) where we can define possible sizes and other map properties. For example (not the cleanest one):

function test_small_maps()
{
	return {
		"test": () = {
			let map = new RandomMap(0, 0, "blackness");
			// Does not error with floating point radius
			let area = createArea(map, new DiskPlacer(86.4, new Vector2D(160, 160)));
			// ...
		},
		"cases": [{"Size": 320}, {"Size": 512, "CircularMap": false}],
	}
}
vladislavbelov: Hmm. Maybe we need additional test environment. But it seems like a lot of work. For example…
phositAuthorUnsubmitted
Done
Inline Actions

Yes that would certenly be cleaner. But it diverges more from the normal map generation scripts.

phosit: Yes that would certenly be cleaner. But it diverges more from the normal map generation scripts.
if (!scriptInterface.LoadGlobalScriptFile(script))
{ {
return m_Worker->GetProgress(); LOGERROR("CMapGeneratorWorker::Run: Failed to load RMS '%s'", script.string8());
progress.store(MapGenerator::INVALID_PROGRESS);
return nullptr;
}
vladislavbelovUnsubmitted
Not Done
Inline Actions

I'd add a comment here to describe which scoped is ended here: } // anonymous namespace.

vladislavbelov: I'd add a comment here to describe which scoped is ended here: `} // anonymous namespace`.
return progress.load() == 0 ? std::move(callbackData.m_MapData) : nullptr;
vladislavbelovUnsubmitted
Done
Inline Actions

I think it's better to use {} instead of nullptr.

vladislavbelov: I think it's better to use `{}` instead of `nullptr`.
phositAuthorUnsubmitted
Done
Inline Actions

{} can't be used in ternary expressions.

Also nullptr is returned in other places on error.

phosit: `{}` can't be used in ternary expressions. Also `nullptr` is returned in other places on error.
vladislavbelovUnsubmitted
Done
Inline Actions

Pure {} - can't, Script::StructuredClone{} can be used. But I don't insist here.

vladislavbelov: Pure `{}` - can't, `Script::StructuredClone{}` can be used. But I don't insist here.
phositAuthorUnsubmitted
Done
Inline Actions

I'd still use nullptr to keep it consistent with the other returns.

phosit: I'd still use `nullptr` to keep it consistent with the other `returns`.
} }
Script::StructuredClone CMapGenerator::GetResults() Script::StructuredClone MapGenerator::GenerateMap(std::atomic<int>& progress, const VfsPath& script,
const std::string& settings)
{ {
return m_Worker->GetResults(); const std::shared_ptr<ScriptContext> mapgenContext = ScriptContext::CreateContext(RMS_CONTEXT_SIZE);
ScriptInterface scriptInterface{"Engine", "MapGenerator", mapgenContext};
return MapGenerator::GenerateMap(progress, scriptInterface, script, settings, false);
} }
Wildfire Games · Phabricator