Page MenuHomeWildfire Games
Differential D2644

Support JS class syntax / non-enumerable entity component message handlers
ClosedPublic
Actions

Authored by elexis on Feb 26 2020, 4:55 PM.

Details

Reviewers
Itms
Commits
rP23771: Rework ScriptInterface EnumeratePropertyNames to support non-enumerable…
Summary

In rP23514/D2492, there was the entity component Autobuildable committed where all prototype methods except the message value handlers were specified using the class syntax keyword.
See comments on http://irclogs.wildfiregames.com/2020-02/2020-02-26-QuakeNet-%230ad-dev.log
The specs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes don't seem to mention it,
but https://javascript.info/class does ("Class methods are non-enumerable"), and
an Object.defineProperty(AutoBuildable.prototype, 'OnValueModification', {enumerable: true}); does confirm
that the prototype methods specified using class syntax are not enumerable (contrary to the prototype syntax).

Since Script_RegisterComponentType_Common iterates only through enumerable properties, it won't find these handlers and thus not subscribe them.
This can be fixed by iterating through the known handler names and probing if they exist.

Test Plan

Notice there aren't a ton of (no) message errors when starting the game with the patch applied.
Add a warn to these particular message handlers to confirm theyre called with the patch but not without.
Notice that global and local message handlers are called.
Read through the code and comprehend what it does, compare it with the previous code and see that it does the same except for the enumerability part.
Notice that all message types are registered in the interfaces/ folder and that that is evaluated before the entity component scripts,
therefore iterating through known event handler names is correct.
Search other instances where the entity component code iterates through enumerable properties or disregards non-enumerable properties.
Notice that it works for "Init" already. Check that it works for "Serialize" and "Deserialize" too for instance.

Diff Detail

Event Timeline

elexis created this revision.Feb 26 2020, 4:55 PM
Vulcan added a comment.Feb 26 2020, 4:55 PM

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

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

Harbormaster failed remote builds in B11199: Diff 11433!Feb 26 2020, 4:55 PM
elexis edited the test plan for this revision. (Show Details)Feb 26 2020, 4:59 PM
Vulcan added a comment.Feb 26 2020, 5:00 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1300/display/redirect

Vulcan added a comment.Feb 26 2020, 5:02 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/396/display/redirect

Stan added a reviewer: Itms.Feb 26 2020, 8:40 PM
elexis updated this revision to Diff 11441.Feb 27 2020, 6:31 PM

Use JS_HasProperty instead of JS_HasOwnProperty.

Remove warning about OnFoo script function when there is no MessageType Foo registered.

Notice the warning only happened for enumerable properties.
So if someone declares a non-enumerable OnFoo script function (for instance using the class syntax),
then this warning would not occur.

If the idea is to use class keyword for the simulation from now on (is it?),
then the warning would basically not protect devs anymore from spelling errors.

If one can iterate over non-enumerable properties, something analog to JS_Enumerate, then one can lessen the changes in this diff and keep the warning as is.
I don't find such a function in the specs nor jsapi.h.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_Enumerate

Vulcan added a comment.Feb 27 2020, 6:36 PM

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

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

Harbormaster failed remote builds in B11207: Diff 11441!Feb 27 2020, 6:36 PM
Freagarach added a subscriber: Freagarach.Feb 27 2020, 8:07 PM

Tested (also with D1960) and it works :)
Just one question, why is the Schema moved?

Works; i.e.:

  • No error messages.
  • Calling confirmed.
  • (Can't comment on the C++-code.)
elexis added a comment.Feb 27 2020, 8:25 PM

why is the Schema moved?

I suppose that's out of scope, but I would recommend against get syntax when there is the choice.
If we call a function, it should be visible as such.
If we actually want to have a prototype property, then it should be expressed as such.
Storing literals in the prototype is a use case, for example UnitAI globals, or
BuildingAI.prototype.MAX_PREFERENCE_BONUS = 2;
And IMO it would be ugly to add a function call and making it impossible to assign a value to that property name.
(Its possible to have one value on a given property name on the prototype and another value on the same name on an instance of the prototype, so the prototype value can be used as a fallback.)
Especially if there were many literal values stored in the prototype, having a get function for each would accumulate function ugliness quickly. (If we want an assignment X = Y; then why make it a function. Just for the single class scope shouldn't be worth it if it simultaneosly has these other effects on code, possibly even performance due to the +1 function call)

source/simulation2/system/ComponentManager.cpp
285

Could use ScriptInterface::HasProperty, but that seems like adding instructions for no benefit, also has no error reporting (which then would rather mean that that wrapper is perhaps unneeded)

Freagarach added a comment.Apr 10 2020, 8:11 AM

It would be nice to have this kind of information in the CC, especially for the "new" class syntax ;) (I'll see what I can do about that.)
I guess another advantage of literals in the prototype is that it is easier for modders to overwrite just that value, don't know. At least it seems to be more logical ^^

wraitii added a subscriber: wraitii.EditedMay 23 2020, 6:58 PM

It seems you could have kept the enumeration-like behaviour with a combination of the (undocumented, but still present in SM[latest]), from the jsfriendapi.h:

JS::AutoIdVector vec(m->m_cx);
JSID_IS_STRING()
JSID_TO_STRING()
js::GetPropertyKeys(m->m_cx, obj, 0, &vec);

See implementation of js::GetOwnPropertyKeys in [spidermonkey]/js/src/builtin/Object.cpp.
That being said, the code as written looks pretty clean.
The other user of this function is in XmppClient (why is there JS there?), but that doesn't use the prefix logic and it seems the enumerability of only enumerable properties should be a feature. It seems plausible that we should do something custom here. Performance when registering a component isn't really a concern.

I would say either:

  • delete the "prefix" bit from our enumerate wrapper in ScriptInterface and ship this.
  • delete the "prefix" bit from our enumerate wrapper, add a "enumerableOnly" flag, use the above method in componentManager and filter the props there.
wraitii mentioned this in D2367: Move the ability to hold a turret to a separate file..Jun 7 2020, 9:48 AM
wraitii updated this revision to Diff 12193.Jun 7 2020, 10:38 AM

(Not commandeering as this is still largely the same patch.)

This uses GetPropertyKeys in the scriptinterface instead. It's a neater API for what we want since it recurses up the prototype chain on its own.
I'm removing the prefix logic since that hardly seems worth it.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jun 7 2020, 10:38 AM
Vulcan added a comment.Jun 7 2020, 11:26 AM

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

Linter detected issues:
Executing section Source...

source/scriptinterface/ScriptInterface.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"
Executing section JS...
|    | [NORMAL] ESLintBear (comma-spacing):
|    | There should be no space before ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|   2|   2| {
|   3|   3| 	Init()
|   4|   4| 	{
|   5|    |-		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate , this.entity);
|    |   5|+		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate, this.entity);
|   6|   6| 		if (this.rate)
|   7|   7| 			this.StartTimer();
|   8|   8| 	}
|    | [NORMAL] ESLintBear (comma-spacing):
|    | There should be no space before ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|  17|  17| 
|  18|  18| 	UpdateRate()
|  19|  19| 	{
|  20|    |-		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate , this.entity);
|    |  20|+		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate, this.entity);
|  21|  21| 
|  22|  22| 		if (this.rate)
|  23|  23| 			this.StartTimer();
Executing section cli...

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

Harbormaster completed remote builds in B11993: Diff 12193.Jun 7 2020, 11:27 AM
wraitii updated this revision to Diff 12298.Jun 14 2020, 10:57 AM

Remove debug template before merging.

Vulcan added a comment.Jun 14 2020, 11:44 AM

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

Linter detected issues:
Executing section Source...

source/scriptinterface/ScriptInterface.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"
Executing section JS...
|    | [NORMAL] ESLintBear (comma-spacing):
|    | There should be no space before ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|   2|   2| {
|   3|   3| 	Init()
|   4|   4| 	{
|   5|    |-		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate , this.entity);
|    |   5|+		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate, this.entity);
|   6|   6| 		if (this.rate)
|   7|   7| 			this.StartTimer();
|   8|   8| 	}
|    | [NORMAL] ESLintBear (comma-spacing):
|    | There should be no space before ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/AutoBuildable.js
|  17|  17| 
|  18|  18| 	UpdateRate()
|  19|  19| 	{
|  20|    |-		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate , this.entity);
|    |  20|+		this.rate = ApplyValueModificationsToEntity("AutoBuildable/Rate", +this.template.Rate, this.entity);
|  21|  21| 
|  22|  22| 		if (this.rate)
|  23|  23| 			this.StartTimer();
Executing section cli...

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

Harbormaster completed remote builds in B12091: Diff 12298.Jun 14 2020, 11:44 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2020, 11:49 AM
Closed by commit rP23771: Rework ScriptInterface EnumeratePropertyNames to support non-enumerable… (authored by wraitii). · Explain Why
This revision was automatically updated to reflect the committed changes.
wraitii added a commit: rP23771: Rework ScriptInterface EnumeratePropertyNames to support non-enumerable….

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
simulation/
components/
44 lines
templates/
structures/
4 lines
source/
lobby/
4 lines
scriptinterface/
10 lines
54 lines
simulation2/
system/
10 lines

Diff 12193

View Options

binaries/data/mods/public/simulation/components/AutoBuildable.js

Loading...
View Options

binaries/data/mods/public/simulation/templates/structures/brit_house.xml

Loading...
View Options

source/lobby/XmppClient.cpp

Loading...
View Options

source/scriptinterface/ScriptInterface.h

Loading...
View Options

source/scriptinterface/ScriptInterface.cpp

Loading...
View Options

source/simulation2/system/ComponentManager.cpp

Loading...
Wildfire Games · Phabricator