Infer missing bufferView targets #275
Conversation
lib/addDefaults.js
Outdated
| function inferBufferViewTargets(gltf) { | ||
| // If bufferView elements are missing targets, we can infer their type from their use | ||
| var needsTarget = {}; | ||
| var shouldTraverse = false; |
There was a problem hiding this comment.
Is this optimization worth it: change this boolean to an integer with the number of bufferViews that need a target, decrease it and set needsTarget[bufferViewId] = false every time a target is set, then exit the outer most ForEach.mesh early when it is zero. The return value of the callback could indicate break if it doesn't already.
Given that this is simple, it could be worth it, I just think most models will either always have target defined or never have target defined.
|
LGTM, just that one comment. |
|
@pjcozzi, updated |
| var offset = defaultValue(handler(object, i), 0); | ||
| i += offset; | ||
| var returnValue = handler(object, i); | ||
| if (typeof returnValue === 'number') { |
There was a problem hiding this comment.
Is this needed? I didn't expect it and it doesn't look like it is used.
There was a problem hiding this comment.
It is used by the removal functions: RemoveUnusedProperties.js#L42, to make sure that the ForEach adjusts the index when elements get removed.
| i += returnValue; | ||
| } | ||
| else if (returnValue) { | ||
| break; |
There was a problem hiding this comment.
Can you add a unit test for this?
|
Updated |
|
Looks good. |
@pjcozzi, @lilleyse, this infers missing bufferView targets as discussed offline