Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single property schema array #5243

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

diarmidmackenzie
Copy link
Contributor

Description:

Small change to fix #5242 and allow an array to be used as the property in a single property schema.
Unit tests for this.

Changes proposed:

I don't have much confidence in this fix, so needs review by someone who understands the code better. It may be that an altogether different approach is needed.

Following up on my previous analysis here, I experimented with various changes.

What seemed to work best was skipping the object pool altogether for this array property, by updating this test:

if ((value instanceof Object) && !(value instanceof window.HTMLElement))

to this:

if ((value instanceof Object) &&
        !(value instanceof Array) &&
        !(value instanceof window.HTMLElement))

I tried this simple option:

if (this.isObjectBased && !(value instanceof window.HTMLElement))

but that broke some cursor UTs, presumably because of they use Vector3s, for which it's also the case that instanceof Object is true, but isObject is false... I guess we don't want to make any changes for Vector3s, just arrays...?

A build including this fix does resolve the problem.
https://glitch.com/edit/#!/single-property-schema-array-fix?path=index.html%3A8%3A23

There's still lots I don't understand about the code I have modified, exactly when it is invoked, and what it should do.

Things I don't understand:

  • Why does the problem we have with Arrays not also happen for Vector3s? Components like position and rotation use a single property schema, with a Vector3, and they clearly work fine.
  • Why did we only have a problem on entity creation, and not on subsequent setAttribute() calls?

Regarding the new UTs...

I have reasonable confidence that the 2 x new UTs are valid, but not sure they belong in the component suite: although they are testing code in src/core/component they are more like end-to-end tests, rather than unit tests of this module.

Maybe there's a better place for these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single property schema where the property is an array does not work (broken since 1.0.0)
1 participant