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.

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

Partial ellipsoid #5995

Merged
merged 72 commits into from Sep 18, 2019

Conversation

@srtrotter
Copy link
Contributor

commented Nov 21, 2017

Fixes #5167

Added the ability to create a partial ellipsoid volume using new ellipsoid geometry options: innerRadii, azimuthMin, azimuthMax, elevationMin, elevationMax.

For example:
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(-100, 40, 5000),
ellipsoid: {
radii: new Cesium.Cartesian3(1500000, 1000000, 800000),
innerRadii: new Cesium.Cartesian3(500000, 400000, 300000),
azimuthMin: 0,
azimuthMax: 250,
elevationMin: 20,
elevationMax: 70,
material: Cesium.Color.DARKCYAN.withAlpha(0.3),
outline: true
}
});

Here is the thread in the forum that this originated from.

There are a few items that might need some work, but hopefully the hard part is done.

  • Unit tests
  • Sometimes an exception is thrown when the geometry spans a pole or meridian if 2D mode is enabled. Wasn't sure what was happening. 3D works fine.
  • Not supported for vertexFormat of: st, tangent, bitangent. I wasn't sure what was going on there. There are TODOs in the code where this needs work.
  • I did not duplicate the point at the poles of the ellipsoid. I couldn't tell why this was necessary.

This work is released under U.S. DoD Distribution Statement A: Approved for public release. Distribution is unlimited.

srtrotter added 9 commits Oct 3, 2017
Added partial ellipsoid capability.
Added partial ellipsoid capability.
Added partial ellipsoid capability.
Added partial ellipsoid capability.
Fixed typo from last commit.
Added partial ellipsoid capability.
@cesium-concierge

This comment has been minimized.

Copy link

commented Nov 21, 2017

Signed CLA is on file.

@srtrotter, thanks for the pull request! Maintainers, we have a signed CLA from @srtrotter, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

I added a bullet to CHANGES.md.

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

Wow, this is great @srtrotter! Thanks so much for the contribution!

I was playing around with this a little bit, here are some screenshots:

image image image

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

Sometimes an exception is thrown when the geometry spans a pole or meridian if 2D mode is enabled

Can you paste a code snippet to reproduce this? I wasn't able to

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

Not supported for vertexFormat of: st, tangent, bitangent

These are used for putting textured materials on the ellipsoid.

st is the texture coordinate (an (x, y) value for each vertex that maps to the x and y coordinates of an image. These values should be between 0 and 1)

tangent and bitangent are both normalized vectors that are orthogonal to the normal vector. For the ellipsoid, I believe we compute the tangent to point north an the bitangent is the cross product of normal and tangent

Here is some sample code to help with debugging these properties. DebugAppearance colors the ellipsoid based on the texture coordinate. createTangentSpaceDebugPrimitive creates a mini axis reference frame at each vertex so you can see which directions the normal/tangent/bitangent are pointing

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;

var radii = new Cesium.Cartesian3(200000.0, 200000.0, 200000.0);
var positionOnEllipsoid = Cesium.Cartesian3.fromDegrees(-100.0, 40.0);
var modelMatrix = Cesium.Matrix4.multiplyByTranslation(
    Cesium.Transforms.eastNorthUpToFixedFrame(positionOnEllipsoid),
    new Cesium.Cartesian3(0.0, 0.0, radii.z), new Cesium.Matrix4()
);

var ellipsoidGeometry = new Cesium.EllipsoidGeometry({
    vertexFormat : Cesium.VertexFormat.ALL,
    radii : radii
});
var ellipsoidInstance = new Cesium.GeometryInstance({
    geometry : ellipsoidGeometry,
    modelMatrix : modelMatrix,
});
scene.primitives.add(new Cesium.Primitive({
    geometryInstances : ellipsoidInstance,
    appearance : new Cesium.DebugAppearance({
        attributeName: 'st',
        translucent : false,
        closed : true
    })
}));
scene.primitives.add(Cesium.createTangentSpaceDebugPrimitive({
 geometry : ellipsoidGeometry,
 length : 10000.0,
 modelMatrix : modelMatrix
}));

image

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

I did not duplicate the point at the poles of the ellipsoid

I think this is also related to texturing. We want to create a hard seam so the left and right edges of the 2D texture don't blend together. Duplicating the positions lets assign different texture coordinates for the right edge and left edge of that seam.

For example, this is what the DebugAppearance looks like in master

image

And this is what it looks like in your branch that doesn't have the duplication:

image

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

You have some indicies winding order issues:

image

This is happening because some of the triangles are facing outward and some are facing in. The triangles should all have a counter-clockwise winding order

In this example, the indicies for the two triangles would be (1, 2, 3) and (1, 3, 4)

image

Here's the code to reproduce:

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;

var radii = new Cesium.Cartesian3(200000.0, 200000.0, 300000.0);
var positionOnEllipsoid = Cesium.Cartesian3.fromDegrees(-100.0, 40.0);
var modelMatrix = Cesium.Matrix4.multiplyByTranslation(
    Cesium.Transforms.eastNorthUpToFixedFrame(positionOnEllipsoid),
    new Cesium.Cartesian3(0.0, 0.0, radii.z), new Cesium.Matrix4()
);
var ellipsoidGeometry = new Cesium.EllipsoidGeometry({
    vertexFormat : Cesium.PerInstanceColorAppearance.VERTEX_FORMAT,
    radii: new Cesium.Cartesian3(1500000, 1000000, 800000),
    innerRadii: new Cesium.Cartesian3(500000, 400000, 300000),
    minimumAzimuth: 0,
    maximumAzimuth: Cesium.Math.toRadians(250),
    minimumElevation: Cesium.Math.toRadians(20),
    maximumElevation: Cesium.Math.toRadians(70),
});
var ellipsoidInstance = new Cesium.GeometryInstance({
    geometry : ellipsoidGeometry,
    modelMatrix : modelMatrix,
    attributes : {
        color : Cesium.ColorGeometryInstanceAttribute.fromColor(Cesium.Color.CYAN)
    }
});
scene.primitives.add(new Cesium.Primitive({
    geometryInstances : ellipsoidInstance,
    appearance : new Cesium.PerInstanceColorAppearance({
        translucent : false,
        closed: true
    })
}));
@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

The normals for the inner ellipsoid are pointing the wrong way:

image

I would expect them to be pointing in towards the center

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

Thanks again for the pull request @srtrotter! This is a pretty common feature request and our community will be thrilled once it is finally merged in =)

This are my initial comments for now. I'll take a closer look at the code after those few things are addressed, but this is a great start! Let me know if you have any questions or need clarification on anything.

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

Thanks for the info, @hpinkos ! I figured I had some issues like these that weren't exposed for my use cases. It will be next week at the earliest before I can look into any of these.

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

@srtrotter no problem! Hope you have a good Thanksgiving

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

To trigger the 2D exception:

viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(-100, 90, 5000),
ellipsoid: {
radii: new Cesium.Cartesian3(1500000, 1000000, 800000),
innerRadii: new Cesium.Cartesian3(500000, 400000, 300000),
azimuthMin: 0,
azimuthMax: 250,
elevationMin: 20,
elevationMax: 90,
material: Cesium.Color.DARKCYAN.withAlpha(0.3),
outline: true
}
});

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@hpinkos, could you please provide a code example to reproduce the 2D texture blend image, regarding duplicating the point at the poles?

@@ -1534,6 +1534,11 @@ define([

processPacketData(Boolean, ellipsoid, 'show', ellipsoidData.show, interval, sourceUri, entityCollection, query);
processPacketData(Cartesian3, ellipsoid, 'radii', ellipsoidData.radii, interval, sourceUri, entityCollection, query);
processPacketData(Cartesian3, ellipsoid, 'innerRadii', ellipsoidData.innerRadii, interval, sourceUri, entityCollection);
processPacketData(Number, ellipsoid, 'azimuthMin', ellipsoidData.azimuthMin, interval, sourceUri, entityCollection);

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Nov 27, 2017

Member

Throughout, please write out Minimum and Maximum.

We avoid abbreviations in the public API: https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#naming

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Nov 28, 2017

Member

Also, to be consistent the rest of Cesium, make "minimum" and "maximum" the prefix instead of the suffix, e.g., minimumAzimuth.

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@hpinkos, nevermind. I see how to draw the image.

CHANGES.md Outdated
@@ -6,6 +6,7 @@ Change Log
* Added ability to support touch event in Imagery Layers Split demo application. [#5948](https://github.com/AnalyticalGraphicsInc/cesium/pull/5948)
* Fixed `Invalid asm.js: Invalid member of stdlib` console error by recompiling crunch.js with latest emscripten toolchain. [#5847](https://github.com/AnalyticalGraphicsInc/cesium/issues/5847)
* Added CZML support for `polyline.depthFailMaterial`, `label.scaleByDistance`, `distanceDisplayCondition`, and `disableDepthTestDistance`. [#5986](https://github.com/AnalyticalGraphicsInc/cesium/pull/5986)
* Added ability to create partial ellipsoid volumes using new geometry options: inner radii, min/max azimuth, min/max elevation.

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Nov 28, 2017

Member

Can this explicitly mention the new Cesium classes and CZML properties? And link to the Sandcastle example if you are able to add one?

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

@srtrotter would you be able to add a new Sandcastle example for this that shows a few representative cases?

@@ -50,6 +50,11 @@ define([
*
* @param {Object} [options] Object with the following properties:
* @param {Cartesian3} [options.radii=Cartesian3(1.0, 1.0, 1.0)] The radii of the ellipsoid in the x, y, and z directions.
* @param {Cartesian3} [options.innerRadii=options.radii] The inner radii of the ellipsoid in the x, y, and z directions.
* @param {Number} [options.azimuthMin=0.0] The minimum azimuth in degrees (0 is north, +CW).

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Nov 28, 2017

Member

Please use radians throughout to be consistent with the rest of Cesium.

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@pjcozzi I am working on those changes and yes, I should be able to put a Sandcastle example together.

@nahgrin

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@hpinkos

I've been informed that the CLA has been sent to you guys on 5/30/19. I believe it had been sent to @pjcozzi .

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Ah, yes, we have a CLA just for this specific pull request so it is not in our automated systems. Thanks and sorry!

@nahgrin

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@pjcozzi No problems at all. Let me know if there's anything additional @srtrotter or I could do to help things get merged :)

@nahgrin

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@pjcozzi @hpinkos

Any updates? How's the deadline stuff going?

@cesium-concierge

This comment has been minimized.

Copy link

commented Aug 1, 2019

Thanks again for your contribution @srtrotter!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@nahgrin

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

@hpinkos
@pjcozzi

Sorry to keep pinging you guys, any status on getting this merged in?

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Sorry we haven't had a chance to review this one yet @nahgrin. I'm not sure exactly when I'll have a chance, but it shouldn't be much longer

@cesium-concierge

This comment has been minimized.

Copy link

commented Sep 5, 2019

Thanks again for your contribution @srtrotter!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos added 5 commits Sep 18, 2019
@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Thank you so much for the fantastic work on this @srtrotter and @nahgrin! I'm sorry it took so long for me to take a final look at this. I just tested everything and it looks great. I just made some minor changes to the Sandcastle example.

I'm going to merge this as soon as CI passes

@nahgrin

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@hpinkos thank you so much for all your help!

I'm so excited! :)

And thank you @srtrotter for starting this and helping getting it to finish.

@hpinkos hpinkos merged commit d799593 into AnalyticalGraphicsInc:master Sep 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

🎉 🎊 💯

@hpinkos

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@shunter FYI new CZML properties for ellipsoid: innerRadii, minimumClock, maximumClock, minimumCone, and maximumCone

@srtrotter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Great! Thanks @hpinkos for getting this merged in! And thanks @nahgrin for your work in getting this through to the finish line! Very excited that this is complete.

@srtrotter srtrotter deleted the srtrotter:partial-ellipsoid branch Oct 4, 2019
@srtrotter srtrotter restored the srtrotter:partial-ellipsoid branch Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.