Remove Vector C++/JS prototype workaround from r14645

[Itms]

In r14645, a workaround was put at the end of vector.js​ to make the prototype available in C++.

The corresponding C++ code is in ScriptInterface.cpp​. Vector2/3D.prototype is retrieved from JS, stored in a value called Vector2/3Dprototype and stored in C++ for future creations of instances of Vector2/3D. The correct way to do this would be to retrieve Vector2/3D and use JS_GetPrototype for the same purpose.

The issue

However, that doesn't seem to work. When moving units, for instance, errors pop along the line of "angleTo is not a function", showing that the methods of Vector2D are not present. But the units correctly move: the attributes of the object are here.

Upgrading to SM45 also makes the issue apparent: the workaround breaks with the update, and has to be adapted. Using JS_GetPrototype doesn't work with SM45 either.

My personal understanding is that, by asking the value of Vector2/3D from the C++ side, one does not get an object with a full prototype for some reason. So one has to get the prototype on the JS side, make a new object from it, and send the object to C++. I do not know why this works and the above doesn't.

How to reproduce (on SM38 or SM45)

The easiest way to reproduce is to not use the workaround: in C++, use the patch

-if (JS_GetProperty(m->m_cx, global, "Vector2Dprototype", &proto))
+if (JS_GetProperty(m->m_cx, global, "Vector2D.prototype", &proto))

to access directly the prototype instead of using the workaround. Then try to move some units.

How to fix

All my attempts at accessing the prototype in a meaningful way created the same issue as described above.

Alternatively, we could just define this prototype in C++ and make it available in JS (just like we do with GUI objects), but that would be acceptable only if vectors don't need to be moddable in any way (I think it is the case).

[elexis]

See also #4698 and #5198.

define this prototype in C++ and make it available in JS (just like we do with GUI objects), but that would be acceptable only if vectors don't need to be moddable in any way (I think it is the case).

The JS GUI Objects are interfaces to C++ class instances. The Vector is just X/Y and some methods, nothing linked to a C++ class.

Having to rewrite the entire prototype in C++ sounds sad, as we often add new Vector2D / Vector3D methods and will continue to do so, the more we use the prototype in the GUI, simulation and rmgen.

But I agree that it would yield cleaner C++ code to define the JSClass in C++.

(Another workaround would be calling new Vector2D(x, y), but we don't want those anyhow.)

To me it sounds like the JS <-> C++ conversion isn't the issue, but the serialization and deserialization. The serialization only recognizes that it is an object, but does not store the prototype, so the deserializer doesn't apply the prototype.

[elexis]

I could run several SP and MP matches without issues, including rejoin, but in at least one case it did not work, same errors as you mentioned.

That invalidates my serialization hypothesis (if I recall correctly the serialization issue is circumvented by not serializing vectors in JS at all).

Actually JS_GetPrototype does work, but if used on an instance of a Vector2D. So for example declaring this.myVector2D = new Vector2D(0, 0); and then getting the prototype of myVector2D. Still a mess of course.

[Itms]

By applying the patch above, and starting a SP game on Acropolis Bay (2) against an AI, I can also trigger the errors just like in my other experiments (if you want reproduction steps).

Replying to elexis:

Actually JS_GetPrototype does work, but if used on an instance of a Vector2D. So for example declaring this.myVector2D = new Vector2D(0, 0); and then getting the prototype of myVector2D. Still a mess of course.

Yes, I agree: Vector2D is not an instance of the class, so asking for its prototype is not supposed to work. It is not a property of the global object either, which is why it breaks with the SM update, I believe.

[Itms]

This piece of code is duplicated in test-global-helper.js​, which is, in my opinion, a good argument in favor of moving this to the engine.

If we want to keep this code in the JS and remove duplication, then it would be necessary to fix #4427 first.

[Krinkle]

Replying to elexis:

Actually JS_GetPrototype does work, but if used on an instance of a Vector2D.

Yeah, Vector2D here is a function, which as JS object is an instance of Function and has the built-in Function.prototype as its object prototype.

From the language and engine perspective there is no relation between Vector2D and Vector2D.prototype beyond the fact that it is stored as a property named "prototype" on the Vector2D function object. This can be counter-intuitive, but really the logic for constructing an object with the new keyword is nothing more than getting the given function's object property named "prototype", create a plain object that inherits from this, and invoke the given function with the new object as its ThisContext.

I'm relatively new to SpiderMonkey and CPP, but to get it one would need to do something like JS_GetProperty(… valueVector2D , "prototype").

Alternatively if we know the constructor args, something like JS::Construct would be simpler and allows this step to be bypassed (letting SM do it). That might also perform better, but don't know for sure.

[elexis]

moving this to the engine

Actually it's not obvious that using C++ defined vectors in JS might not worsen performance.

use JS_GetPrototype

Notice that the previous code created an object with a given prototype, whereas Krinkle's patch creates an object with Vector2D as the constructor, so the latter seems more elegant. Nice!

[elexis]

In 22487:

Remove Vector2D/Vector3D prototype workaround from EngineScriptConversions.

Fixes #5376, refs #2394.
Differential Revision: ​https://code.wildfiregames.com/D1991
Patch By: Krinkle
Comments By: Vladislav, wraitii