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

Fix shared array reference #2615

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Fix shared array reference #2615

merged 2 commits into from
Apr 25, 2017

Conversation

takahirox
Copy link
Collaborator

Description:

This PR fixes the issue "Array property type default share the same reference" #2554

Changes proposed:

  • clone default value with .slice() if it's an array in parseProperty() of schema.js

@takahirox
Copy link
Collaborator Author

takahirox commented Apr 24, 2017

Ah, wait.
Maybe I think I should've done that in arrayParse() of propertyTypes.js.
I'll update...

@@ -138,6 +138,7 @@ function parseProperty (value, propDefinition) {
// Use default value if value is falsy.
if (value === undefined || value === null || value === '') {
value = propDefinition.default;
if (Array.isArray(value)) { value = value.slice(); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do something similar for objects as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by objects? The only types we have that can receive an object are vec2, vec3 and vec4 and those are already copied when parsed: https://github.com/aframevr/aframe/blob/master/src/utils/coordinates.js#L17

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case I define my own property type and set the default value as an object. I often create "schemaless" components that just accept flexible data.

default: {},
parse: function (val) {
  // ... Parse to object or pass through object.
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you do that is the parse function that should make sure that the object is copied.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying objects is inefficient and should be a deliberate decision. I think sharing objects is a more sensible default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but your custom parse function can copy the objects

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more of a weird workaround. It's not really parsing, I'd have to run a check if it's exactly the default object to figure to copy it, and I only need it to be done once if it's using the default value and not on every parse. I propose to handle it because the case is unexpected.

The object copy won't be hit that often if we handle the logic here. Only once per property initialization, and only if it's usually the default value. Plus, it's usually an empty object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an object type in the property types that is shallow? It allows just one level where the attributes are primitive types based on the default value? Should we deal with this in a separate issue / PR?

Copy link
Member

@ngokevin ngokevin Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have a need for that. I might have a component that generates entities. Or I sometimes use it to create schemaless components. One level is often limiting. I just the component to hold whatever data I have and pass it on.

AFRAME.registerComponent('creator', {
  schema: {
    geometry: myCustomObjPropertyType,
    material: myCustomObjPropertyType
  }
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will merge though and figure it out later.

@ngokevin ngokevin merged commit f3789b0 into aframevr:master Apr 25, 2017
@takahirox takahirox deleted the FixSharedArrayReference branch April 25, 2017 11:31
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.

3 participants