PolygonGraphics.hierarchy #2353
Conversation
Used mostly for documentation, but can be instantiated with positions and holes. Also deprecate `PolygonGraphics.positions` and add `PolygonGraphics.hierarchy` to replace it.
Update CHANGES. Clean up some doc. Make CzmlDataSource use `hierarchy` instead of `positions` (CZML still doesn't support holes).
|
Consider changing names so that rings can interpreted as holes or islands depending on position in hierarchy. |
I'm not against this change, since it seems to be fairly standard terminology. However, we already use positions/holes for the lower level and public |
|
I think this is fine, but I suggest we also keep |
|
The problem with keeping both is that it makes the API harder to use. We get away with this for I think we need to be careful about making incremental changes here and creating churn. If you want, I can close this PR and merge this branch into implicit properties and just have them all come in together (when it is ready); this way we can see the big picture rather than trying to make tough api decisions now. |
Conflicts: CHANGES.md
Conflicts: CHANGES.md
|
@pjcozzi can I get a ruling here; I'd like to get this into master because several branches depend on it. I'll go a long with whatever you want; we have bigger fish to fry. Just let me know. |
|
I rather not making this renaming decision in haste when we have much bigger things on the radar. Let's leave it as is. We can revisit later if needed. @bagnell can you review this? |
| @@ -532,12 +540,17 @@ define([ | |||
| } | |||
|
|
|||
| var options = this._options; | |||
| var positions = Property.getValueOrUndefined(polygon.positions, time, options.polygonHierarchy.positions); | |||
| if (!defined(positions)) { | |||
| var hierarchy = Property.getValueOrUndefined(polygon.hierarchy, time, this._scrach); | |||
There was a problem hiding this comment.
Did you mean to assign to _scrach too? Its always undefined.
There was a problem hiding this comment.
It's actually not needed at all. So I deleted the variable and parameter. Thanks. This is ready.
|
@mramato Just that one comment. The rest looks good. |
…oles PolygonGraphics.hierarchy
This can wait until after 1.6 instead of rushing it into master; I'll probably pull it into the
implicit-propertiesbranch first; to aid with other efforts.PolygonHierarchyto Core to make defining polygons with holes clearer.PolygonGraphics.positionswas deprecated and replaced withPolygonGraphics.hierarchy, which evaluates to aPolygonHierarchyinstead of an array of positions.GeoJsonDataSourcenow supports polygons with holes.ConstantPropertycan now hold any value; previously it was limited to values that implementedequalsandclonesfunctions, as well as a few special cases.PolygonHierarchy, which is still supported during the deprecation period. I'm still trying to determine the best way to do holes in CZML (which will most likely be a separate pull request unless anyone has any objections).PolygonGraphics.positionscreated byGeoJSONDataSourcenow evaluate to aPolygonHierarchyobject instead of an array of positions. This one is technically a breaking change I could probably jump through some additional hoops to avoid this if we think it's important; but it's an easy fix for anyone it may affect.The overall goal is to move exclusively to the
hierarchyproperty forPolygonGraphicsand have it always evaluate to aPolygonHierarchyobject. This simplifies the API for end users.@bagnell Some of the GeoJSON files we test with look worse when using holes. I think it may be a big on our end; I'll see if I can pull together some examples for you to look at.