Page MenuHomeWildfire Games

simulation2: Remove Vector2D/Vector3D prototype workaround from EngineScriptConversions
ClosedPublic

Authored by Krinkle on Jun 16 2019, 7:22 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22487: Remove Vector2D/Vector3D prototype workaround from EngineScriptConversions.
Trac Tickets
#5376
Summary

Follows-up rP14645. Fixes #5376.

Background

This indirection doesn't appear to be necessary. It looks like there may've been
some confusion between the prototype object of a class, vs of an object.
The "prototype of an object" refers to the parent object that a given object
inherits from (retrieved via JS_GetPrototype in cpp, or Object.getPrototypeOf(obj)
in ES5). This should be rare in a codebase that mimics classical inheritance.

The "prototype of a class" is informal concept that refers to the object that
newly constructed instances will be inheriting from. Eg function makeFoo() might
spawn objects that inherit from FooBase (their prototype). The new operator
provides syntax-sugar that connects the constructor function with the parent
object for its instance, with the convention that the base object is stored
at "MyConstructor.prototype". The new operator basically just does
obj = Object.create(Ctor.prototype); Ctor.call(obj);. Bottom line is:
"Ctor.prototype" is the prototype of Ctor objects, not of the "Ctor" function.

Change

Instead of storing "Vector2D.prototype" as its own global variable that we
access, access "Vector2D" instead, and then access "prototype" as property
of that. Also, remove the three manual steps (discovery of prototype property,
creating an object from it, and setting its properties), in favour of letting
SpiderMonkey and the vector.js do this.

Test Plan
  • Compiles without errors in debug and release mode.
  • Run in debug mode, start single player, select some units and move them somewhere.
  • Observe there are no errors in-game or CLI.

Also, to verify that this code really does run with the above steps, I added some debug_printf(),
and indeed it gets regularly called both during the loading of a new game, and when moving units.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Krinkle created this revision.Jun 16 2019, 7:22 PM
Owners added a subscriber: Restricted Owners Package.Jun 16 2019, 7:22 PM
Krinkle added inline comments.Jun 16 2019, 7:23 PM
source/simulation2/scripting/EngineScriptConversions.cpp
187 ↗(On Diff #8521)

I believe this can be simplified even further by using the native JS_Construct or JS_New function. However, I'm still quite new with both CPP and SpiderMonkey and trying to figure out how to make it work. Assuming this works fine and isn't a perf regression, I can do that step next.

Krinkle edited the summary of this revision. (Show Details)Jun 16 2019, 7:39 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  14| »   »   this.x·=·this.y·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1749/display/redirect

Krinkle updated this revision to Diff 8526.Jun 16 2019, 10:01 PM
Krinkle edited the summary of this revision. (Show Details)
Krinkle edited the test plan for this revision. (Show Details)

Switch to JS::Construct, instead of JS_GetProperty, JS_NewObjectWithGivenProto, and JS_SetProperty.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  14| »   »   this.x·=·this.y·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1755/display/redirect

Krinkle added inline comments.Jun 16 2019, 11:34 PM
source/simulation2/scripting/EngineScriptConversions.cpp
199 ↗(On Diff #8526)

:facepalm:

Krinkle updated this revision to Diff 8530.Jun 16 2019, 11:37 PM

Fixed the bug caught by Vector3D roundtrip test. Was accidentally setting y twice, and not z.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  14| »   »   this.x·=·this.y·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1757/display/redirect

This seems to be the cleanest we'll get.

The question as discussed on IRC is "does this have a performance impact" ? I find that unlikely, but this code is probably quite hot.

As I've told you, our games are run deterministically, so as this should not change that, you can profile this easily at the game level. Just run a game, then run the replay of that, non-visually with and without the change and profile using time. If you want to get more involved, I would suggest using Profiler2.

If there is a need for caching prototypes, I would suggest moving the caching in ScriptInterface to a specific function.

source/simulation2/scripting/EngineScriptConversions.cpp
244 ↗(On Diff #8530)

This is basically equivalent to new Vector2D(x, y) right?

Krinkle marked an inline comment as done.Tue, Jun 18, 1:07 AM
Krinkle added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
244 ↗(On Diff #8530)

That's right.

Did you reproduce @Itms steps before the patch?

Also I'd prefer to write a test that checks the case.

source/simulation2/scripting/EngineScriptConversions.cpp
189 ↗(On Diff #8530)

I think that we have to remove such duplication by using templates. Like:

template<typename... Args>
void ConstructJSObject(JSContext* cx, const char* name, JS::MutableHandleValue ret, Args... args)
{
	JSAutoRequest rq(cx);

	ScriptInterface::CxPrivate* pCxPrivate = ScriptInterface::GetScriptInterfaceAndCBData(cx);
	JS::RootedObject global(cx, &pCxPrivate->pScriptInterface->GetGlobalObject().toObject());
	JS::RootedValue value(cx);
	JS_GetProperty(cx, global, name, &value);

	JS::AutoValueArray<sizeof...(Args)> argsArray(cx);
	AutoValueArrayHelper::Fill(cx, argsArray, args...);

	JS::RootedValue obj(cx);
	JS::Construct(cx, value, argsArray, &obj);
	ret.setObject(obj.toObject());
}

template<> void ScriptInterface::ToJSVal<CFixedVector3D>(JSContext* cx, JS::MutableHandleValue ret, const CFixedVector3D& val)
{
    ConstructJSObject(cx, "Vector3D", ret, val.X, val.Y, val.Z);
}

template<> void ScriptInterface::ToJSVal<CFixedVector2D>(JSContext* cx, JS::MutableHandleValue ret, const CFixedVector2D& val)
{
    ConstructJSObject(cx, "Vector2D", ret, val.X, val.Y);
}

// Not full, but just an example.
template<size_t index, size_t N, typename T, typename... Args>
struct AutoValueArrayHelper
{
	static void Fill(JSContext* cx, JS::AutoValueArray<N>& argsArray, T value)
	{
		JS::RootedValue rootedValue(cx);
		ScriptInterface::ToJSVal(cx, &rootedValue, value);
		argsArray[index].set(rootedValue);

		AutoValueArrayHelper::Fill<index + 1, N, Args>(cx, argsArray, args...);
	}
};
Krinkle marked an inline comment as done.Sat, Jun 22, 2:41 AM
Krinkle added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
189 ↗(On Diff #8530)

Yeah, definitely seems worth abstracting a little for re-use.

But, I have stretched myself quite a bit to even get this patch to work. I've never done anything in C++ before, it's only by imitation and trial-error that I got this to work in its current form.

I can't currently even write a new function without copying something else nearby. I should probably focus on my JS patches more where I can be more effective.

Do you want to add this re-use block in a follow-up commit and/or take over my patch and add it here?

wraitii added inline comments.Sat, Jun 22, 8:15 AM
source/simulation2/scripting/EngineScriptConversions.cpp
189 ↗(On Diff #8530)

Good idea but perhaps out of scope? This would only be used in two closely related functions, so adding an abstraction is almost as long as not.

The autvaluearray abstraction could be more useful I guess, but that's definitely another patch imo.

source/simulation2/scripting/EngineScriptConversions.cpp
189 ↗(On Diff #8530)

I mean if we add a duplication why not to replace it with the same size of code but more flexible?

Here's a simpler version (it has exactly the same length as the duplication one):

template<size_t index, size_t N, typename T>
void SetAutoValueArrayElement(JSContext* cx, JS::AutoValueArray<N>& argsArray, T value)
{
	JS::RootedValue rootedValue(cx);
	ScriptInterface::ToJSVal(cx, &rootedValue, value);
	argsArray[index].set(rootedValue);
}

template<typename T>
void FillAutoValueArray(JSContext* cx, JS::AutoValueArray<3>& argsArray, T val1, T val2, T val3)
{
	SetAutoValueArrayElement<0>(cx, argsArray, val1);
	SetAutoValueArrayElement<1>(cx, argsArray, val2);
	SetAutoValueArrayElement<2>(cx, argsArray, val3);
}

template<typename T>
void FillAutoValueArray(JSContext* cx, JS::AutoValueArray<2>& argsArray, T val1, T val2)
{
	SetAutoValueArrayElement<0>(cx, argsArray, val1);
	SetAutoValueArrayElement<1>(cx, argsArray, val2);
}

template<typename... Args>
void ConstructJSObject(JSContext* cx, const char* name, JS::MutableHandleValue ret, Args... args)
{
	JSAutoRequest rq(cx);

	ScriptInterface::CxPrivate* pCxPrivate = ScriptInterface::GetScriptInterfaceAndCBData(cx);
	JS::RootedObject global(cx, &pCxPrivate->pScriptInterface->GetGlobalObject().toObject());
	JS::RootedValue value(cx);
	JS_GetProperty(cx, global, name, &value);

	JS::AutoValueArray<sizeof...(args)> argsArray(cx);
	FillAutoValueArray(cx, argsArray, args...);

	JS::RootedValue obj(cx);
	JS::Construct(cx, value, argsArray, &obj);
	ret.setObject(obj.toObject());
}

template<> void ScriptInterface::ToJSVal<CFixedVector3D>(JSContext* cx, JS::MutableHandleValue ret, const CFixedVector3D& val)
{
	ConstructJSObject(cx, "Vector3D", ret, val.X, val.Y, val.Z);
}

template<> void ScriptInterface::ToJSVal<CFixedVector3D>(JSContext* cx, JS::MutableHandleValue ret, const CFixedVector3D& val)
{
	ConstructJSObject(cx, "Vector2D", ret, val.X, val.Y);
}

Did you reproduce @Itms steps before the patch?

Yes. Test plan I wrote in the commit myself includes those steps, and I've confirmed them locally as well before submitting. The game compiles, starts, and can create a game and select/move units without errors.

Also I'd prefer to write a test that checks the case.

Sounds good. I'll need a pointer or two for where a test like this would go, and how I get an instance of the necessary objects. I'm very new to C++. Feel free to commandeer if you wish, I have other patches I can be more effective with. I won't mind :)

wraitii added inline comments.Mon, Jun 24, 11:35 AM
source/simulation2/scripting/EngineScriptConversions.cpp
189 ↗(On Diff #8530)

I mean if we add a duplication why not to replace it with the same size of code but more flexible?

I suppose the argument would be that templated code is harder to read and harder to grasp in general than the simple straighforward variant here. Variadic templates get us somewhat into the dark magic territory of C++.

Where's there's not a need to resort to more complex C++ function , I don't think we necessarily need to.

Furthermore, here the patch owner isn't super knowledgeable about C++, so what I would recommend is just uploading that as a separate diff. Feel free to handle this however you want.

Yes. Test plan I wrote in the commit myself includes those steps, and I've confirmed them locally as well before submitting. The game compiles, starts, and can create a game and select/move units without errors.

Did you test (de)serialising?

Krinkle marked 3 inline comments as done.Mon, Jun 24, 8:58 PM
Krinkle added a subscriber: elexis.

Yes. Test plan I wrote in the commit myself includes those steps, and I've confirmed them locally as well before submitting. The game compiles, starts, and can create a game and select/move units without errors.

Did you test (de)serialising?

I'm not sure exactly what that means in the specific context of 0AD and EngineScriptConversions.

From the ticket description (#5376), there was no mention of a specific user action that triggers serialisation in the steps to reproduce the issue. Based on the first comment from @elexis on that ticket, I assumed serialising/deserialising referred to the internal triggering of the FromJSVal and ToJSVal code, which happens as part of the reproduction steps I described (selecting units and moving them).

When I added print statements to these blocks, I confirmed that the modified code is indeed run, and the game worked as expected without any errors.

Are you and/or was @elexis referring to something else I can test? If so, how do I test that?

Thanks :)

Oh, I'm referring to saving and loading :)
That causes problems with Vector3D in D1971.

elexis accepted this revision.Mon, Jul 15, 4:10 AM

Yes. Test plan I wrote in the commit myself includes those steps, and I've confirmed them locally as well before submitting. The game compiles, starts, and can create a game and select/move units without errors.

This function is tested definitely by just walking around with units, since there are many JS <-> C++ conversions for entity positions. Making a savestate and loading that may also be an idea, although the previous test should be sufficient already.

Also I'd prefer to write a test that checks the case.

Sounds good. I'll need a pointer or two for where a test like this would go

There is a test_ScriptConversions.h file already with test_vector2d() and test_vector3d() which converts from C++ to JS and back from JS to C++ and tests equality.

Fixed the bug caught by Vector3D roundtrip test. Was accidentally setting y twice, and not z.

Oh, I'm referring to saving and loading :)
That causes problems with Vector3D in D1971.

Saving/loading should be tested, but actually vector prototype serialization is not implemented #4698 (it de/serializes X, Y, but not the prototype property), so not much that might go wrong that hasnt gone wrong elsewhere.

I must accept the patch because I couldn't find any errors. I encourage you to remove the few lines that can be simplified.

source/simulation2/scripting/EngineScriptConversions.cpp
189 ↗(On Diff #8530)

I attempted this deduplication once too and abandoned because it looked terrible, I tried with macros though.
Well I can make it 7 lines shorter :P
But also, the deduplication won't be applicable to the future unless we will implement some 4D breaking technology.
So looking back at this again, to me it seems like deduplication is the first thought when reading the duplication. But actually some things like JS::Rooted have to be repeated when working with this, it's not so unusual, the benefit is little and comes with the cost of some indirection.
I'm not against it if you want to change to that, but I'm also not necessarily jumping the bandwagon, the current diff is acceptable to me.
(Also notice we the FromJSVal functions are duplicated too. If going for that, it would be more transparent in a new file.)

237 ↗(On Diff #8530)

Can remove the four calls above by going for
args[0].setNumber(val.X.ToDouble());

(The ToJSVal<fixed> is few lines above, so noone can say that one needs to hide the Double type to make it more failsafe IMO.)

244 ↗(On Diff #8530)

Wondering if it should check whether JS_GetProperty and JS::Construct succeeded or failed or whether thats a waste of performance as they always suceed or the game is fundamantelly broken.

246 ↗(On Diff #8530)

(Apparently there is no .setValue)

187 ↗(On Diff #8521)

JS_New seems to be more consistent, there are 4 calls in the codebase, most notably in ScriptInterface::CallConstructor which is called from CComponentManager::ConstructComponent.

JS_New also seems to be closer to the currentcodebase since it doesnt use the classes flavor at all, neither JS nor C++ side.

JS::Construct calls a specified function as a constructor, fun.

(SM and their supposed fun)

JS::Construct actually calls InvokeConstructor whereas JS_New calls JS_NewHelper which does some stuff plus InvokeConstructor, so I suppose JS::Construct is actually faster?!

(both defined at libraries/source/spidermonkey/mozjs-38.0.0/js/src/jsapi.cpp)

This revision is now accepted and ready to land.Mon, Jul 15, 4:10 AM
Krinkle added inline comments.Mon, Jul 15, 4:24 AM
source/simulation2/scripting/EngineScriptConversions.cpp
237 ↗(On Diff #8530)

Interesting. That would be shorter indeed. (Reminder, I've never worked with CPP or SpiderMonkey before last month.)

I'm seeing JS::AutoValueArray and JS::Value and I understand how it would work.

Now, I am actually wondering where the documentation of the currently-used .val() is, I don't see it.. I must've stumbled across it somewhere and forced it to work (which I what I did).

244 ↗(On Diff #8530)

Yeah, seems like a case for a (unit) test rather than a run-time check. Haven't yet figured out how the Cpp tests work, perhaps for a future commit?

187 ↗(On Diff #8521)

Indeed, JS::Construct will be marginally faster and more direct. Emulating all fancy features that the new operator is capable of isn't really what we're after.

Krinkle marked 2 inline comments as done and 2 inline comments as done.Mon, Jul 15, 4:30 AM
Krinkle added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
237 ↗(On Diff #8530)

(I meant .set() not .val())

elexis added inline comments.Mon, Jul 15, 12:58 PM
source/simulation2/scripting/EngineScriptConversions.cpp
103 ↗(On Diff #8530)

e.g. safeguard here

244 ↗(On Diff #8530)

couldn't find any errors
Wondering if it should check whether JS_GetProperty and JS::Construct succeeded or failed or whether thats a waste of performance as they always suceed or the game is fundamantelly broken.

(Strictly speaking that might be an error as the other functions do check for GetProperty results)

Krinkle marked an inline comment as not done.Tue, Jul 16, 8:46 PM
Krinkle updated this revision to Diff 8939.EditedTue, Jul 16, 10:34 PM
Krinkle marked 2 inline comments as done.

Switched from long-form RootedValue/ToJSVal, to the shorter JS::Value#setNumber() form instead, per @elexis suggestion.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  14| »   »   this.x·=·this.y·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/104/display/redirect

Krinkle updated this revision to Diff 8940.Tue, Jul 16, 11:09 PM
Krinkle marked 2 inline comments as done.

Add failure checks for JS_GetProperty and JS::Construct. Also removed the redundant ret.setObject/obj.getObject indirection in favour of letting JS::Construct populate ret directly. Seems to work, and the type hints match. Don't know this is "right", though.

source/simulation2/scripting/EngineScriptConversions.cpp
227 ↗(On Diff #8940)

Created a FAILVOID because FAIL results in the void ToJSVal returning a bool, which the compiler doesn't like.

Krinkle marked 2 inline comments as not done.Tue, Jul 16, 11:10 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  14| »   »   this.x·=·this.y·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/105/display/redirect

elexis accepted this revision.Tue, Jul 16, 11:47 PM

Thanks so much for the patch Krinkle!

To me the code looks much shorter, not sure if the deduplication approach will be benefitial, it will depend on implementation (perhaps a macro would be considerable).
I would propose to make consider this independently. If the duplication is annoying, it will haunt us and it will be revisited.

source/scriptinterface/ScriptInterface.cpp
458 ↗(On Diff #8940)

Delete it, yes! yeeeeees!

483 ↗(On Diff #8940)

(Very satisfying to remove that)

source/simulation2/scripting/EngineScriptConversions.cpp
184 ↗(On Diff #8940)

(comment a bit pointles)

234 ↗(On Diff #8940)

nice avoiding of the ret.setObject call!

Krinkle updated this revision to Diff 8942.Tue, Jul 16, 11:52 PM
Krinkle marked an inline comment as done.

Removed not so useful comment.

source/simulation2/scripting/EngineScriptConversions.cpp
184 ↗(On Diff #8940)

Agreed. Didn't want to say anything :)

This revision was landed with ongoing or failed builds.Tue, Jul 16, 11:52 PM
This revision was automatically updated to reflect the committed changes.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  14| »   »   this.x·=·this.y·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.

binaries/data/mods/_test.sim/globalscripts/test-global-helper.js
|  33| »   »   this.x·=·this.y·=·this.z·=·0;
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/107/display/redirect