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.

Sign up for GitHub

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

Jump to bottom

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

Closed
diarmidmackenzie opened this issue Feb 14, 2023 · 3 comments · Fixed by #5474 · May be fixed by #5243
Closed

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

diarmidmackenzie opened this issue Feb 14, 2023 · 3 comments · Fixed by #5474 · May be fixed by #5243

Comments

@diarmidmackenzie
Copy link
Contributor

Description:

Working version using A-Frame 0.8.0 here:
https://glitch.com/edit/#!/single-property-schema-array?path=index.html%3A8%3A23

Found when updating superframe emplate examples to 1.4.1.

Linked Glitch is a very simple example which uses a single property schema, where the property is an array, and outputs the data to screen via the 'text' property.

This works OK at 0.8.0. From 1.0.0, including at 1.4.1, this no longer works.

This branch adds a Unit Test that exhibits the problem, but I haven't figured out a good fix for this yet.

Diagnosis of the problem so far:

The component does not get classified as isObjectBased (I don't know for sure whether that's correct or not) because this expression is not true:

isObject(parseProperty(undefined, this.schema))

here

Then, in updateCachedAttrValue, this.attrValue gets set up correctly, but the array entries it points to have been copied in by reference, not value, and so get changed to undefined on the call to clearObject

The relevant code is here

A puzzling earlier line of code is the use of value instanceof Object here, rather than using isObject.

For an array, isObject is false, but value instanceof Object is true, because an Array is an instance of an Object.
@diarmidmackenzie diarmidmackenzie changed the title Single property schema where the property is an array does not work (broken aince 1.0.0 Single property schema where the property is an array does not work (broken since 1.0.0) Feb 14, 2023
@vincentfretin
Copy link
Contributor

This seems indeed to be a long standing issue. I didn't know it worked before. I came across aframe-layers-component from 2020 that have a comment about the issue.

@diarmidmackenzie
Copy link
Contributor Author

Looks like it dates back to this change, which introduced all the code i have commented on & went into 0.9.0.
#3772

I haven't been able to test with 0.9.0 because it didn't work at all - a WebXR issue.

device.js:5 Uncaught TypeError: navigator.xr.requestDevice is not a function
    at Object.<anonymous> (device.js:5:16)
    at 176._process (device.js:141:33)
    at o (_prelude.js:1:1)
    at _prelude.js:1:1
    at 179../bind (index.js:6:14)
    at o (_prelude.js:1:1)
    at _prelude.js:1:1
    at 152.../package (index.js:25:13)
    at o (_prelude.js:1:1)
    at r (_prelude.js:1:1)

But I presume it would be broken at 0.9.0.

The unit test that I thought repro'd the problem doesn't actually work (it fails for other reasons).

Also, this only seems to be broken when attributes are set prior to entity creation.

E.g. el.setAttribute("test-component", "a, b, c") in the console works fine.

However doing this in the console does exhibit the problem:

el = document.createElement('a-entity')
el.setAttribute("test-component", "a, b, c")
el.setAttribute("position", "0 1.5 -2")
document.querySelector('a-scene').appendChild(el)

and you can then fix with this command:

el.setAttribute("test-component", "a, b, c")

I'll try to do an updated UT based on this repro.

diarmidmackenzie added a commit to diarmidmackenzie/aframe that referenced this issue Feb 14, 2023
@diarmidmackenzie
Unit tests for aframevr#5242
a925fca
diarmidmackenzie added a commit to diarmidmackenzie/aframe that referenced this issue Feb 14, 2023
@diarmidmackenzie
Fix for aframevr#5242
10ce4c4
@diarmidmackenzie diarmidmackenzie mentioned this issue Feb 14, 2023
Draft
@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Feb 14, 2023 •
edited

#5243 offers a fix for this, which seems to work - but there's a few things I don't understand well enough to have confidence in the fix.

@mrxz mrxz mentioned this issue Feb 29, 2024
Merged
mrxz pushed a commit to mrxz/aframe that referenced this issue Mar 23, 2024
@diarmidmackenzie @mrxz
Unit tests for aframevr#5242
747fa17
dmarcos pushed a commit that referenced this issue Mar 24, 2024
@mrxz @diarmidmackenzie
Added unit tests covering previously inconsistent component update be…
81f9626 
…haviour (#5500)

* Added unit tests covering previously inconsistent component update behaviour

* Unit tests for #5242

---------

Co-authored-by: Noeri Huisman <mrxz@users.noreply.github.com>
Co-authored-by: diarmidmackenzie <diarmid.mackenzie@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@vincentfretin @diarmidmackenzie