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 custom properties to CZML files #5105

Merged
merged 31 commits into from
Mar 27, 2017
Merged

Add custom properties to CZML files #5105

merged 31 commits into from
Mar 27, 2017

Conversation

shunter
Copy link
Contributor

@shunter shunter commented Mar 14, 2017

Includes and replaces #3548. See #3548 for additional commentary. Thanks to @RacingTadpole and @kring for starting this, and sorry for the lengthy delay.

This builds on their work to add detection of the type of custom properties, which enables sampled custom properties that work just like built-in properties. I also added custom properties to the validation document.

Related czml-writer PR (I will merge that after this goes in) - AnalyticalGraphicsInc/czml-writer#129

@shunter
Copy link
Contributor Author

shunter commented Mar 14, 2017

One outstanding item to consider - the change mentioned in #3548 which makes GeoJSON properties into ConstantProperty instances. I am still considering whether we should add valueOf and toString to ConstantProperty to hide their property nature.

@RacingTadpole
Copy link
Contributor

Thanks heaps @shunter. I've checked this in TerriaJS and it all works, so I'm very happy with your updates! :-)

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 18, 2017

@kring or @mramato do you want to look at this?

@mramato
Copy link
Contributor

mramato commented Mar 20, 2017

I plan on looking at this ASAP.

@shunter
Copy link
Contributor Author

shunter commented Mar 20, 2017

Note that this PR includes #5118 to fix deployment on this branch. Merge that PR first.

CHANGES.md Outdated
@@ -21,6 +22,7 @@ Change Log
* Fixed a bug in `ModelAnimationCache` causing different animations to reference the same animation. [#5064](https://github.com/AnalyticalGraphicsInc/cesium/pull/5064)
* Fixed a bug that caused an exception in `CesiumInspectorViewModel` when using the NW / NE / SW / SE / Parent buttons to navigate to a terrain tile that is not yet loaded.
* Added support for an orthographic projection in 3D and Columbus view. Set `projectionPicker` to `true` in the options when creating a `Viewer` to add a widget that will switch projections. [#5021](https://github.com/AnalyticalGraphicsInc/cesium/pull/5021)
* Added support for time-varying properties in CZML [#3162](https:/github.com/AnalyticalGraphicsInc/cesium/issues/3162).
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated and corrected.

@mramato
Copy link
Contributor

mramato commented Mar 20, 2017

Awesome work here @shunter. I knew I'd win that war of attrition if I held out long enough 😈

I did an initial look over the code and nothing jumps out at me except for the GeoJSON/KML issue (but I'll do another pass). I don't have a good answer yet so this is just me thinking out loud.

The GeoJSON change, while minimal, seems to be a fairly significant breaking change. In isolation, I am okay with it, but it brings up an issue regarding KML. KML currently dumps some properties into Entity.kml as an instance of KmlFeatureData, it seems odd to have GeoJSON properties be ConstantProperty while KML properties retain their raw values. Ultimately I'm okay with this inconsistency because because they are two different data sources and the KmlFeatureData stuff is not really part of the spec like GeoJSON properties are and instead is a custom object used to just avoid losing data that was in the kml doc. Do you think it's okay to just leave KML as is for now? I'm leaning towards yes.

I guess the fundamental question is, did we change the GeoJSON behavior just because it was using properties or do we feel that any custom data should be in Entity properties now? What does this mean for KML?

As for valueOf and toString. I go back and forth on it. My concern with valueOf is that it can lead developers to write "lazy" code that works for constant values but not for anything else, which ultimately leads to weird problems down the line. (leaving the time parameter off of getValue for constant property does the same thing and we definitely do that in a few examples). On the other hand, introducing valueOf would probably eliminate much of the breakage this change would introduce and would be useful in cases where devs are using ConstantProperty exclusively (which I'm guessing is a lot of use cases). It also makes the properties more symetrical, since you can define a ConstantProperty value by assigning the primitive type anyway. So I think my answer here is yes, let's add valueOf and toString.

@mramato
Copy link
Contributor

mramato commented Mar 20, 2017

I'll also add my own apology for this not being done years ago. Especially since it's only ~100 lines of code in CZMLDataSource (albeit @shunter did a bunch of PropertyBag work for models that makes this possible)

@shunter
Copy link
Contributor Author

shunter commented Mar 22, 2017

OK, I added valueOf and toString to ConstantProperty. valueOf is really only useful for number-valued properties, but it's better than nothing.

I also added a sandcastle example that shows some ideas for how custom properties, specifically time-varying ones, can be useful. Unsurprisingly, it starts to look a bit like Scalars in STK Components.

@shunter
Copy link
Contributor Author

shunter commented Mar 22, 2017

Also, I know we've discussed this for years, but to make a note of it for the future - custom properties will be a key part of computed time-varying strings, using some kind of interpolation syntax, e.g. for data display-like labels, such as showing the speed of an object as a label attached to it.

@mramato
Copy link
Contributor

mramato commented Mar 27, 2017

Spoke to @shunter offline and he agrees that keeping KML the way it is is the right choice for now.

The Sandcastle example could probably be a little better from a viz standpoint, but that can be improved in future PRs.

Thanks @RacingTadpole and @shunter and sorry again for this taking so long to get in.

@mramato mramato merged commit 1c15bbe into master Mar 27, 2017
@mramato mramato deleted the custom_czml_properties branch March 27, 2017 23:47
@RacingTadpole
Copy link
Contributor

Awesome, thanks guys!

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 27, 2017

Nice! This is Cesium forum post worthy. Can someone post it?

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.

5 participants