Interpolate custom vertex attributes on triangles crossing the IDL #6644
Conversation
|
@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
likangning93
force-pushed
the
customVertexAttribsTrianglesIDL
branch
from
1e77d98 to
a76957d
Compare
|
Thanks buddy, I updated CHANGES and then force-pushed to cover my shaaaame |
Source/Core/GeometryPipeline.js
Outdated
| var customAttributeNames = []; | ||
| for (var attributeName in attributes) { | ||
| if (attributes.hasOwnProperty(attributeName)) { | ||
| if (!NAMED_ATTRIBUTES[attributeName] && defined(attributes[attributeName])) { |
There was a problem hiding this comment.
This can be an && instead of a nested if
| @@ -2106,7 +2150,7 @@ define([ | |||
| } | |||
|
|
|||
| insertedIndex = insertSplitPoint(currentAttributes, currentIndices, currentIndexMap, indices, resultIndex < 3 ? i + resultIndex : -1, point); | |||
| computeTriangleAttributes(i0, i1, i2, point, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, insertedIndex); | |||
| computeTriangleAttributes(i0, i1, i2, point, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, customAttributeNames, customAttributesLength, attributes, insertedIndex); | |||
There was a problem hiding this comment.
Since we're changing this code around anyway, can we make this an options instead of having 50 parameters?
There was a problem hiding this comment.
I thought the gazillion parameters thing might have been an attempt at increasing performance for a hot area of code by avoiding dictionary access.
If that were true the custom attributes code would be "slow," but I think it's acceptable since the geometry that triggers this shouldn't have as much triangle density.
There was a problem hiding this comment.
Normally I would agree with @hpinkos but since this a local private function only called in a few places; I'm not sure turning it into an options object buys us anything in terms of code clarity/easy of maintenance.
There was a problem hiding this comment.
Gotcha, yeah this is fine then
|
Overall this looks really good @likangning93! @bagnell do you have any additional feedback? |
Source/Core/GeometryPipeline.js
Outdated
| @@ -1919,13 +1919,46 @@ define([ | |||
| var p0Scratch = new Cartesian3(); | |||
| var p1Scratch = new Cartesian3(); | |||
| var p2Scratch = new Cartesian3(); | |||
| var barycentricScratch = new Cartesian3(); | |||
| function interpolateAndPackCartesian3(i0, i1, i2, coords, sourceValues, currentValues, insertedIndex, normalize) { | |||
There was a problem hiding this comment.
Since this and the below function only differ with the cartesian type and the number of components, why not generate them?
function generateInterpolateFunction(cartesian, numberOfComponents) {
var p0Scratch = new cartesian();
// ...
return function(i0, i1, /*...*/) {
var p0 = cartesian.fromArray(sourceValues, i0 * numberOfComponents, p0Scratch);
// ...
};
}
var interpolateAndPackCartesian3 = generateInterpolateFunction(Carteisan3, 3);There was a problem hiding this comment.
@mramato Does this impact performance? If so, it might be fine since this most likely only gets executed in a web worker.
There was a problem hiding this comment.
Assuming we're not constantly regenerating the function over and over, then performance should be the same.
Source/Core/GeometryPipeline.js
Outdated
| var currentValues = currentAttribute.values; | ||
| switch(componentsPerAttribute) { | ||
| case 4: | ||
| var q0 = Cartesian4.fromArray(sourceValues, i0 * 4, q0Scratch); |
There was a problem hiding this comment.
You could generate this code with the function mentioned above.
|
Just have the one comment. The rest looks good. |
Fixes #4739.
Looks like #1894 is going to be a little more involved though, because the "custom" attributes on polylines probably can't be generically interpolated. Might have to do something similar to the current plan for polylines on terrain (#6615) which is going to create geometry that's already split across the IDL.
It's trickier for standard polylines though because a model matrix may be involved...