Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
to this:
I tried this simple option:
but that broke some cursor UTs, presumably because of they use Vector3s, for which it's also the case that
instanceof Object
is true, butisObject
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:
position
androtation
use a single property schema, with a Vector3, and they clearly work fine.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 insrc/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?