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
Merged

Partial ellipsoid #5995

merged 72 commits into from
Sep 18, 2019

Conversation

srtrotter
Copy link
Contributor

@srtrotter srtrotter 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.

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
Copy link

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
Copy link
Contributor Author

I added a bullet to CHANGES.md.

@hpinkos
Copy link
Contributor

hpinkos 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
Copy link
Contributor

hpinkos 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
Copy link
Contributor

hpinkos 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
Copy link
Contributor

hpinkos 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
Copy link
Contributor

hpinkos 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
Copy link
Contributor

hpinkos 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
Copy link
Contributor

hpinkos 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
Copy link
Contributor Author

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
Copy link
Contributor

hpinkos commented Nov 21, 2017

@srtrotter no problem! Hope you have a good Thanksgiving

@srtrotter
Copy link
Contributor Author

srtrotter 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
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Throughout, please write out Minimum and Maximum.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@srtrotter
Copy link
Contributor Author

@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:/AnalyticalGraphicsInc/cesium/pull/5948)
* Fixed `Invalid asm.js: Invalid member of stdlib` console error by recompiling crunch.js with latest emscripten toolchain. [#5847](https:/AnalyticalGraphicsInc/cesium/issues/5847)
* Added CZML support for `polyline.depthFailMaterial`, `label.scaleByDistance`, `distanceDisplayCondition`, and `disableDepthTestDistance`. [#5986](https:/AnalyticalGraphicsInc/cesium/pull/5986)
* Added ability to create partial ellipsoid volumes using new geometry options: inner radii, min/max azimuth, min/max elevation.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

pjcozzi 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).
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@srtrotter
Copy link
Contributor Author

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

@nahgrin
Copy link
Contributor

nahgrin commented Jul 1, 2019

@pjcozzi @hpinkos

Any updates? How's the deadline stuff going?

@cesium-concierge
Copy link

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
Copy link
Contributor

nahgrin commented Aug 5, 2019

@hpinkos
@pjcozzi

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

@hpinkos
Copy link
Contributor

hpinkos 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
Copy link

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
Copy link
Contributor

hpinkos 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
Copy link
Contributor

nahgrin 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 CesiumGS:master Sep 18, 2019
@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

🎉 🎊 💯

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

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

@srtrotter
Copy link
Contributor Author

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.

@vince-hunt
Copy link

The implementation of this appears a bit buggy. Clipping against the terrain is not consistent at different zoom levels.

https://sandcastle.cesium.com/?src=Partial%20Ellipsoids.html

Zoomed out:
image

Zoomed in:
image

This clipping issue affects all the partial ellipsoids. It is particularly noticeable in the wedge shape shown above, but it can be seen on other ellipsoids where the ellipsoid meets the earth.

@srtrotter
Copy link
Contributor Author

@vince-hunt I don't think it has anything to do with the ellipsoid being partial. This problem exists (and has always existed) for all ellipsoids. I see it on full spheres as well. Are you seeing something that's different for a partial ellipsoid?

@serdaryesilmurat
Copy link

serdaryesilmurat commented Nov 5, 2021

A little late for the conversation 😅 , but is it possible to draw the sector (3rd geometry) shown in the picture below?
The "Wedge" sample in https://sandcastle.cesium.com/?src=Partial%20Ellipsoids.html (the darkcyan partial ellipsoid) does not cover what we need actually.

By the way, I can draw a "plane" sector by setting minimumCone and maximumCone to 90°.

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

@srtrotter
Copy link
Contributor Author

viewer.entities.add({
  name: "C",
  position: Cesium.Cartesian3.fromDegrees(-102.0, 35.0, 150000.0),
  ellipsoid: {
    radii: new Cesium.Cartesian3(500000.0, 500000.0, 800000.0),
    innerRadii: new Cesium.Cartesian3(400000.0, 400000.0, 400000.0),
    minimumClock: Cesium.Math.toRadians(45.0),
    maximumClock: Cesium.Math.toRadians(315.0),
    minimumCone: Cesium.Math.toRadians(80.0),
    maximumCone: Cesium.Math.toRadians(100.0),
    material: Cesium.Color.DARKCYAN.withAlpha(0.3),
    outline: true,
  },
});

@serdaryesilmurat that should get you something similar. You may have to create multiple ellipsoids and place them next to each other to get the exact shape you're looking for. Not sure what @hpinkos used to create that original shape.

@serdaryesilmurat
Copy link

@srtrotter Yes, that's what I am looking for 😃
Really appriciated for the quick response 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dome Geometry
9 participants