CoplanarPolygonGeometry and CoplanarPolygonOutlineGeometry #6769

hpinkos · 2018-07-03T21:57:27Z

Adds support for drawing polygons composed of arbitrary coplanar positions. (It doesn't crash if the positions are not coplanar, but the resulting triangulation is unpredictable and probably won't work as desired)

Related #3349 (adds support for vertical polygons but doesn't fix that KML file)

Test code:

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

var polygonGeometry = new Cesium.CoplanarPolygonGeometry({
    vertexFormat : Cesium.VertexFormat.ALL,
    positions : Cesium.Cartesian3.fromDegreesArrayHeights([
       -91.0, 50.0, 0.0,
       -90.0, 50.0, 200000.0,
       -89.0, 50.0, 200000.0,
       -88.0, 50.0, 0.0
    ])
});

var polygonInstance = new Cesium.GeometryInstance({
    geometry : polygonGeometry,
    attributes : {
        color : Cesium.ColorGeometryInstanceAttribute.fromColor(Cesium.Color.BLUE)
    }
});

scene.primitives.add(new Cesium.Primitive({
    geometryInstances : polygonInstance,
    appearance : new Cesium.MaterialAppearance({
        material : Cesium.Material.fromType('Checkerboard'),
        materialSupport : Cesium.MaterialAppearance.MaterialSupport.TEXTURED
    })
}));

scene.primitives.add(Cesium.createTangentSpaceDebugPrimitive({
	geometry : polygonGeometry,
	length : 100000.0
}));

cesium-concierge · 2018-07-03T21:57:28Z

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

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

🌍 🌎 🌏

hpinkos · 2018-07-03T22:03:05Z

This doesn't connect anything to the Entity API layer. I was planning to figure out if and how we want to do that in a separate PR.

hpinkos · 2018-07-06T16:04:08Z

@ggetz can you review?

ggetz · 2018-07-06T18:04:47Z

Apps/Sandcastle/gallery/development/Coplanar Polygon Outline.html

+    <meta charset="utf-8">
+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
+    <meta name="description" content="Draw the outline of a cylinder.">


Update description.

ggetz · 2018-07-06T18:05:31Z

Apps/Sandcastle/gallery/development/Coplanar Polygon.html

+    <meta charset="utf-8">
+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
+    <meta name="description" content="Draw an ellipsoid.">


Update description.

ggetz · 2018-07-06T18:09:42Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+
+        var i;
+        // If all the points are on a line, just remove one of the zero dimensions
+        if ((xMag === 0 && (yMag === 0 || zMag === 0)) || (yMag === 0 && zMag === 0)) {


Should there be some sort of error or warning rather than the geometry just not being created?

I guess to answer my own question, that is the behavior when less than 3 positions are supplied.

Yep, we decided a while ago to just return undefined and not render invalid geometry instead of throwing an error. This is mostly because there's a lot of garbage data in KML and GeoJSON files that are automatically generated and we don't want to crash the whole file because we don't know how to render one of the polygons

ggetz · 2018-07-06T18:12:52Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+        var zMag = Cartesian3.magnitude(zAxis);
+        var min = Math.min(xMag, yMag, zMag);
+
+        var i;


Declare i down where it's being used.

ggetz · 2018-07-06T18:14:16Z

Source/Core/CoplanarPolygonGeometry.js

+
+    /**
+     * A description of a polygon composed of arbitrary coplanar positions.
+     *


Should we mention in the docs that if the positions are not coplanar, it will re-project them to a plane?

It doesn't. It will attempt to draw whatever positions are passed in, but for best results the positions should be coplanar

ggetz · 2018-07-06T18:49:33Z

Source/Core/CoplanarPolygonGeometry.js

+     *
+     * @param {Object} options Object with the following properties:
+     * @param {Cartesian3[]} options.positions The positions of the polygon
+     * @param {VertexFormat} [options.vertexFormat=VertexFormat.DEFAULT] The vertex attributes to be computed.


Should we also have stRotation as an option? PolygonGeometry has this as an option. Besides the polygon hierarchy, all the other options aren't applicable here AFAIK.

Nope, trying to keep the options as minimal as possible for this one. We can expand it later if someone asks or it.

ggetz · 2018-07-06T18:51:12Z

Source/Core/CoplanarPolygonGeometry.js

+        if (positions.length < 3) {
+            return;
+        }
+        var bs = BoundingSphere.fromPoints(positions);


Per the coding guide, I believe we avoid abbreviations.

ggetz · 2018-07-06T18:54:30Z

Source/Core/CoplanarPolygonGeometry.js

+
+        var attributes = new GeometryAttributes();
+
+        if (vertexFormat.position) {


Shouldn't we always have position?

Theoretically yes, but someone could technically define whatever they want for vertexFormat

ggetz · 2018-07-06T18:56:26Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+        Check.defined('positionsResult', positionsResult);
+        //>>includeEnd('debug');
+
+        var obb = OrientedBoundingBox.fromPoints(positions, obbScratch);


Same comment here about abbreviations.

ggetz · 2018-07-06T18:59:04Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+    var obbScratch = new OrientedBoundingBox();
+
+    // call after removeDuplicates
+    PolygonGeometryLibrary.projectTo2D = function(positions, positionsResult, normalResult, tangentResult, bitangentResult) {


Add a spec for this?

It's covered between CoplanarPolygonGeometry and CoplanarPolygonOutlineGeometry. We don't have specs for any of the other *GeometryLibrary static classes.

ggetz · 2018-07-06T19:16:43Z

@hpinkos Can you clarify what projectTo2D is doing in cases where the geometry is not coplanar, like in this case?

hpinkos · 2018-07-06T19:58:53Z

Can you clarify what projectTo2D is doing in cases where the geometry is not coplanar, like in this case?

projectTo2D is just an private helper function. We need to project the points to 2D for the polygon triangulation, but it does not alter the positions that are passed into the geometry

bagnell · 2018-07-06T20:46:03Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+        var planeYAxis;
+        var origin = Cartesian3.clone(center, scratchOrigin);
+        var normal;
+        if (min === xMag) {


This can probably be more simple. Once you find the vectors with the minimum and maximum length:

z = cross(x, y); // normal y = cross(z, x); // normalize x, y, and z

It doesn't matter if you use z or -z for the normal. It also doesn't matter if you use +/-x or +/-y as long as they are orthogonal in the plane.

bagnell · 2018-07-06T20:46:53Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+        var normal;
+        if (min === xMag) {
+            if (min !== 0) {
+                origin = Cartesian3.add(origin, xAxis, origin);


Why move the origin from the bounding box center?

It's because all the points are already in a plane and I need a new plane to project them onto =P
There's definitely a better way to do this. I hadn't given this logic any thought since I wrote it at our first code sprint down the shore. Thanks for pointing that out!

Oh no wait! It's because I want the intersection plane to be where the OBB box face is instead of at the center of the box.

bagnell · 2018-07-06T20:49:03Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+        for (i = 0; i < positions.length; i++) {
+            ray.origin = Cartesian3.clone(positions[i], ray.origin);
+
+            var intersectionPoint = IntersectionTests.rayPlane(ray, plane, scratchIntersectionPoint);


You don't need to intersect the ray with the plane. You just need this:

v = subtract(positions[i], origin); x = dot(xAxis, v); y = dot(yAxis, v);

I agree that's the case when the positions are actually coplanar. But I need it to work with positions that are mostly coplanar and to "try it's best" for a set of arbitrary points so I think I still need this?

Hmm, I guess you're right though, the results come out the same for both. Thanks!

hpinkos · 2018-07-06T22:02:34Z

Thanks @ggetz, @bagnell! Anything else?

bagnell · 2018-07-06T22:35:41Z

Source/Core/CoplanarPolygonGeometryLibrary.js

+            return;
+        }
+        var center = orientedBoundingBox.center;
+        var origin = Cartesian3.clone(center, scratchOrigin);


You don't need this anymore. Just subtract from center below.

bagnell · 2018-07-06T22:39:00Z

👍 Looks good to me, just that one last comment. @ggetz can merge when ready.

hpinkos · 2018-07-10T14:49:57Z

@ggetz ready

ggetz · 2018-07-10T15:40:54Z

👍 Thanks @hpinkos!

bmckilligan · 2018-07-24T20:48:03Z

how do things render for this example?

https://cesiumjs.org/Cesium/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=aaf8c76debb0e8ee19c4d65291bae289

hpinkos · 2018-07-24T20:50:06Z

@bmckilligan all fixed!

coplanar polygon

86e29d5

hpinkos mentioned this pull request Jul 3, 2018

KML renders incorrectly - vertical polygons #3349

Closed

Merge branch 'master' into space-polygon

526eb05

ggetz self-assigned this Jul 6, 2018

Merge branch 'master' into space-polygon

149cb35

ggetz requested changes Jul 6, 2018

View reviewed changes

bagnell reviewed Jul 6, 2018

View reviewed changes

cleanup

992c250

bagnell reviewed Jul 6, 2018

View reviewed changes

ggetz approved these changes Jul 9, 2018

View reviewed changes

hpinkos added 2 commits July 10, 2018 10:46

Merge branch 'master' into space-polygon [ci skip]

06c3e4c

cleanup

6c2f9be

ggetz merged commit 7151a21 into master Jul 10, 2018

ggetz deleted the space-polygon branch July 10, 2018 15:41


		var attributes = new GeometryAttributes();

		if (vertexFormat.position) {

CoplanarPolygonGeometry and CoplanarPolygonOutlineGeometry #6769

CoplanarPolygonGeometry and CoplanarPolygonOutlineGeometry #6769

Conversation

hpinkos commented Jul 3, 2018 • edited Loading

cesium-concierge commented Jul 3, 2018

hpinkos commented Jul 3, 2018

hpinkos commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Jul 6, 2018

hpinkos commented Jul 6, 2018

bagnell Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Jul 6, 2018

Choose a reason for hiding this comment

bagnell commented Jul 6, 2018

hpinkos commented Jul 10, 2018

ggetz commented Jul 10, 2018

bmckilligan commented Jul 24, 2018

hpinkos commented Jul 24, 2018

hpinkos commented Jul 3, 2018 •

edited

Loading

bagnell Jul 6, 2018 •

edited

Loading

hpinkos Jul 6, 2018 •

edited

Loading