Changeset View
Standalone View
source/graphics/MapReader.cpp
Show All 26 Lines | |||||
#include "graphics/Patch.h" | #include "graphics/Patch.h" | ||||
#include "graphics/Terrain.h" | #include "graphics/Terrain.h" | ||||
#include "graphics/TerrainTextureEntry.h" | #include "graphics/TerrainTextureEntry.h" | ||||
#include "graphics/TerrainTextureManager.h" | #include "graphics/TerrainTextureManager.h" | ||||
#include "lib/timer.h" | #include "lib/timer.h" | ||||
#include "maths/MathUtil.h" | #include "maths/MathUtil.h" | ||||
#include "ps/CLogger.h" | #include "ps/CLogger.h" | ||||
#include "ps/Loader.h" | #include "ps/Loader.h" | ||||
#include "ps/TaskManager.h" | |||||
#include "ps/World.h" | #include "ps/World.h" | ||||
#include "ps/XML/Xeromyces.h" | #include "ps/XML/Xeromyces.h" | ||||
#include "renderer/PostprocManager.h" | #include "renderer/PostprocManager.h" | ||||
#include "renderer/SkyManager.h" | #include "renderer/SkyManager.h" | ||||
#include "renderer/WaterManager.h" | #include "renderer/WaterManager.h" | ||||
#include "scriptinterface/Object.h" | #include "scriptinterface/Object.h" | ||||
#include "scriptinterface/ScriptContext.h" | #include "scriptinterface/ScriptContext.h" | ||||
#include "scriptinterface/ScriptInterface.h" | |||||
#include "scriptinterface/ScriptRequest.h" | #include "scriptinterface/ScriptRequest.h" | ||||
#include "scriptinterface/JSON.h" | #include "scriptinterface/JSON.h" | ||||
#include "simulation2/Simulation2.h" | #include "simulation2/Simulation2.h" | ||||
#include "simulation2/components/ICmpCinemaManager.h" | #include "simulation2/components/ICmpCinemaManager.h" | ||||
#include "simulation2/components/ICmpGarrisonHolder.h" | #include "simulation2/components/ICmpGarrisonHolder.h" | ||||
#include "simulation2/components/ICmpObstruction.h" | #include "simulation2/components/ICmpObstruction.h" | ||||
#include "simulation2/components/ICmpOwnership.h" | #include "simulation2/components/ICmpOwnership.h" | ||||
#include "simulation2/components/ICmpPlayer.h" | #include "simulation2/components/ICmpPlayer.h" | ||||
#include "simulation2/components/ICmpPlayerManager.h" | #include "simulation2/components/ICmpPlayerManager.h" | ||||
#include "simulation2/components/ICmpPosition.h" | #include "simulation2/components/ICmpPosition.h" | ||||
#include "simulation2/components/ICmpTerrain.h" | #include "simulation2/components/ICmpTerrain.h" | ||||
#include "simulation2/components/ICmpTurretHolder.h" | #include "simulation2/components/ICmpTurretHolder.h" | ||||
#include "simulation2/components/ICmpVisual.h" | #include "simulation2/components/ICmpVisual.h" | ||||
#include "simulation2/components/ICmpWaterManager.h" | #include "simulation2/components/ICmpWaterManager.h" | ||||
#include <boost/algorithm/string/predicate.hpp> | #include <boost/algorithm/string/predicate.hpp> | ||||
#if defined(_MSC_VER) && _MSC_VER > 1900 | #if defined(_MSC_VER) && _MSC_VER > 1900 | ||||
#pragma warning(disable: 4456) // Declaration hides previous local declaration. | #pragma warning(disable: 4456) // Declaration hides previous local declaration. | ||||
#pragma warning(disable: 4458) // Declaration hides class member. | #pragma warning(disable: 4458) // Declaration hides class member. | ||||
#endif | #endif | ||||
CMapReader::CMapReader() | CMapReader::CMapReader() | ||||
: xml_reader(0), m_PatchesPerSide(0), m_MapGen(0) | : xml_reader(0), m_PatchesPerSide(0) | ||||
vladislavbelov: JFYI we have `KiB`, `MiB` and `GiB` constants :) | |||||
{ | { | ||||
cur_terrain_tex = 0; // important - resets generator state | cur_terrain_tex = 0; // important - resets generator state | ||||
} | } | ||||
// LoadMap: try to load the map from given file; reinitialise the scene to new data if successful | // LoadMap: try to load the map from given file; reinitialise the scene to new data if successful | ||||
void CMapReader::LoadMap(const VfsPath& pathname, const ScriptContext& cx, JS::HandleValue settings, CTerrain *pTerrain_, | void CMapReader::LoadMap(const VfsPath& pathname, const ScriptContext& cx, JS::HandleValue settings, CTerrain *pTerrain_, | ||||
WaterManager* pWaterMan_, SkyManager* pSkyMan_, | WaterManager* pWaterMan_, SkyManager* pSkyMan_, | ||||
CLightEnv *pLightEnv_, CGameView *pGameView_, CCinemaManager* pCinema_, CTriggerManager* pTrigMan_, CPostprocManager* pPostproc_, | CLightEnv *pLightEnv_, CGameView *pGameView_, CCinemaManager* pCinema_, CTriggerManager* pTrigMan_, CPostprocManager* pPostproc_, | ||||
▲ Show 20 Lines • Show All 1,229 Lines • ▼ Show 20 Lines | if (ret <= 0) | ||||
SAFE_DELETE(xml_reader); | SAFE_DELETE(xml_reader); | ||||
} | } | ||||
return ret; | return ret; | ||||
} | } | ||||
//////////////////////////////////////////////////////////////////////////////////////////////////////////////// | //////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||||
//////////////////////////////////////////////////////////////////////////////////////////////////////////////// | //////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||||
Done Inline ActionsThis seems like it could be moved to GenerateMap ? wraitii: This seems like it could be moved to GenerateMap ? | |||||
Done Inline ActionsThe test does inject it's own ScriptInterface. Because of ScriptTestSetup. phosit: The test does inject it's own `ScriptInterface`. Because of `ScriptTestSetup`.
I wan't the… | |||||
Done Inline ActionsAh, right, missed that. I don't think it's a big issue if the 'inner-implementation' called by tests expects a scriptInterface and the regular code doesn't. In doubt, test code should not make our actual code worse, which I think it does here slightly. wraitii: Ah, right, missed that.
Still think moving this to MapGenerator.cpp would be a good idea, as… | |||||
int CMapReader::LoadRMSettings() | int CMapReader::LoadRMSettings() | ||||
{ | { | ||||
// copy random map settings over to sim | // copy random map settings over to sim | ||||
ENSURE(pSimulation2); | ENSURE(pSimulation2); | ||||
pSimulation2->SetMapSettings(m_ScriptSettings); | pSimulation2->SetMapSettings(m_ScriptSettings); | ||||
return 0; | return 0; | ||||
} | } | ||||
struct CMapReader::GeneratorState | |||||
{ | |||||
std::atomic<int> progress{CMapGenerator::INVALID_PROGRESS}; | |||||
Future<Script::StructuredClone> task; | |||||
~GeneratorState() | |||||
{ | |||||
task.CancelOrWait(); | |||||
} | |||||
}; | |||||
Done Inline ActionsEmpty line. vladislavbelov: Empty line. | |||||
int CMapReader::GenerateMap(const CStrW& scriptFile) | int CMapReader::GenerateMap(const CStrW& scriptFile) | ||||
{ | { | ||||
ScriptRequest rq(pSimulation2->GetScriptInterface()); | ScriptRequest rq(pSimulation2->GetScriptInterface()); | ||||
Not Done Inline ActionsIt's preferable to have a verb in the name of a method. I'd suggest StartMapGeneration. vladislavbelov: It's preferable to have a verb in the name of a method. I'd suggest `StartMapGeneration`. | |||||
Done Inline ActionsI wouldn't make such a broad rule. Certenly not for queries all_of, contains phosit: I wouldn't make such a broad rule. Certenly not for queries `all_of`, `contains`
Splitting this… | |||||
Not Done Inline ActionsThose functions aren't methods. Also contains is a verb and I think there might be are omitted in all_of. In MapGeneration it's ambiguous, you can't distinct: does it have omitted Do, Start, Delay or something else without looking at the implementation. vladislavbelov: Those functions aren't methods. Also `contains` is a verb and I think there might be `are`… | |||||
if (!m_MapGen) | if (!m_GeneratorState) | ||||
{ | { | ||||
// Initialize map generator | // Initialize map generator | ||||
m_MapGen = new CMapGenerator(); | m_GeneratorState = std::make_unique<GeneratorState>(); | ||||
VfsPath scriptPath; | |||||
if (scriptFile.length()) | |||||
scriptPath = L"maps/random/" + scriptFile; | |||||
// Stringify settings to pass across threads | // Stringify settings to pass across threads | ||||
std::string scriptSettings = Script::StringifyJSON(rq, &m_ScriptSettings); | std::string scriptSettings = Script::StringifyJSON(rq, &m_ScriptSettings); | ||||
// No error occured. Set `progress` to a non-error value. | |||||
Not Done Inline Actions// No error occured. Set progress` to a non-error and non-complete value.` vladislavbelov: `// No error occured. Set `progress` to a non-error and non-complete value.` | |||||
// Don't do it in the task since `progress` might be checked before the task is started. | |||||
Done Inline ActionsThat comment is redundant, it's clear that we set the positive value. But what can be useful is a comment why we do so. vladislavbelov: That comment is redundant, it's clear that we set the positive value. But what can be useful is… | |||||
// Don't do it after the task is started since we must not write to `progress` while | |||||
// `GenerateMap` is running. | |||||
m_GeneratorState->progress.store(1); | |||||
// Try to generate map | // Try to generate map | ||||
m_MapGen->GenerateMap(scriptPath, scriptSettings); | m_GeneratorState->task = Threading::TaskManager::Instance().PushTask( | ||||
[this, scriptFile, settings = std::move(scriptSettings)] | |||||
{ | |||||
PROFILE2("Map Generation"); | |||||
const auto scriptPath = scriptFile.empty() ? VfsPath{} : L"maps/random/" + scriptFile; | |||||
Done Inline ActionsIt seems a wrong indentation. vladislavbelov: It seems a wrong indentation. | |||||
Done Inline ActionsWe have to find a CC about intentation of lambdas ^^ phosit: We have to find a CC about intentation of lambdas ^^ | |||||
return CMapGenerator::GenerateMap(m_GeneratorState->progress, scriptPath, settings); | |||||
}); | |||||
Done Inline ActionsCan we move scriptPath and scriptSettings instead of copying? vladislavbelov: Can we move `scriptPath` and `scriptSettings` instead of copying? | |||||
Done Inline ActionsNow i std::move scriptSettings into the closure and construct scriptSettings in the lambda-body. phosit: Now i `std::move` `scriptSettings` into the closure and construct `scriptSettings` in the… | |||||
} | } | ||||
Done Inline ActionsCurrently it should be: m_GeneratorState->task = Threading::TaskManager::Instance().PushTask( [this, scriptFile, settings = std::move(scriptSettings)] { PROFILE2("Map Generation"); const auto scriptPath = scriptFile.empty() ? VfsPath{} : L"maps/random/" + scriptFile; return CMapGenerator::GenerateMap(m_GeneratorState->progress, scriptPath, settings); }); vladislavbelov: Currently it should be:
```lang=cpp
m_GeneratorState->task = Threading::TaskManager::Instance… | |||||
// Check status | // Check status | ||||
int progress = m_MapGen->GetProgress(); | const int progress = m_GeneratorState->progress.load(); | ||||
if (progress < 0) | if (progress == CMapGenerator::INVALID_PROGRESS) | ||||
{ | { | ||||
// RMS failed - return to main menu | // RMS failed - return to main menu | ||||
throw PSERROR_Game_World_MapLoadFailed("Error generating random map.\nCheck application log for details."); | throw PSERROR_Game_World_MapLoadFailed("Error generating random map.\nCheck application log for details."); | ||||
} | } | ||||
else if (progress == 0) | else if (progress == 0) | ||||
{ | { | ||||
// Finished, get results as StructuredClone object, which must be read to obtain the JS::Value | // Finished, get results as StructuredClone object, which must be read to obtain the JS::Value | ||||
Script::StructuredClone results = m_MapGen->GetResults(); | Script::StructuredClone results = m_GeneratorState->task.Get(); | ||||
// Parse data into simulation context | // Parse data into simulation context | ||||
JS::RootedValue data(rq.cx); | JS::RootedValue data(rq.cx); | ||||
Script::ReadStructuredClone(rq, results, &data); | Script::ReadStructuredClone(rq, results, &data); | ||||
if (data.isUndefined()) | if (data.isUndefined()) | ||||
{ | { | ||||
// RMS failed - return to main menu | // RMS failed - return to main menu | ||||
▲ Show 20 Lines • Show All 284 Lines • ▼ Show 20 Lines | #undef GET_CAMERA_PROPERTY | ||||
return 0; | return 0; | ||||
} | } | ||||
CMapReader::~CMapReader() | CMapReader::~CMapReader() | ||||
{ | { | ||||
// Cleaup objects | // Cleaup objects | ||||
delete xml_reader; | delete xml_reader; | ||||
delete m_MapGen; | |||||
} | } |
JFYI we have KiB, MiB and GiB constants :)