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.

Sign up for GitHub

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

Interpolate custom vertex attributes on triangles crossing the IDL #6644

Merged
merged 3 commits into from
Jun 5, 2018
Merged

Interpolate custom vertex attributes on triangles crossing the IDL #6644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a76957d
interpolate custom vertex attributes on triangles crossing the IDL
likangning93 May 31, 2018
da39c4e
merge master into customVertexAttribsTrianglesIDL
likangning93 Jun 5, 2018
65b6a4e
PR comments
likangning93 Jun 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
Change Log
==========

### 1.47 - 2018-07-01

##### Fixes :wrench:
* Fixed a bug with custom vertex attributes on `Geometry` crossing the IDL causing crashes. Attributes will be barycentrically interpolated. [#6644](https://github.com/AnalyticalGraphicsInc/cesium/pull/6644)

### 1.46 - 2018-06-01

##### Breaking Changes :mega:
Expand Down
156 changes: 100 additions & 56 deletions Source/Core/GeometryPipeline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

@bagnell bagnell Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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);

Copy link
Contributor

Choose a reason for hiding this comment

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

@mramato Does this impact performance? If so, it might be fine since this most likely only gets executed in a web worker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we're not constantly regenerating the function over and over, then performance should be the same.

var p0 = Cartesian3.fromArray(sourceValues, i0 * 3, p0Scratch);
var p1 = Cartesian3.fromArray(sourceValues, i1 * 3, p1Scratch);
var p2 = Cartesian3.fromArray(sourceValues, i2 * 3, p2Scratch);

Cartesian3.multiplyByScalar(p0, coords.x, p0);
Cartesian3.multiplyByScalar(p1, coords.y, p1);
Cartesian3.multiplyByScalar(p2, coords.z, p2);

var value = Cartesian3.add(p0, p1, p0);
Cartesian3.add(value, p2, value);

if (normalize) {
Cartesian3.normalize(value, value);
}

Cartesian3.pack(value, currentValues, insertedIndex * 3);
}

var s0Scratch = new Cartesian2();
var s1Scratch = new Cartesian2();
var s2Scratch = new Cartesian2();
function interpolateAndPackCartesian2(i0, i1, i2, coords, sourceValues, currentValues, insertedIndex) {
var s0 = Cartesian2.fromArray(sourceValues, i0 * 2, s0Scratch);
var s1 = Cartesian2.fromArray(sourceValues, i1 * 2, s1Scratch);
var s2 = Cartesian2.fromArray(sourceValues, i2 * 2, s2Scratch);

Cartesian2.multiplyByScalar(s0, coords.x, s0);
Cartesian2.multiplyByScalar(s1, coords.y, s1);
Cartesian2.multiplyByScalar(s2, coords.z, s2);

function computeTriangleAttributes(i0, i1, i2, point, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, insertedIndex) {
if (!defined(normals) && !defined(tangents) && !defined(bitangents) && !defined(texCoords) && !defined(extrudeDirections)) {
var value = Cartesian2.add(s0, s1, s0);
Cartesian2.add(value, s2, value);

Cartesian2.pack(value, currentValues, insertedIndex * 2);
}

var barycentricScratch = new Cartesian3();
function computeTriangleAttributes(i0, i1, i2, point, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, customAttributeNames, customAttributesLength, allAttributes, insertedIndex) {
if (!defined(normals) && !defined(tangents) && !defined(bitangents) && !defined(texCoords) && !defined(extrudeDirections) && customAttributesLength === 0) {
return;
}

Expand All @@ -1935,19 +1968,7 @@ define([
var coords = barycentricCoordinates(point, p0, p1, p2, barycentricScratch);

if (defined(normals)) {
var n0 = Cartesian3.fromArray(normals, i0 * 3, p0Scratch);
var n1 = Cartesian3.fromArray(normals, i1 * 3, p1Scratch);
var n2 = Cartesian3.fromArray(normals, i2 * 3, p2Scratch);

Cartesian3.multiplyByScalar(n0, coords.x, n0);
Cartesian3.multiplyByScalar(n1, coords.y, n1);
Cartesian3.multiplyByScalar(n2, coords.z, n2);

var normal = Cartesian3.add(n0, n1, n0);
Cartesian3.add(normal, n2, normal);
Cartesian3.normalize(normal, normal);

Cartesian3.pack(normal, currentAttributes.normal.values, insertedIndex * 3);
interpolateAndPackCartesian3(i0, i1, i2, coords, normals, currentAttributes.normal.values, insertedIndex, true);
}

if (defined(extrudeDirections)) {
Expand All @@ -1974,50 +1995,55 @@ define([
}

if (defined(tangents)) {
var t0 = Cartesian3.fromArray(tangents, i0 * 3, p0Scratch);
var t1 = Cartesian3.fromArray(tangents, i1 * 3, p1Scratch);
var t2 = Cartesian3.fromArray(tangents, i2 * 3, p2Scratch);

Cartesian3.multiplyByScalar(t0, coords.x, t0);
Cartesian3.multiplyByScalar(t1, coords.y, t1);
Cartesian3.multiplyByScalar(t2, coords.z, t2);

var tangent = Cartesian3.add(t0, t1, t0);
Cartesian3.add(tangent, t2, tangent);
Cartesian3.normalize(tangent, tangent);

Cartesian3.pack(tangent, currentAttributes.tangent.values, insertedIndex * 3);
interpolateAndPackCartesian3(i0, i1, i2, coords, tangents, currentAttributes.tangent.values, insertedIndex, true);
}

if (defined(bitangents)) {
var b0 = Cartesian3.fromArray(bitangents, i0 * 3, p0Scratch);
var b1 = Cartesian3.fromArray(bitangents, i1 * 3, p1Scratch);
var b2 = Cartesian3.fromArray(bitangents, i2 * 3, p2Scratch);

Cartesian3.multiplyByScalar(b0, coords.x, b0);
Cartesian3.multiplyByScalar(b1, coords.y, b1);
Cartesian3.multiplyByScalar(b2, coords.z, b2);

var bitangent = Cartesian3.add(b0, b1, b0);
Cartesian3.add(bitangent, b2, bitangent);
Cartesian3.normalize(bitangent, bitangent);

Cartesian3.pack(bitangent, currentAttributes.bitangent.values, insertedIndex * 3);
interpolateAndPackCartesian3(i0, i1, i2, coords, bitangents, currentAttributes.bitangent.values, insertedIndex, true);
}

if (defined(texCoords)) {
var s0 = Cartesian2.fromArray(texCoords, i0 * 2, s0Scratch);
var s1 = Cartesian2.fromArray(texCoords, i1 * 2, s1Scratch);
var s2 = Cartesian2.fromArray(texCoords, i2 * 2, s2Scratch);

Cartesian2.multiplyByScalar(s0, coords.x, s0);
Cartesian2.multiplyByScalar(s1, coords.y, s1);
Cartesian2.multiplyByScalar(s2, coords.z, s2);
interpolateAndPackCartesian2(i0, i1, i2, coords, texCoords, currentAttributes.st.values, insertedIndex);
}

var texCoord = Cartesian2.add(s0, s1, s0);
Cartesian2.add(texCoord, s2, texCoord);
if (customAttributesLength > 0) {
for (var i = 0; i < customAttributesLength; i++) {
var attributeName = customAttributeNames[i];
genericInterpolate(i0, i1, i2, coords, insertedIndex, allAttributes[attributeName], currentAttributes[attributeName]);
}
}
}

Cartesian2.pack(texCoord, currentAttributes.st.values, insertedIndex * 2);
var q0Scratch = new Cartesian4();
var q1Scratch = new Cartesian4();
var q2Scratch = new Cartesian4();
function genericInterpolate(i0, i1, i2, coords, insertedIndex, sourceAttribute, currentAttribute) {
var componentsPerAttribute = sourceAttribute.componentsPerAttribute;
var sourceValues = sourceAttribute.values;
var currentValues = currentAttribute.values;
switch(componentsPerAttribute) {
case 4:
var q0 = Cartesian4.fromArray(sourceValues, i0 * 4, q0Scratch);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could generate this code with the function mentioned above.

var q1 = Cartesian4.fromArray(sourceValues, i1 * 4, q1Scratch);
var q2 = Cartesian4.fromArray(sourceValues, i2 * 4, q2Scratch);

Cartesian4.multiplyByScalar(q0, coords.x, q0);
Cartesian4.multiplyByScalar(q1, coords.y, q1);
Cartesian4.multiplyByScalar(q2, coords.z, q2);

var value = Cartesian4.add(q0, q1, q0);
Cartesian4.add(value, q2, value);

Cartesian4.pack(value, currentValues, insertedIndex * 4);
break;
case 3:
interpolateAndPackCartesian3(i0, i1, i2, coords, sourceValues, currentValues, insertedIndex, false);
break;
case 2:
interpolateAndPackCartesian2(i0, i1, i2, coords, sourceValues, currentValues, insertedIndex);
break;
default:
currentValues[insertedIndex] = sourceValues[i0] * coords.x + sourceValues[i1] * coords.y + sourceValues[i2] * coords.z;
}
}

Expand All @@ -2044,6 +2070,14 @@ define([
return insertIndex;
}

var NAMED_ATTRIBUTES = {
position : true,
normal : true,
bitangent : true,
tangent : true,
st : true,
extrudeDirection : true
};
function splitLongitudeTriangles(instance) {
var geometry = instance.geometry;
var attributes = geometry.attributes;
Expand All @@ -2055,6 +2089,16 @@ define([
var extrudeDirections = (defined(attributes.extrudeDirection)) ? attributes.extrudeDirection.values : undefined;
var indices = geometry.indices;

var customAttributeNames = [];
for (var attributeName in attributes) {
if (attributes.hasOwnProperty(attributeName)) {
if (!NAMED_ATTRIBUTES[attributeName] && defined(attributes[attributeName])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be an && instead of a nested if

customAttributeNames.push(attributeName);
}
}
}
var customAttributesLength = customAttributeNames.length;

var eastGeometry = copyGeometryForSplit(geometry);
var westGeometry = copyGeometryForSplit(geometry);

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're changing this code around anyway, can we make this an options instead of having 50 parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, yeah this is fine then

}
} else {
if (defined(result)) {
Expand All @@ -2126,13 +2170,13 @@ define([
}

insertedIndex = insertSplitPoint(currentAttributes, currentIndices, currentIndexMap, indices, i, p0);
computeTriangleAttributes(i0, i1, i2, p0, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, insertedIndex);
computeTriangleAttributes(i0, i1, i2, p0, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, customAttributeNames, customAttributesLength, attributes, insertedIndex);

insertedIndex = insertSplitPoint(currentAttributes, currentIndices, currentIndexMap, indices, i + 1, p1);
computeTriangleAttributes(i0, i1, i2, p1, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, insertedIndex);
computeTriangleAttributes(i0, i1, i2, p1, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, customAttributeNames, customAttributesLength, attributes, insertedIndex);

insertedIndex = insertSplitPoint(currentAttributes, currentIndices, currentIndexMap, indices, i + 2, p2);
computeTriangleAttributes(i0, i1, i2, p2, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, insertedIndex);
computeTriangleAttributes(i0, i1, i2, p2, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, customAttributeNames, customAttributesLength, attributes, insertedIndex);
}
}

Expand Down
134 changes: 134 additions & 0 deletions Specs/Core/GeometryPipelineSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1871,6 +1871,140 @@ defineSuite([
expect(splitInstance).toBe(instance);
});

it('splitLongitude interpolates custom attributes for geometry split by the IDL', function() {
var p0 = Cartesian3.fromDegrees(-179.0, 0.0);
var p1 = Cartesian3.fromDegrees(179.0, 0.0);
var p2 = Cartesian3.fromDegrees(-179.0, 1.0);

var positions = new Float64Array([
p0.x, p0.y, p0.z,
p1.x, p1.y, p1.z,
p2.x, p2.y, p2.z
]);

var vec4s = new Uint8Array([
0, 0, 0, 0,
0, 0, 0, 255,
0, 0, 0, 0
]);

var vec3s = new Uint8Array([
0, 0, 0,
0, 0, 255,
0, 0, 0
]);

var vec2s = new Uint8Array([
0, 0,
0, 255,
0, 0
]);

var scalars = new Uint8Array([0, 255, 0]);

var instance = new GeometryInstance({
geometry : new Geometry({
attributes : {
position : new GeometryAttribute({
componentDatatype : ComponentDatatype.DOUBLE,
componentsPerAttribute : 3,
values : positions
}),
vec4s: new GeometryAttribute({
componentDatatype: ComponentDatatype.UNSIGNED_BYTE,
componentsPerAttribute: 4,
values: vec4s
}),
vec3s: new GeometryAttribute({
componentDatatype: ComponentDatatype.UNSIGNED_BYTE,
componentsPerAttribute: 3,
values: vec3s
}),
vec2s: new GeometryAttribute({
componentDatatype: ComponentDatatype.UNSIGNED_BYTE,
componentsPerAttribute: 2,
values: vec2s
}),
scalars: new GeometryAttribute({
componentDatatype: ComponentDatatype.UNSIGNED_BYTE,
componentsPerAttribute: 1,
values: scalars
})
},
indices : new Uint16Array([0, 1, 2]),
primitiveType : PrimitiveType.TRIANGLES,
boundingSphere : BoundingSphere.fromVertices(positions)
})
});

var splitInstance = GeometryPipeline.splitLongitude(instance);
var eastHemisphereGeometry = splitInstance.eastHemisphereGeometry;
expect(eastHemisphereGeometry.indices.length).toEqual(3);

var newVec4s = eastHemisphereGeometry.attributes.vec4s.values;
var newVec3s = eastHemisphereGeometry.attributes.vec3s.values;
var newVec2s = eastHemisphereGeometry.attributes.vec2s.values;
var newScalars = eastHemisphereGeometry.attributes.scalars.values;
var i;
var index;

// Expect eastern hemisphere vertices to all be 255 or 127 at the end of the value
expect(newScalars.indexOf(127)).not.toBe(-1);
expect(newVec4s.indexOf(127)).not.toBe(-1);
expect(newVec3s.indexOf(127)).not.toBe(-1);
expect(newVec2s.indexOf(127)).not.toBe(-1);
for (i = 0; i < 3; i++) {
expect(newScalars[i] === 255 || newScalars[i] === 127).toBe(true);

index = i * 2;
expect(newVec2s[index]).toBe(0);
expect(newVec2s[index + 1] === 255 || newVec2s[index + 1] === 127).toBe(true);

index = i * 3;
expect(newVec3s[index]).toBe(0);
expect(newVec3s[index + 1]).toBe(0);
expect(newVec3s[index + 2] === 255 || newVec3s[index + 2] === 127).toBe(true);

index = i * 4;
expect(newVec4s[index]).toBe(0);
expect(newVec4s[index + 1]).toBe(0);
expect(newVec4s[index + 2]).toBe(0);
expect(newVec4s[index + 3] === 255 || newVec4s[index + 3] === 127).toBe(true);
}

var westHemisphereGeometry = splitInstance.westHemisphereGeometry;
expect(westHemisphereGeometry.indices.length).toEqual(6);

newVec4s = westHemisphereGeometry.attributes.vec4s.values;
newVec3s = westHemisphereGeometry.attributes.vec3s.values;
newVec2s = westHemisphereGeometry.attributes.vec2s.values;
newScalars = westHemisphereGeometry.attributes.scalars.values;

// Expect eastern hemisphere vertices to all be 0 or 127 at the end of the value
expect(newScalars.indexOf(127)).not.toBe(-1);
expect(newVec4s.indexOf(127)).not.toBe(-1);
expect(newVec3s.indexOf(127)).not.toBe(-1);
expect(newVec2s.indexOf(127)).not.toBe(-1);
for (i = 0; i < 4; i++) {
expect(newScalars[i] === 0 || newScalars[i] === 127).toBe(true);

index = i * 2;
expect(newVec2s[index]).toBe(0);
expect(newVec2s[index + 1] === 0 || newVec2s[index + 1] === 127).toBe(true);

index = i * 3;
expect(newVec3s[index]).toBe(0);
expect(newVec3s[index + 1]).toBe(0);
expect(newVec3s[index + 2] === 0 || newVec3s[index + 2] === 127).toBe(true);

index = i * 4;
expect(newVec4s[index]).toBe(0);
expect(newVec4s[index + 1]).toBe(0);
expect(newVec4s[index + 2]).toBe(0);
expect(newVec4s[index + 3] === 0 || newVec4s[index + 3] === 127).toBe(true);
}
});

it('splitLongitude provides indices for an un-indexed triangle list', function() {
var instance = new GeometryInstance({
geometry : new Geometry({
Expand Down