Combine nodes #151
Combine nodes #151
Conversation
|
@lasalvavida does this replace #51? Is any of this code based on #51? Have you tested on a variety of models, including ones where some sub-trees can be combined and some can't? |
|
No, none of this is based on #51. I'm pretty sure I've handled all of the edge cases. I used reciprocating saw as an example here because CAD models typically have repeated meshes/overlapping mesh data which are the two cases where primitives can't be transformed and passed up the node tree. I have tested on: |
| "mime": "1.3.4", | ||
| "mkdirp": "0.5.1", | ||
| "object-values": "1.0.0", | ||
| "promise": "7.1.1", | ||
| "underscore": "^1.8.3", |
There was a problem hiding this comment.
Remove the carets from these.
|
Testing looks good from my end too. |
…-pipeline into combine-nodes
|
Updated |
| 'use strict'; | ||
| var Cesium = require('cesium'); | ||
| var jp = require('jsonpath'); | ||
| var _ = require('underscore'); |
There was a problem hiding this comment.
@mramato is it standard practice to use the name _ here?
There was a problem hiding this comment.
Unfortunately, yes. It's the same idea as using $ for jquery, which I also hate. That being said, I would strongly suggest not depending on underscore, there is absolutely no reason that I know of to use it over native JavaScript functions.
There was a problem hiding this comment.
There was a problem hiding this comment.
It does have a deep comparison _.equals which is really what I'm using it for. We have rewritten that function so many times in this library, I figured it might be best to try to consolidate it to a third party utility.
@mramato I'm fine with using a different node module for deep equals, rolling our own or just keeping underscore, whatever you think is best practice here.
There was a problem hiding this comment.
Also _.range is useful for generating indices.
There was a problem hiding this comment.
Underscore is an old, traditionally browser-based library meant to make up for missing features of the browser API and incompatibility from platform to platform. For Node.js, there is only a single platform and 99% of functionality is either directly supported by the native API or is otherwise only a couple of lines of code. Here's an article that tries to lay out some examples: https://www.reindex.io/blog/you-might-not-need-underscore/ That article does use some ES2015 examples, which technically we can't use yet, but it's really just shorthand for stuff that can be done in ES5 already. We should never use a library if there is an equivalent native API already available, it just makes the code slower and less uniform.
As for equals, I don't see it actually being called anywhere in this PR, but if we really need a deep equality function, something like https://www.npmjs.com/package/deep-equal is probably a better choice. Also, if we are only using deep-equal in tests, then there's already jasimine.toEqual and assert.equals
Normally, we avoid generic deep comparisons in our code and prefer implementing custom equals functions directly (such as Matrix4.equals for example) because they are way faster. If we need to compare completely unknown blobs, then of course we may need to have generic comparisons, but since gltf is a known schema, I'm not sure of why that would be the case.
There was a problem hiding this comment.
I'm using _.isEqual and _.find to search for duplicate primitives in an array. That was something else -> optimizing array searching.
I'll take a look around though and see if there's some better choices for that as well. ES2015 will resolve this, but for now we will need some other utility.
There was a problem hiding this comment.
Other than deep-equals, I'm not sure why we would need any other libraries. .find is just a for loop with a break statement. It provides no benefit. We already have binarySearch in Cesium if your arrays are sorted. If they aren't sorted, then there is no "optimized" search method; it's always a linear search.
There was a problem hiding this comment.
Yup, you're right. I thought maybe they were doing a randomized search, which is more optimal than linear for non-sorted data, but it is just linear.
|
@tfili can you please review the test cases in combineNodesSpec.js and see if we missed any from your experience with COLLADA? |
|
Consider adding one more unit tests that is more like a "system test" that tests several scenarios within one node hierarchy where some sub-trees can be combined and some can't. Verify that it is as aggressive as possible above and below the roots of the sub-trees that can't be merged into their parent. Along these lines, can siblings be merged, e.g.,? If |
|
If |
| @@ -0,0 +1,367 @@ | |||
| 'use strict'; | |||
| var Cesium = require('cesium'); | |||
| var jp = require('jsonpath'); | |||
There was a problem hiding this comment.
variable names should always match module names.
|
Sounds good, please make sure this is covered by tests.
|
Adds stage to flatten node hierarchies where possible. Reciprocating saw screenshot attached for comparison.
@lilleyse Could you look at this when you get a chance?