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

Jump to bottom

Scale bounding sphere #1155

Merged
merged 6 commits into from Sep 24, 2013
Merged

Scale bounding sphere #1155

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6fc4a20
scale bounding sphere
Sep 17, 2013
dae0285
correct bounding sphere scale.
Sep 18, 2013
91261e9
cleanup
Sep 18, 2013
2b99acb
cleanup
Sep 18, 2013
6be9796
Merge branch 'master' into scaleBoundingSphere
Sep 23, 2013
eb7dfd6
cleanup
Sep 23, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions Source/Core/BoundingSphere.js
View file
Open in desktop
Expand Up @@ -701,6 +701,7 @@ define([
* @exception {DeveloperError} sphere is required.
* @exception {DeveloperError} transform is required.
*/
var radiusScratch = new Cartesian3();
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a better name. Radius is a scalar. This is a vector.

Copy link
Contributor

Choose a reason for hiding this comment

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

The variable also needs to be placed before the JSDoc comment above, so that the JSDoc is correctly attached to BoundingSphere.transform.

BoundingSphere.transform = function(sphere, transform, result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test explicitly for this to BoundingSphereSpec.js? We like to test from the bottom up.

if (!defined(sphere)) {
throw new DeveloperError('sphere is required.');
Expand All @@ -716,8 +717,12 @@ define([

Matrix4.multiplyByPoint(transform, sphere.center, transformCart4);

Cartesian3.clone(transformCart4, result.center);
result.radius = sphere.radius;
result.center = Cartesian3.clone(transformCart4, result.center);
var r = Cartesian3.magnitude(Matrix4.getColumn(transform, 0, radiusScratch));
Copy link
Contributor

Choose a reason for hiding this comment

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

Math.max takes any number of arguments so this can be one call to Math.max with three arguments.

r = Math.max(r, Cartesian3.magnitude(Matrix4.getColumn(transform, 1, radiusScratch)));
r = Math.max(r, Cartesian3.magnitude(Matrix4.getColumn(transform, 2, radiusScratch)));
result.radius = r * sphere.radius;

return result;
};

Expand Down
6 changes: 3 additions & 3 deletions Source/Core/GeometryPipeline.js
View file
Open in desktop
Expand Up @@ -765,7 +765,8 @@ define([
for (var i = 0; i < length; i += 3) {
Cartesian3.unpack(values, i, scratchCartesian3);
Matrix3.multiplyByVector(matrix, scratchCartesian3, scratchCartesian4);
Cartesian3.pack(scratchCartesian4, values, i);
scratchCartesian3 = Cartesian3.normalize(scratchCartesian4, scratchCartesian3);
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 need another scratch variable, right? The input can be the same as the output so we can just use scratchCartesian3 everywhere.

Cartesian3.pack(scratchCartesian3, values, i);
}
}
}
Expand Down Expand Up @@ -827,8 +828,7 @@ define([
var boundingSphere = instance.geometry.boundingSphere;

if (defined(boundingSphere)) {
Matrix4.multiplyByPoint(modelMatrix, boundingSphere.center, boundingSphere.center);
boundingSphere.center = Cartesian3.fromCartesian4(boundingSphere.center);
instance.geometry.boundingSphere = BoundingSphere.transform(boundingSphere, modelMatrix, boundingSphere);
}

instance.modelMatrix = Matrix4.IDENTITY.clone();
Expand Down
43 changes: 43 additions & 0 deletions Specs/Core/GeometryPipelineSpec.js
View file
Open in desktop
Expand Up @@ -812,6 +812,49 @@ defineSuite([
expect(transformed.modelMatrix).toEqual(Matrix4.IDENTITY);
});

it('transformToWorldCoordinates with uniform scale', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to test with a non-uniform scale?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both tests now?

var instance = new GeometryInstance({
geometry : new Geometry({
attributes : {
position : new GeometryAttribute({
componentDatatype : ComponentDatatype.FLOAT,
componentsPerAttribute : 3,
values : [
0.0, 0.0, 0.0,
1.0, 0.0, 0.0,
0.0, 1.0, 0.0
]
}),
normal : new GeometryAttribute({
componentDatatype : ComponentDatatype.FLOAT,
componentsPerAttribute : 3,
values : [
0.0, 0.0, 1.0,
0.0, 0.0, 1.0,
0.0, 0.0, 1.0
]
})
},
indices : [0, 1, 2],
primitiveType : PrimitiveType.TRIANGLES,
boundingSphere : new BoundingSphere(new Cartesian3(0.5, 0.5, 0.0), 1.0)
}),
modelMatrix : Matrix4.multiplyByUniformScale(new Matrix4(0.0, 0.0, 1.0, 0.0,
1.0, 0.0, 0.0, 0.0,
0.0, 1.0, 0.0, 0.0,
0.0, 0.0, 0.0, 1.0), 2)
});

var transformed = GeometryPipeline.transformToWorldCoordinates(instance);
var transformedPositions = [0.0, 0.0, 0.0, 0.0, 2.0, 0.0, 0.0, 0.0, 2.0];
var transformedNormals = [1.0, 0.0, 0.0, 1.0, 0.0, 0.0, 1.0, 0.0, 0.0];

expect(transformed.geometry.attributes.position.values).toEqual(transformedPositions);
expect(transformed.geometry.attributes.normal.values).toEqual(transformedNormals);
expect(transformed.geometry.boundingSphere).toEqual(new BoundingSphere(new Cartesian3(0.0, 1.0, 1.0), 2.0));
expect(transformed.modelMatrix).toEqual(Matrix4.IDENTITY);
});

it('transformToWorldCoordinates does nothing when already in world coordinates', function() {
var instance = new GeometryInstance({
geometry : new Geometry({
Expand Down