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

Handle missing maki icons in GeoJson load #4452

Merged
merged 1 commit into from
Oct 19, 2016
Merged

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Oct 19, 2016

Instead of failing the entire load when a maki icon is bad or missing, just create a pin with the icon color instead.

Replaces #2889

Longer term, we probably want a good strategy for missing/bad images throughout the code base. Maybe we have the same "bad image" image that we end up using everywhere?

Instead of failing the entire load when a maki icon is bad or missing, just
create a pin with the icon color instead.

Replaces #2889
@hpinkos
Copy link
Contributor

hpinkos commented Oct 19, 2016

@mramato This change looks good to me. I think we should use the same strategy throughout the Entity API at least. Does KML use icons?

And if you do something like this where the url or the maki name is invalid, it'd be nice to have the pins show up anyway.

var url = Cesium.buildModuleUrl('Assets/Textures/maki/grocery.badpath');
var groceryPin = Cesium.when(pinBuilder.fromUrl(url, Cesium.Color.GREEN, 48), function(canvas) {
    return viewer.entities.add({
        name : 'Grocery store',
        position : Cesium.Cartesian3.fromDegrees(-75.1705217, 39.921786),
        billboard : {
            image : canvas.toDataURL(),
            verticalOrigin : Cesium.VerticalOrigin.BOTTOM
        }
    });
});

//Create a red pin representing a hospital from the maki icon set.
var hospitalPin = Cesium.when(pinBuilder.fromMakiIconId('invalid-name', Cesium.Color.RED, 48), function(canvas) {
    return viewer.entities.add({
        name : 'Hospital',
        position : Cesium.Cartesian3.fromDegrees(-75.1698606, 39.9211275),
        billboard : {
            image : canvas.toDataURL(),
            verticalOrigin : Cesium.VerticalOrigin.BOTTOM
        }
    });
});

If you don't want to tackle this now, could you write up an issue so we can address this and make it consistent everywhere?

@mramato
Copy link
Contributor Author

mramato commented Oct 19, 2016

And if you do something like this where the url or the maki name is invalid, the when.all never resolves

It's not supposed to resolve, it rejects. This is the desired/expected behavior (of course all of our examples should properly handle rejected promises, which they currently don't. I'll write up an issue for that instead.

The default image stuff has far reaching implications, throughout both the entity and primitive layers, so we definitely shouldn't try and tackle that now.

@hpinkos
Copy link
Contributor

hpinkos commented Oct 19, 2016

It's not supposed to resolve, it rejects.

Yeah yeah, just realized that a second before you replied. haha

Looks great to me! 👍 Thanks

@hpinkos hpinkos merged commit fd90be0 into master Oct 19, 2016
@hpinkos hpinkos deleted the handle-missing-image branch October 19, 2016 15:57
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 19, 2016

Should CHANGES.md be updated in master?

mramato added a commit that referenced this pull request Oct 19, 2016
Fixes #4438

Also updated changes from #4452, since I forgot to do that originally.
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