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

Enable orphaned nodes editing #59

Merged
merged 8 commits into from
Jul 19, 2018
Merged

Enable orphaned nodes editing #59

merged 8 commits into from
Jul 19, 2018

Conversation

tony056
Copy link
Contributor

@tony056 tony056 commented Jul 18, 2018

  • Features:
    • Remove orphaned nodes
    • Move orphaned nodes to other nodes
    • Update conflict resolution
  • Add:
    • Included two sample scenes with missing nodes or duplicate node names in the inherited file (missing-n-duplicate.scene and missing-node2.scene)
      orphaned-nodes

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

Minor-ish comments. LGTM otherwise!

}
},
"inherits": "./ArchitectureKit/Pillar1x3Rnd.gltf"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually demonstrate duplicate nodes.

@@ -0,0 +1,97 @@
export default class ConflictHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, if a module exports a single default class, I think we want the file name to match the class name.

}
};

setDuplicateStatus = newStatus => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If a class member is only used internally, or meant for private use, we start them with an underscore. I think that applies to a few of the methods in this class. Also applies to properties like duplicateList.

return this.duplicateList[name] > 1;
};

getMissingStatus = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this method is not used anywhere.

});
};

checkResolvedMissinRoot = scene => {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

this.setMissingStatus(newStatus);
return resolvedList;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job on refactoring this out into a class! It's much cleaner now.

list.forEach(resolvedMissingRoot => {
this.props.editor.removeObject(resolvedMissingRoot);
});
if (handler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I guess we're doing this because new scenes don't have conflictHandlers? Maybe they should.
Doesn't have to be fixed in this PR, but this kind of if statement (without an else) could lead to some surprising bugs. Maybe just keep a mental note to come back to this.

@tony056 tony056 merged commit bce1cca into master Jul 19, 2018
@tony056 tony056 deleted the features/orphaned-nodes branch July 19, 2018 00:53
robertlong pushed a commit that referenced this pull request Aug 13, 2018
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.

None yet

2 participants