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

GeoJSON support #890

Merged
merged 27 commits into from
Jun 24, 2013
Merged

GeoJSON support #890

merged 27 commits into from
Jun 24, 2013

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jun 20, 2013

  1. Add GeoJsonDataSource which as the name implies is a DataSource object which supports GeoJSON. The only aspect of the specification that we do not currently support is holes in polygons. This is a limitation of the DynamicScene layer which must be corrected first (I'll be writing an issue for it).
  2. Hook it up to viewerDragDropMixin and the Cesium Viewer source query parameter.
  3. Add a ConstantProperty which makes it easy to specify static values for DynamicScene objects.
  4. Since GeoJSON does not include styling information, I provide user-modifiable default styling. People with an eye for such things may want to check it out and offer suggestions.

image

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

@shunter, you know you are dying to review this. 😉

@emackey and @hpinkos, you have the best eyes for style around here. Any comments on the default styling?

@@ -50,6 +52,10 @@ define([
window.alert(e);
});

function endsWith(str, suffix) {
return str.indexOf(suffix, str.length - suffix.length) !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work even if suffix is longer than str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The value of the second argument can be positive or negative and if suffix is longer it's impossible for this function to equal anything other than -1. However, your question brings up a good point, I don't need str.length - suffix.length because simply -suffix.length will work exactly the same.

@emackey
Copy link
Contributor

emackey commented Jun 24, 2013

Since you're asking for bike shed color recommendations, allow me to recommend Color.CYAN instead of the primary Blue you're using (along with rgba : [0, 255, 255, 25.5] for polygons). Generally, cyan survives poor-quality video links better than dark blue, and I find it easier on the eyes.

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

It's not bike shedding for things that actually matter. I'll try out CYAN. What about the opacity of the polygons? Does that look good to you (too opaque or too translucent?)

@emackey
Copy link
Contributor

emackey commented Jun 24, 2013

The opacity looks good. I don't think the casual observer will find a difference between 25 and 25.5, if you want to trim two bytes 😉

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

Okay, those changes are in. Are we happy with just using a Point primitive for points? Should we use a fancy placemark-like billboard instead?

@@ -50,6 +52,10 @@ define([
window.alert(e);
});

function endsWith(str, suffix) {
return str.indexOf(suffix, -suffix.length) !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

indexOf doesn't take a negative argument. From the ES5 spec, start is min(max(pos, 0), len), which means that passing -suffix.length is equivalent to passing 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MDN lied to me, which is where I was looking. But you are correct. I double-checked the official spec PDF just to be sure. I'll undo my previous changes.

@emackey
Copy link
Contributor

emackey commented Jun 24, 2013

The default point is fine, people can configure it. Taking a second look at the OSM globe, it's very cyan already. Let's switch to Color.YELLOW (255, 255, 0). Sorry for changing my mind after you coded it.

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

@emackey I made it yellow, and I agree it's the most clear. That being said, keep in mind that anyone using Cesium Viewer will get these default values. I think we need a set of good looking defaults for Cesium Viewer (and I don't know why these wouldn't match the general defaults). That's why I think a custom placemark and easy on the eyes coloring is important. Yellow is visible, but not very pretty.

@emackey
Copy link
Contributor

emackey commented Jun 24, 2013

Yeah, the goal of a default color is just to be visible-by-default in as many different situations as possible. You can't really change your defaults based on which imagery layer is selected etc, but you can make educated guesses that the OSM layer might be popular with GeoJSON users.

A custom placemark can be nice. The upside-down raindrop shape allows more of the target area to be seen. Would we use a raster image for this, or just code a shader?

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

We would just base encode a png into the source, just like we do with the Bing logo. If we go with the standard placemark change, I would also change the bitmap origin so that the botton of the placemark is the actual position in the file.

@emackey
Copy link
Contributor

emackey commented Jun 24, 2013

Sure, non-scalable raster images can be useful. You could also use a GLSL shader.

    float len = length(st + vec2(0.0, -2.0));
    float circle = smoothstep(0.80, 0.85, len);
    float curve = (1.0 - cos(st.t * PI * 0.5)) * 0.5 * 0.80;
    float lower = 1.0 - (1.0 - smoothstep(curve, curve + 0.05, abs(st.s))) * (1.0 - smoothstep(0.95, 1.0, abs(st.t - 1.0)));
    float inscription = smoothstep(0.30, 0.35, len);
    float val = (1.0 - circle * lower) * inscription;

    gl_FragColor = color * val;

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

Except we have no way to express such things in the DynamicScene layer and we'd also be burdening the video card with shading all of those things that could just easily be a single static texture.

On a related topic, I was also thinking we could incorporate the Cesium logo into the placemark somehow, like instead of a black hole in the center, have the logo.

var crs = geoJson.crs;
if (typeof crs !== 'undefined') {
if (crs === null) {
throw new DeveloperError('crs is null.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably these are RuntimeErrors, not DeveloperErrors, since they're caused by data loaded at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I almost never use RuntimeError so it didn't even occur to me. I suppose you could argue that it's a DeveloperError when using load and a RuntimeError when using loadUrl, but then we are really splitting hairs. I'll change them to RuntimeError.

throw new DeveloperError('Unknown crs name: ' + properties.name);
}

crsPromise = when(crsFunction, function(crsFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me what you're trying to do here. This looks like a no-op to me:

  • adapt crsFunction to make it a promise if it's not already one
  • when that promise resolves, make a new promise that is already resolved to the crsFunction value.

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

Okay, all review changes so far are in. Anything else?

@emackey
Copy link
Contributor

emackey commented Jun 24, 2013

Good with me 👍 @shunter please merge when ready.

}
for ( var property in other) {
if (other.hasOwnProperty(property)) {
if (this.hasOwnProperty(property) && (typeof this[property] === 'undefined')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check this.hasOwnProperty(property) here? Shouldn't you merge other[property] into this regardless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. I simplified it to:

for ( var property in other) {
    if (other.hasOwnProperty(property)) {
        this[property] = defaultValue(this[property], other[property]);
    }
}

id = finalId;
}
var dynamicObject = dynamicObjectCollection.getOrCreateObject(id);
dynamicObject.geoJson = geojson;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth being consistent between geoJson and geojson throughout.

1. GeoJSON is the format name.
2. geojson is the extension we use.
3. geoJson is the identifier casing.
var dynamicObject = createObject(geojson, dataSource._dynamicObjectCollection);

var coordinates = geometry.coordinates;
var positions = new Array(coordinates.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth factoring out this function to convert an array of coordinates to an array of positions?

@mramato
Copy link
Contributor Author

mramato commented Jun 24, 2013

Okay, I think we're close. Anything else?

@shunter
Copy link
Contributor

shunter commented Jun 24, 2013

Looks good, merging.

shunter added a commit that referenced this pull request Jun 24, 2013
@shunter shunter merged commit fd6d625 into master Jun 24, 2013
@shunter shunter deleted the geojson branch June 24, 2013 21:18
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.

3 participants