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

Image expression #8684

Merged
merged 23 commits into from
Sep 19, 2019
Merged

Image expression #8684

merged 23 commits into from
Sep 19, 2019

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Aug 23, 2019

Launch Checklist

This PR implements a new Image expression type and image operator as discussed in #8052 particularly #8052 (comment)

  • briefly describe the changes in this PR
    • adds an image expression operator to allow for determining an image's availability before requesting it
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
    • will need a native port @mapbox/gl-native

Changelog
Add image expression operator which determines if a requested image is available in the map's style or not

@asheemmamoowala
Copy link
Contributor

cc @mapbox/map-design-team @mapbox/studio @kkaefer

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

@ryanhamley Just a few minor comments.

Question, do you plan to add support for default image that is not in a sprite sheet? Or, would it be handled via onStyleImageMissing callback?

Another question is that, what happens when developer removes image from a sprite sheet while worker is parsing tile? It looks like availableImages and images that are in sprite sheet could be out-of-sync and image operator would not fall-back to default value (e.g., when used in coalesce).

src/style-spec/expression/definitions/image.js Outdated Show resolved Hide resolved
src/style-spec/expression/definitions/image.js Outdated Show resolved Hide resolved
@ryanhamley ryanhamley marked this pull request as ready for review September 18, 2019 00:47
@@ -140,4 +140,3 @@ export default class ImageAtlas {

register('ImagePosition', ImagePosition);
register('ImageAtlas', ImageAtlas);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no change needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was automatically added by my editor. It's pretty standard to end files with a new line isn't it? Most of our other files have a trailing new line.

src/style-spec/expression/definitions/image.js Outdated Show resolved Hide resolved
src/style-spec/reference/v8.json Outdated Show resolved Hide resolved
@asheemmamoowala asheemmamoowala added this to the release-ristretto milestone Sep 18, 2019
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Sep 19, 2019

Question, do you plan to add support for default image that is not in a sprite sheet? Or, would it be handled via onStyleImageMissing callback?

@alexshalamov I'm adding an additional step in a follow-up PR to determine the first requested image in an expression (reading from left to right and assuming that the first image requested is the "most desirable" option) which evaluates to null and returning that image's name so that styleimagemissing will always work as expected.

Another question is that, what happens when developer removes image from a sprite sheet while worker is parsing tile? It looks like availableImages and images that are in sprite sheet could be out-of-sync and image operator would not fall-back to default value (e.g., when used in coalesce).

I think this is analogous to what happens if you asynchronously load an image with addImage. Before this PR, asynchronously loading an image will not cause it to appear in a tile that had requested the image previous to it being loaded. This PR does not change that behavior because it would require reloading tiles every time an image is added or removed, which likely would be distracting and probably a performance hit. I think the best way to deal with both of these situations is still an open question.

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