Page MenuHomeWildfire Games
Differential D1085 Diff 4445 source/ps/scripting/JSInterface_VFS.cpp

Changeset View

Standalone View

View Options

source/ps/scripting/JSInterface_VFS.cpp

Show All 22 Lines
#include "ps/CStr.h" #include "ps/CStr.h"
#include "ps/Filesystem.h" #include "ps/Filesystem.h"
#include "scriptinterface/ScriptVal.h" #include "scriptinterface/ScriptVal.h"
#include "scriptinterface/ScriptInterface.h" #include "scriptinterface/ScriptInterface.h"
#include "ps/scripting/JSInterface_VFS.h" #include "ps/scripting/JSInterface_VFS.h"
#include "lib/file/vfs/vfs_util.h" #include "lib/file/vfs/vfs_util.h"
// shared error handling code // shared error handling code
#define JS_CHECK_FILE_ERR(err)\ #define JS_CHECK_FILE_ERR(err)\
wraitiiUnsubmitted
Not Done
Inline Actions

Think you should put this above the function definition rather than at the top. Also constexpr perhaps? As we're going to drop VS13 soon

wraitii: Think you should put this above the function definition rather than at the top. Also constexpr…
elexisAuthorUnsubmitted
Not Done
Inline Actions

Shouldn't nonlocal variables always be mentioned at the top of the file?
I'm not familiar with VS2013 restrictions, can you explain me what you mean? Afaics constexpr means const and that the variable can be inlined, but const vectors and maps won't be dropped, will they?

elexis: Shouldn't nonlocal variables always be mentioned at the top of the file? I'm not familiar with…
wraitiiUnsubmitted
Not Done
Inline Actions

Well we declare globals at the top, but this is eminently local. In fact it should be static const, and put somewhere around the macro imo. "static" would make it unavailable to other compilation units.

I don't have a complete mastery of constexpr admittedly, so I'll link http://en.cppreference.com/w/cpp/language/constexpr, but my understanding is that it's supposed to tell the compiler that this can be evaluated at compile-time, which is much stronger than const.

Now in this case I'm actually wrong as none of this is a literal type so it wouldn't compile, but it'd still be better if possible, as the compiler could inline/precompute it all. Might be possible come C++20 or so.

wraitii: Well we declare globals at the top, but this is eminently local. In fact it should be static…
elexisAuthorUnsubmitted
Not Done
Inline Actions

Constants at the top also help documenting the concepts used by the code that follows and configuration before being confronted with code.

Are(n't) C++ compilers we care about are smart enough to evaluate at compile time that a constant map of constant vectors can be inlined?
And indeed the type ‘const std::map<CStr8, std::vector<CStrW> >’ of constexpr variable ‘pathRestriction’ is not literal

Correct, static was missing.

elexis: Constants at the top also help documenting the concepts used by the code that follows and…
wraitiiUnsubmitted
Not Done
Inline Actions

Are(n't) C++ compilers we care about are smart enough to evaluate at compile time that a constant map of constant vectors can be inlined?

Inlined, probably, but "completely taken apart and basically meta-programmed in", probably not.

I still think it should go above/inside the function where it's used, but it's not a huge thing.

wraitii: > Are(n't) C++ compilers we care about are smart enough to evaluate at compile time that a…
elexisAuthorUnsubmitted
Not Done
Inline Actions

The last word wasnt spoken in this file anyway. I'm eager to see if this file can be come agnostic of the scripts afterwards.

elexis: The last word wasnt spoken in this file anyway. I'm eager to see if this file can be come…
/* this is liable to happen often, so don't complain */\ /* this is liable to happen often, so don't complain */\
if (err == ERR::VFS_FILE_NOT_FOUND)\ if (err == ERR::VFS_FILE_NOT_FOUND)\
elexisAuthorUnsubmitted
Not Done
Inline Actions

Could also be simulation/data/. Depends on whether we wish to enforce our will to only use that subdirectory on mods and future vanilla developers. Doesn't really seem needed to manifest it here.

elexis: Could also be `simulation/data/`. Depends on whether we wish to enforce our will to only use…
{\ {\
return 0; \ return 0; \
}\ }\
/* unknown failure. We output an error message. */\ /* unknown failure. We output an error message. */\
else if (err < 0)\ else if (err < 0)\
LOGERROR("Unknown failure in VFS %i", err ); LOGERROR("Unknown failure in VFS %i", err );
/* else: success */ /* else: success */
▲ Show 20 Lines • Show All 131 Lines • ▼ Show 20 Lines while (std::getline(ss, line))
JS::RootedValue val(cx); JS::RootedValue val(cx);
ScriptInterface::ToJSVal(cx, &val, CStr(line).FromUTF8()); ScriptInterface::ToJSVal(cx, &val, CStr(line).FromUTF8());
JS_SetElement(cx, line_array, cur_line++, val); JS_SetElement(cx, line_array, cur_line++, val);
} }
return JS::ObjectValue(*line_array); return JS::ObjectValue(*line_array);
} }
JS::Value JSI_VFS::ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath) JS::Value JSI_VFS::Script_ReadJSONFile_GUI(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath)
{ {
return ReadJSONFile(pCxPrivate, {L""}, filePath);
}
JS::Value JSI_VFS::Script_ReadJSONFile_Simulation(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath)
{
return ReadJSONFile(pCxPrivate, {L"simulation/"}, filePath);
}
JS::Value JSI_VFS::Script_ReadJSONFile_Maps(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath)
{
return ReadJSONFile(pCxPrivate, {L"simulation/", L"maps/"}, filePath);
}
JS::Value JSI_VFS::ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::vector<std::wstring>& validPaths, const std::wstring& filePath)
elexisAuthorUnsubmitted
Not Done
Inline Actions

If would have been nicer to abstract these functions.
The next patch after this one should unfiy the FileExist and ListFiles functions and those will need one wrapper each as well.

One wrapper per context per function is about a dozen of lines each.
That seems cheap and preferable over not having unifiable globalscripts/ code.

Not having to require one wrapper for each function by using some C++ magic (macros? templates? classes?) would yield less repetition, less to read, less to maintain, less surface for inconsistency bugs. But such voodoo can still be done later. Also we don't want another NativeWrapperDecls.h.

elexis: If would have been nicer to abstract these functions. The next patch after this one should…
{
if (!PathRestrictionMet(pCxPrivate, validPaths, filePath))
return JS::NullValue();
JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); JSContext* cx = pCxPrivate->pScriptInterface->GetContext();
JSAutoRequest rq(cx); JSAutoRequest rq(cx);
JS::RootedValue out(cx); JS::RootedValue out(cx);
pCxPrivate->pScriptInterface->ReadJSONFile(filePath, &out); pCxPrivate->pScriptInterface->ReadJSONFile(filePath, &out);
return out; return out;
} }
void JSI_VFS::WriteJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath, JS::HandleValue val1) void JSI_VFS::WriteJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath, JS::HandleValue val1)
{ {
JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); JSContext* cx = pCxPrivate->pScriptInterface->GetContext();
JSAutoRequest rq(cx); JSAutoRequest rq(cx);
// TODO: This is a workaround because we need to pass a MutableHandle to StringifyJSON. // TODO: This is a workaround because we need to pass a MutableHandle to StringifyJSON.
JS::RootedValue val(cx, val1); JS::RootedValue val(cx, val1);
std::string str(pCxPrivate->pScriptInterface->StringifyJSON(&val, false)); std::string str(pCxPrivate->pScriptInterface->StringifyJSON(&val, false));
VfsPath path(filePath); VfsPath path(filePath);
WriteBuffer buf; WriteBuffer buf;
buf.Append(str.c_str(), str.length()); buf.Append(str.c_str(), str.length());
g_VFS->CreateFile(path, buf.Data(), buf.Size()); g_VFS->CreateFile(path, buf.Data(), buf.Size());
} }
void JSI_VFS::RegisterReadOnlyScriptFunctions(const ScriptInterface& scriptInterface) bool JSI_VFS::PathRestrictionMet(ScriptInterface::CxPrivate* pCxPrivate, const std::vector<std::wstring>& validPaths, const std::wstring& filePath)
{
for (const std::wstring& validPath : validPaths)
if (filePath.find(validPath) == 0)
wraitiiUnsubmitted
Not Done
Inline Actions

This should definitely be a starts-with of some kind.

wraitii: This should definitely be a starts-with of some kind.
elexisAuthorUnsubmitted
Not Done
Inline Actions

I won't be using boost

elexis: I won't be using boost
wraitiiUnsubmitted
Not Done
Inline Actions

Right, I think that's either C++17 or C++20, still it should behave like one. Otherwise you can just name your JSON file "whaterversimulation/.json" and it's going to be loadable from anywhere.

wraitii: Right, I think that's either C++17 or C++20, still it should behave like one. Otherwise you can…
wraitiiUnsubmitted
Not Done
Inline Actions

Had misread this as != end() initially, so never mind on the above, we're good to go.

wraitii: Had misread this as != end() initially, so never mind on the above, we're good to go.
elexisAuthorUnsubmitted
Not Done
Inline Actions

np

elexis: np
return true;
std::wstring allowedPaths;
for (std::size_t i = 0; i < validPaths.size(); ++i)
{
if (i != 0)
allowedPaths += L", ";
allowedPaths += L"\"" + validPaths[i] + L"\"";
}
wraitiiUnsubmitted
Not Done
Inline Actions

The above is one of those cases where C++ is pretty ugly…
This might be a really terrible idea, but how about we call a JS function to warn users that it's not allowed?

wraitii: The above is one of those cases where C++ is pretty ugly… This might be a really terrible idea…
elexisAuthorUnsubmitted
Not Done
Inline Actions

Why?

elexis: Why?
wraitiiUnsubmitted
Not Done
Inline Actions

cause then it's like

allowedPaths.map(x=> '"' + x + '"').join(",")
wraitii: cause then it's like ``` allowedPaths.map(x=> '"' + x + '"').join(",") ```
elexisAuthorUnsubmitted
Not Done
Inline Actions

A string concatenation in JS is nicer than in C++.
But doing so here comes with additional costs of introducing a globalscripts function that doesnt fit anywhere and only contains this line and hardcoding JS function name in C++.
If we wouldn't have to allocate a new JS object and receive a less intuitive string, I'd have offered JSON stringify.
In rP19491 we used a stringstream, but that seems less readable (performance doesn't matter here since the program aborts after these microseconds)

elexis: A string concatenation in JS is nicer than in C++. But doing so here comes with additional…
wraitiiUnsubmitted
Not Done
Inline Actions

True. Well it was just a dumb suggestion anyways.

wraitii: True. Well it was just a dumb suggestion anyways.
JS_ReportError(
pCxPrivate->pScriptInterface->GetContext(),
"This part of the engine may only read from: %s!",
utf8_from_wstring(allowedPaths).c_str());
return false;
}
void JSI_VFS::RegisterScriptFunctions_GUI(const ScriptInterface& scriptInterface)
wraitiiUnsubmitted
Not Done
Inline Actions

This function doesn't need to be part of JSI_VFS, move it above ReadJSonFile and it can exist only in this cpp file.

wraitii: This function doesn't need to be part of JSI_VFS, move it above ReadJSonFile and it can exist…
elexisAuthorUnsubmitted
Not Done
Inline Actions

doable

elexis: doable
{ {
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &JSI_VFS::BuildDirEntList>("BuildDirEntList"); scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &JSI_VFS::BuildDirEntList>("BuildDirEntList");
scriptInterface.RegisterFunction<bool, CStrW, JSI_VFS::FileExists>("FileExists"); scriptInterface.RegisterFunction<bool, CStrW, JSI_VFS::FileExists>("FileExists");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_GUI>("ReadJSONFile");
scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &WriteJSONFile>("WriteJSONFile");
scriptInterface.RegisterFunction<double, std::wstring, &JSI_VFS::GetFileMTime>("GetFileMTime"); scriptInterface.RegisterFunction<double, std::wstring, &JSI_VFS::GetFileMTime>("GetFileMTime");
scriptInterface.RegisterFunction<unsigned int, std::wstring, &JSI_VFS::GetFileSize>("GetFileSize"); scriptInterface.RegisterFunction<unsigned int, std::wstring, &JSI_VFS::GetFileSize>("GetFileSize");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFile>("ReadFile"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFile>("ReadFile");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFileLines>("ReadFileLines"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFileLines>("ReadFileLines");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &ReadJSONFile>("ReadJSONFile");
} }
void JSI_VFS::RegisterWriteScriptFunctions(const ScriptInterface& scriptInterface) void JSI_VFS::RegisterScriptFunctions_Simulation(const ScriptInterface& scriptInterface)
{ {
scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &WriteJSONFile>("WriteJSONFile"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Simulation>("ReadJSONFile");
}
void JSI_VFS::RegisterScriptFunctions_Maps(const ScriptInterface& scriptInterface)
{
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &JSI_VFS::BuildDirEntList>("BuildDirEntList");
scriptInterface.RegisterFunction<bool, CStrW, JSI_VFS::FileExists>("FileExists");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Maps>("ReadJSONFile");
elexisAuthorUnsubmitted
Done
Inline Actions

The context dependent functions could be moved to the functions, but the GUI context ScriptFunctions.cpp looks much nicer with only having one line each there.

elexis: The context dependent functions could be moved to the functions, but the GUI context…
} }
Wildfire Games · Phabricator