Page MenuHomeWildfire Games
Differential D5001 Diff 22099 source/graphics/MapReader.cpp

Changeset View

Standalone View

View Options

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)
vladislavbelovUnsubmitted
Not Done
Inline Actions

JFYI we have KiB, MiB and GiB constants :)

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;
} }
//////////////////////////////////////////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////////////////////////////////////////////
wraitiiUnsubmitted
Done
Inline Actions

This seems like it could be moved to GenerateMap ?

wraitii: This seems like it could be moved to GenerateMap ?
phositAuthorUnsubmitted
Done
Inline Actions

The test does inject it's own ScriptInterface. Because of ScriptTestSetup.
I wan't the testing and the non testing interface to be similar.

phosit: The test does inject it's own `ScriptInterface`. Because of `ScriptTestSetup`. I wan't the…
wraitiiUnsubmitted
Done
Inline Actions

Ah, right, missed that.
Still think moving this to MapGenerator.cpp would be a good idea, as RMS_CONTEXT_SIZE is useful there.

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();
}
};
vladislavbelovUnsubmitted
Done
Inline Actions

Empty line.

vladislavbelov: Empty line.
int CMapReader::GenerateMap(const CStrW& scriptFile) int CMapReader::GenerateMap(const CStrW& scriptFile)
{ {
ScriptRequest rq(pSimulation2->GetScriptInterface()); ScriptRequest rq(pSimulation2->GetScriptInterface());
vladislavbelovUnsubmitted
Not Done
Inline Actions

It'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`.
phositAuthorUnsubmitted
Done
Inline Actions

I wouldn't make such a broad rule. Certenly not for queries all_of, contains
Splitting this function up in to StartMapGeneration and PollMapGeneration might be a good idea.

phosit: I wouldn't make such a broad rule. Certenly not for queries `all_of`, `contains` Splitting this…
vladislavbelovUnsubmitted
Not Done
Inline Actions

Those 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.
vladislavbelovUnsubmitted
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.
vladislavbelovUnsubmitted
Done
Inline Actions

That 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;
vladislavbelovUnsubmitted
Done
Inline Actions

It seems a wrong indentation.

vladislavbelov: It seems a wrong indentation.
phositAuthorUnsubmitted
Done
Inline Actions

We 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);
});
vladislavbelovUnsubmitted
Done
Inline Actions

Can we move scriptPath and scriptSettings instead of copying?

vladislavbelov: Can we move `scriptPath` and `scriptSettings` instead of copying?
phositAuthorUnsubmitted
Done
Inline Actions

Now 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…
} }
vladislavbelovUnsubmitted
Done
Inline Actions

Currently 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;
} }
Wildfire Games · Phabricator