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

User settable GeoJSON defaults #2167

Closed
mramato opened this issue Oct 1, 2014 · 6 comments
Closed

User settable GeoJSON defaults #2167

mramato opened this issue Oct 1, 2014 · 6 comments
Assignees

Comments

@mramato
Copy link
Contributor

mramato commented Oct 1, 2014

The ability to set default properties on GeoJSON features has been a hot topic on the mailing list. There are some subtleties that make it tricky. Me and @kring started to discuss them in #2162. To summarize that conversation:

  1. The simplest thing to do would be do expose default PolygonGraphics, BillboardGraphics, and PolylineGraphics instances and be done with it. However this is limited, for example it wouldn't be possible to use PointGraphics instead of BillboardGraphics or WallGraphics instead of PolylineGraphics. This limitation may be no big deal (since you could fall back to the load and loop approach if you wanted to do that).
  2. Another approach would be ok to expose simple properties for the defaults. For example, symbol=undefined, color=Color.ROYALBLUE, and size=48. Just turn each of these into properties on GeoJsonDataSource. Definitely limited, but might be enough for what people want.
  3. A third approach is to expose properties on GeoJsonDataSource which are the default simplestyles for each feature type. So you check for each style property on the feature, if it's not there you check for the same one on the default for that feature type, and if it's not there either you use the hard-coded default as you're doing right now. Simple and convenient.

The problem with the last two ideas is that it doesn't account for properties that only Cesium has. For example, what if we wanted to put a default NearFarScalar on the billboard. How would the user easily turn that off (or even know it was on)? I find 3 the most elegant, but I think it may be too limiting. 1 might be the best approach, since it's the least limiting and will most closely match the loaded result. We're open for other ideas as well.

@kring
Copy link
Member

kring commented Oct 1, 2014

The problem with the last two ideas is that it doesn't account for properties that only Cesium has. For example, what if we wanted to put a default NearFarScalar on the billboard.

I don't think this is a problem; certainly not a serious one. 3 is easy to implement, lets users do a large swath of the things they want to do, and is easy to understand. Users can always loop over the entities after load if they need to do something more unusual. We can also extend this in non-breaking ways in the future if necessary. For example, 1 can be used as the source of the defaults that would be hard-coded in 3. It's also easy to imagine adding a callback that is invoked during entity creation. The callback would be given the GeoJSON feature plus the entity that GeoJsonDataSource created to represent it (using the feature's simplestyles plus the simplestyles defaults), and the user could easily make changes to the entity right there.

Regarding your specific example of a default NearFarScalar value: I think it's reasonable to think of this as an option for how GeoJSON is shown in Cesium, rather than a part of the default style. And if we present it that way, there's no conflict with simplestyles-based defaults, it's just a separate property on GeoJsonDataSource.

In short, I think letting users specify default styles in the exact same way they specify per-feature styles makes a ton of sense, even if we eventually provide other things as well.

@mramato
Copy link
Contributor Author

mramato commented Oct 1, 2014

Just so we are clear, you are advocating 3 as your preferred solution in the short tem?

@kring
Copy link
Member

kring commented Oct 1, 2014

Just so we are clear, you are advocating 3 as your preferred solution in the short tem?

Yes :)

@bposselt
Copy link

bposselt commented Oct 1, 2014

I was one of the people asking about this feature on the mailing list. I also like option 3. I don't think its limitations wrt Cesium-specific features are too big of a hurdle; someone loading GeoJson probably isn't looking for the advanced features of Cesium. (I'm not, anyway.)

@mramato
Copy link
Contributor Author

mramato commented Oct 1, 2014

Okay. I really liked 3 as well, my concerns were only in their limitations. I'll open a PR soon with the change. (though it probably won't make it into today's release).

@mramato
Copy link
Contributor Author

mramato commented Nov 5, 2014

I just opened #2256 to handle this (mostly what we agreed on with some tweaks to make the options more Cesium-friendly), let me know what you guys think.

@mramato mramato closed this as completed Nov 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants