Add support for polygon holes to CZML by shunter · Pull Request #7991 · CesiumGS/cesium

shunter · 2019-07-10T17:50:36Z

Holes are specified as an array of arrays of positions. Polygon holes can be constant, interval-based, or defined as references. Using references allows for each vertex of the hole to be expressed as a sampled value. This is essentially the same limitations as the existing array of positions.

These changes also change the behavior of PolygonGraphics.hierarchy to always produce a PolygonHierarchy.

This makes the behavior more closely match the documentation. Previously, CzmlDataSource would shove a Cartesian array into the hierarchy property, and then all use sites had compatibility logic to adapt. For backwards compatibility, we preserve the existing behavior of allowing an array of positions to be set. The ability to set an array of positions was undocumented, though some Sandcastle examples do this. CzmlDataSource no longer sets the incorrect type of data into hierarchy, because we now need to handle holes.

Fixes #3826

Sample CZML syntax for a constant set of holes.

 {
    "polygon" : {
        "positions" : {
            "cartographicDegrees" : [
                -82.0, 40.8, 0,
                -83.0, 36.5, 0,
                -76.0, 35.6, 0,
                -73.5, 43.6, 0
            ]
        },
        "holes" : {
            "cartographicDegrees" : [
                [
                    -81.0, 40.0, 0,
                    -81.0, 38.2, 0,
                    -79.0, 38.2, 0,
                    -78.0, 40.8, 0
                ],
                [
                    -77.5, 36.7, 0,
                    -78.5, 37.0, 0,
                    -76.5, 39.6, 0
                ]
            ]
        }
    }
}

Existing Sandcastle examples for CZML polygons now demonstrate holes using constant, intervals, and references.

For backwards compatibility we preserve the existing behavior of allowing an array of positions to be set. This makes the behavior more closely match the documentation. The ability to set an array of positions was undocumented, though some Sandcastle examples do this. This removes the need for conversion in PolygonGeometryUpdater at the point where the data is used.

…ositionsPacketData

Use CzmlDataSource.load in most tests. Compare against the values from the packet data rather than copy/pasting the expected values.

…d using intervals.

cesium-concierge · 2019-07-10T17:50:39Z

Thanks for the pull request @shunter!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

shunter · 2019-07-10T18:04:40Z

As an additional side note I spent some effort on code coverage for CzmlDataSource. In particular, loading intervals of arrays of references was broken (pretty esoteric, admittedly)

mramato · 2019-07-10T18:26:47Z

Awesome, thanks @shunter. Will look at this ASAP.

emackey · 2019-07-10T18:31:52Z

Source/DataSources/PositionPropertyArray.js

     * @param {ReferenceFrame} referenceFrame The desired referenceFrame of the result.
-     * @param {Cartesian3} [result] The object to store the value into, if omitted, a new instance is created and returned.
-     * @returns {Cartesian3} The modified result parameter or a new instance if the result parameter was not supplied.
+     * @param {Cartesian3[]} [result] The object to store the value into, if omitted, a new instance is created and returned.


Should this say "array of objects" instead of "object"? And is it a breaking API change?

Not a breaking change, the documentation was wrong, not the code.

mramato · 2019-07-10T18:48:36Z

Just to clarify, PolygonGraphics.hierarchy.getValue in master would return a Cartesian3 array or a PolygonHierarchy depending on what the user set, right? And in this branch it will now always return a PolygonHierarchy? Technically the documentation said it was always a PolygonHierarchy but was wrong?

If the above is correct, I still think it's worth mentioning in CHANGES, even if we don't consider it a breaking change. Just treat it like a bug and state that it now always returns a PolygonHirearchy

mramato · 2019-07-10T18:56:41Z

That's the only question/comment I have. Another flawless PR, thanks @shunter

shunter · 2019-07-10T19:04:36Z

PolygonGraphics.hierarchy.getValue in master would return a Cartesian3 array or a PolygonHierarchy depending on what the user set, right?

It would store whatever value was set into it, without really caring what it was, which is what nearly all properties do.

And in this branch it will now always return a PolygonHierarchy? Technically the documentation said it was always a PolygonHierarchy but was wrong?

Yes, it was documented as a property of type PolygonHierarchy but the CZML loading didn't respect that. So the entities created by CZML did not match the documentation.

mramato · 2019-07-10T19:14:24Z

Thanks, I'll merge once green.

mramato · 2019-07-11T00:17:31Z

Thanks again @shunter! I couldn't find any "holes" in your work...

shunter added 12 commits July 8, 2019 10:04

Make code more consistent between processArrayPacketData and processP…

9a60e0d

…ositionsPacketData

Clean up test code.

4849b00

Use CzmlDataSource.load in most tests. Compare against the values from the packet data rather than copy/pasting the expected values.

Add support for polygons with holes to CzmlDataSource.

39412e5

Add additional tests

77dd4be

Add more tests. Fix incorrect behavior with reference arrays specifie…

f71183d

…d using intervals.

Add more tests.

7a721af

Update validation document.

e7964f6

Add more tests.

b211381

In PolygonHierarchy, holes needs to be an array of PolygonHierarchy.

5c74d5b

Add holes to CZML polygon Sandcastle examples.

b7f6389

fix typo

e252b1c

shunter added type - enhancement category - czml labels Jul 10, 2019

shunter requested a review from mramato July 10, 2019 17:50

shunter added 2 commits July 10, 2019 14:04

update CHANGES

b1f2fbb

fix typo

8e76bb0

emackey reviewed Jul 10, 2019

View reviewed changes

update CHANGES

342fc31

Regenerate validation document and assertions.

8de581d

shunter mentioned this pull request Jul 10, 2019

Add polygon holes AnalyticalGraphicsInc/czml-writer#168

Merged

mramato merged commit 254375b into master Jul 11, 2019

mramato deleted the polygonHoles branch July 11, 2019 00:17

hpinkos mentioned this pull request Sep 27, 2019

Polygon entity with callback property hierarchy crashes #8225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for polygon holes to CZML#7991

Add support for polygon holes to CZML#7991
mramato merged 16 commits intomasterfrom
polygonHoles

shunter commented Jul 10, 2019

Uh oh!

cesium-concierge commented Jul 10, 2019

Uh oh!

shunter commented Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

emackey Jul 10, 2019

Uh oh!

shunter Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

shunter commented Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

mramato commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

shunter commented Jul 10, 2019

Uh oh!

cesium-concierge commented Jul 10, 2019

Uh oh!

shunter commented Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

emackey Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

shunter Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

shunter commented Jul 10, 2019

Uh oh!

mramato commented Jul 10, 2019

Uh oh!

mramato commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants