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

Add support for polygon holes to CZML #7991

Merged
merged 16 commits into from
Jul 11, 2019
Merged

Add support for polygon holes to CZML #7991

merged 16 commits into from
Jul 11, 2019

Conversation

shunter
Copy link
Contributor

@shunter shunter commented Jul 10, 2019

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.

shunter added 12 commits July 8, 2019 10:04
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.
Use CzmlDataSource.load in most tests. Compare against the values from the packet data rather than copy/pasting the expected values.
@cesium-concierge
Copy link

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

shunter commented Jul 10, 2019

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

mramato commented Jul 10, 2019

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

@@ -104,8 +104,8 @@ define([
*
* @param {JulianDate} time The time for which to retrieve the value.
* @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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@mramato
Copy link
Contributor

mramato commented Jul 10, 2019

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

mramato commented Jul 10, 2019

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

@shunter
Copy link
Contributor Author

shunter commented Jul 10, 2019

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

mramato commented Jul 10, 2019

Thanks, I'll merge once green.

@mramato
Copy link
Contributor

mramato commented Jul 11, 2019

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

ba dum tss

@mramato mramato merged commit 254375b into master Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CZML support for polygons with holes
4 participants