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

Make `setObject3D` bind `el` inside each child of `obj` when it is a `THREE.Group` (fixes #1403) #1497

Merged
merged 13 commits into from May 26, 2016

Conversation

Projects
None yet
3 participants
@msimpson
Contributor

msimpson commented May 24, 2016

While this fixed #1403 for me, it needs review. I have no idea what else this could effect, regardless of the existing tests passing.

this.object3D.add(obj);
}
if (obj instanceof THREE.Group && obj.children.length) {
obj.traverse(function (child) {

This comment has been minimized.

@ngokevin

ngokevin May 24, 2016

Member

Can name this function bindEl

@ngokevin

This comment has been minimized.

Member

ngokevin commented May 24, 2016

@dmarcos Looks good to me. The alternative is to have the raycaster traverse up, but that's not performant.

@ngokevin ngokevin changed the title from Make `setObject3D` bind `el` inside each child of `obj` when it is a `THREE.Group` to Make `setObject3D` bind `el` inside each child of `obj` when it is a `THREE.Group` (fixes #1403) May 24, 2016

@@ -153,7 +153,7 @@ var proto = Object.create(ANode.prototype, {
this.object3D.add(obj);
}
if (obj instanceof THREE.Group && obj.children.length) {
obj.traverse(function (child) {
obj.traverse(function bindEl (child) {

This comment has been minimized.

@msimpson

msimpson May 24, 2016

Contributor

For debugging, I assume.

This comment has been minimized.

@ngokevin

ngokevin May 24, 2016

Member

Just to help readability by providing more information.

This comment has been minimized.

@msimpson

msimpson May 24, 2016

Contributor

Ah, I see.

this.object3D.add(obj);
}
if (obj instanceof THREE.Group && obj.children.length) {
obj.traverse(function bindEl (child) {

This comment has been minimized.

@dmarcos

dmarcos May 24, 2016

Collaborator

Do we want to do this with only THREE.Group or for any Object3D with children?

This comment has been minimized.

@msimpson

msimpson May 25, 2016

Contributor

Yeah, that's a good point. In fact, do we need to be recursive as well?

This comment has been minimized.

@ngokevin

ngokevin May 25, 2016

Member

Does this solution work with any OBJ mesh if we don't recurse?

This comment has been minimized.

@msimpson

msimpson May 25, 2016

Contributor

From what I can see inside Three's OBJLoader, all groups and objects are throw into a flat list. So this code would work with any OBJ borne mesh. I only worry about other formats; but if that's not the case, this could work:

if (obj instanceof THREE.Object3D) {
  obj.el = self;
  this.object3D.add(obj);
  if (obj.children.length) {
    obj.traverse(function bindEl (child) {
      child.el = self;
    });
  }
}

This comment has been minimized.

@ngokevin

ngokevin May 25, 2016

Member

I think let's just not recurse for now, and do it if we find we have to. Don't want to unnecessarily do this.

@ngokevin

This comment has been minimized.

Member

ngokevin commented May 25, 2016

Can you add a small test in the entity suite?

@msimpson

This comment has been minimized.

Contributor

msimpson commented May 25, 2016

Sure.

Matt Simpson added some commits May 25, 2016

Matt Simpson
Update a-entity's setObject3D to bind `el` to all children of the giv…
…en object.

Add a test for setObject3D to prove the binding works.
@msimpson

This comment has been minimized.

Contributor

msimpson commented May 25, 2016

Hmm. Travis CI seems to fail a lot.

@dmarcos

This comment has been minimized.

Collaborator

dmarcos commented May 26, 2016

@msimpson Thanks!

@dmarcos dmarcos merged commit 75c4aa2 into aframevr:master May 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment