Make image operator compatible with styleimagemissing event #8775

ryanhamley · 2019-09-19T04:13:43Z

Launch Checklist

briefly describe the changes in this PR
- this PR adds a check after the image operator returns null to force the styleimagemissing event to still fire
- this check assumes that the first image requested in the expression (reading from left to right) is the most desired image and uses this heuristic to select which image to fire the event for
manually test the debug page

ryanhamley · 2019-09-20T05:01:12Z

This is fairly close to being a working solution.

I'm not sure how to fix the Flow error without casting to any.

The two unit tests are caused because actor really can't be null anymore when creating a class that extends WorkerSource such as GeojsonWorkerSource but the unit tests call new GeojsonWorkerSource(null....) and the code attempts to call actor.send which it can't. I'm not sure how to fix that. I haven't been able to successfully mock actor.

The render test is very odd. I don't see that failure on my local machine. I get 100% passing render tests.

ryanhamley · 2019-09-20T23:19:31Z

This isn't really something that can be unit tested. We should eventually add some end-to-end tests which would be more appropriate for testing this. I have done extensive manual testing in the browser however.

All of these work as expected - a valid image produces the correct visual and an invalid image causes styleimagemissing to fire. In the case of a coalesce statement, styleimagemissing fires for the the first image requested (in this case "foo").

"icon-image": ["image", "monument-15"],
"icon-image": ["image", ["concat", ["get", "icon"], "-15"]],
"icon-image": ["get", "icon"],
"icon-image": ["coalesce", ["image", "foo"], ["image", "bar"], ["image", "monument-15"], ["image", "baz"]],
"icon-image": {
    "type": "identity",
    "property": "icon"
},

This has been tested for icon-image and *-pattern properties.

vakila · 2019-09-20T23:52:03Z

From the overview explanation from @ryanhamley this seems good, we were just saying that it would be nice to have tests for this, but it's really difficult to create unit/integration tests for this given our current testing infrastructure. So instead it would be helpful to at least have a debug page with a style using this operator to verify that the styleimagemissing event is being fired as expected, so let's add that as a launch checklist todo.

vakila

Thanks for adding the debug page @ryanhamley! Page looks good to me in Firefox & Chrome on Ubuntu, I also tested that it gives the "Image 'foo' could not be loaded" error if needed. I'm not super familiar with the expression parsing code but the changes look good from where I'm standing.

One question, does it make sense to have the actor stubbing in the tests be part of this PR or make it a separate one (since it seems like it doesn't have to do with this feature)? Not a big deal just wondering

ryanhamley · 2019-09-23T22:41:49Z

does it make sense to have the actor stubbing in the tests be part of this PR or make it a separate one (since it seems like it doesn't have to do with this feature)? Not a big deal just wondering

The stub is actually required to get the tests passing. If icon-image or *-pattern is defined, then actor.send will always be fired so that we can complete the roundtrip back to the main thread and fire styleimagemissing. These tests just passed null as actor previously so tests were failing with an error that they couldn't call send of undefined.

ahk · 2019-09-24T00:33:25Z

There is one aspect of the styleimagemissing event I was confused by at first. If you have a coalesce and it contains data-driven properties, but these are superseded by an earlier constant expression, the event will only fire once regardless of the fact that there may be per-feature failures along the chain of coalesce.

I don't think this is a bad design choice, but I had to think about it a bit. Maybe I am just learning about a quirk of coalesce we see in other places, but I do think that if a coalesce contains both data-driven and non-data-driven properties, and it fails, it's arguable whether we should see a failure per feature or not.

Part of me thinks that we should get an event per feature if we use data-driven expressions anywhere in the coalesce and it fails? To handle this though I think ideally we'd want to see "tried exprA, exprB, exprC, ..." as part of a coalesce (including the failing data-driven values for each failing feature so you can find the feature referenced). Cons: that's a lot of work, likely over-engineered, and changes the interface of the event.

Fundamentally I wish we had a way to show or refer to individual features in the error console for a given coalesce that is data-driven and also failing on some instances.

Is this a concern we've had before with coalesce? Do we have some other solutions we've used to help make debugging this easier when a coalesceing style has per-feature errors?

src/data/bucket/symbol_bucket.js

ahk · 2019-09-24T00:58:09Z

src/style-spec/expression/types/resolved_image.js

    }

    toString(): string {
        return this.name;
    }

-    static fromString(imageName: string): ResolvedImage {
-        return new ResolvedImage(imageName);
+    static fromString(options: ResolvedImageOptions): ResolvedImage {


Nit: Why do we have fromString if it doesn't accept strings, and the plain constructor could be used with the same arguments?

Hmm, that's a good point. It's used in other places where it might be less straightforward to use the constructor though (https://github.com/mapbox/mapbox-gl-js/blob/master/src/style-spec/expression/definitions/coercion.js#L105). I'm inclined to leave this as is though because it follows the pattern of other expression types and this type and operator will likely be expanded significantly in the future as we add more image manipulation operators.

ryanhamley · 2019-09-24T03:10:30Z

@ahk I'm not sure if we've had larger discussions about changing the way coalesce emits warnings for failed resolutions. One issue I see with per-feature warnings is that it would change the way coalesce works and be somewhat inconsistent. Would we evaluate every sub-expression, return the first that isn't null and emit warnings for the rest? If not, the number of warnings you get would be dependent on when coalesce returns a non-null value. In either case, I think this is a significantly larger discussion that doesn't relate directly to this PR.

The way that this PR was implemented, we assumed a heuristic in which the first (left-most) image that was requested was assumed to be the most desirable one, therefore the one we emit a warning for. This behavior is analogous to the way a browser evaluates a font stack. I think it makes sense and works well. We could revisit it in the future if we get a lot of feedback on this issue.

…8793)

asheemmamoowala · 2019-09-24T04:55:05Z

src/data/bucket/symbol_bucket.js

+        // this allows us to fire the styleimagemissing event if image evaluation returns null
+        // the only way to distinguish between null returned from a coalesce statement with no valid images
+        // and null returned because icon-image wasn't defined is to check whether or not iconImage.parameters is an empty object
+        const hasIcon = iconImage.value.kind !== 'constant' || !!iconImage.value.value || Object.keys(iconImage.parameters).length > 0;


Part of this comment explains the high-level reasoning for retuning an object from the image expression. That should be documented in the code for that expression, or expressly stated in the PR description (its not too late to do that yet).

This comment seems less of a clarification of what the code does here. Maybe replace it with:

// `iconImage.parameters` distinguishes between an unspecified `icon-image` property and specified but not found value. Check it here to trigger styleimagemissing events as appropriate

alexshalamov

@ryanhamley found few issues while porting to native, please check if comments are valid.

alexshalamov · 2019-11-01T14:28:50Z

src/data/bucket/pattern_bucket_features.js

+            min = min && min.name ? min.name : min;
+            mid = mid && mid.name ? mid.name : mid;
+            max = max && max.name ? max.name : max;


@asheemmamoowala @ryanhamley If min is null => patterns[null] = true?

Wasn't null a possible result here even before this change?

I can't remember exactly why I changed this now off the top of my head, but I think the ternary operator might have been more of a defensive guard. I'm not actually sure that min can ever be null at this point because any value supplied to *-pattern properties will be coerced to a ResolvedImage type. If you tried to just do line-pattern: null or something, you'd get a type error before it got to this point. But maybe there's a pathway I'm not thinking about/a better way to handle this to ensure we don't get patterns[null].

alexshalamov · 2019-11-01T14:29:52Z

src/style-spec/expression/definitions/coalesce.js

+            // we need to keep track of the first requested image in a coalesce statement
+            // if coalesce can't find a valid image, we return the first image name so styleimagemissing can fire
+            if (arg.type.kind === 'image' && !result.available) {
+                if (!requestedImageName) requestedImageName = arg.evaluate(ctx).name;


@ryanhamley @asheemmamoowala arg is already evaluated on line 60

I think this is an oversight which can be corrected

alexshalamov · 2019-11-01T14:37:25Z

src/style-spec/expression/definitions/coalesce.js

            result = arg.evaluate(ctx);
+            // we need to keep track of the first requested image in a coalesce statement
+            // if coalesce can't find a valid image, we return the first image name so styleimagemissing can fire
+            if (arg.type.kind === 'image' && !result.available) {


@ryanhamley Why type of arg is checked? I think it should it be type of coalesce itself and then check if evaluated result is an image.

Changing the check to if (result && result instanceof ResolvedImage && !result.available) seems to work fine. Is that what you had in mind?

ryanhamley added 8 commits September 18, 2019 21:06

Partial implementation of styleimagemissing

30ac880

Cleanup

273b146

Return object from Image operator

b46932f

Allow coalesce to work with Image object

86bcb92

Fix lint

63d2ea4

Clean up code

9ae1733

mostly fix the tests

2f6721c

Fix expression tests

181fea9

ryanhamley added 5 commits September 20, 2019 11:19

Fix Flow issue

2d48c39

Fix lint issue

1a5b2ab

Mock actor in worker source tests

13cfbf7

Fix check for undefined icon-image property

cd63a78

Enable styleimagemissing with pattern properties

bf9b06e

ryanhamley requested a review from asheemmamoowala September 20, 2019 23:20

kkaefer added this to the release-ristretto milestone Sep 23, 2019

ryanhamley requested a review from kkaefer September 23, 2019 19:15

Add debug page

c936b93

vakila reviewed Sep 23, 2019

View reviewed changes

ahk approved these changes Sep 24, 2019

View reviewed changes

Make if statement more legible

a08d256

ryanhamley merged commit 68cfea9 into master Sep 24, 2019

ryanhamley deleted the image-and-styleimagemissing branch September 24, 2019 04:09

ryanhamley pushed a commit that referenced this pull request Sep 24, 2019

Make image operator compatible with styleimagemissing event (#8775)

3406bd7

ryanhamley pushed a commit that referenced this pull request Sep 24, 2019

Make image operator compatible with styleimagemissing event (#8775) (#…

ad86cf2

…8793)

asheemmamoowala reviewed Sep 24, 2019

View reviewed changes

tristen mentioned this pull request Sep 24, 2019

Drop warning thrown when image equates to "" and image expression type is not used #8800

Closed

kkaefer mentioned this pull request Oct 11, 2019

Port Image Expressions mapbox/mapbox-gl-native#15800

Closed

alexshalamov reviewed Nov 1, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make image operator compatible with styleimagemissing event #8775

Make image operator compatible with styleimagemissing event #8775

ryanhamley commented Sep 19, 2019

ryanhamley commented Sep 20, 2019

ryanhamley commented Sep 20, 2019

vakila commented Sep 20, 2019

vakila left a comment

ryanhamley commented Sep 23, 2019

ahk commented Sep 24, 2019

ahk Sep 24, 2019

ryanhamley Sep 24, 2019

ryanhamley commented Sep 24, 2019

asheemmamoowala Sep 24, 2019

alexshalamov left a comment

alexshalamov Nov 1, 2019

ryanhamley Nov 2, 2019 •

edited

Loading

alexshalamov Nov 1, 2019

ryanhamley Nov 1, 2019

alexshalamov Nov 1, 2019

ryanhamley Nov 2, 2019

Make image operator compatible with styleimagemissing event #8775

Make image operator compatible with styleimagemissing event #8775

Conversation

ryanhamley commented Sep 19, 2019

Launch Checklist

ryanhamley commented Sep 20, 2019

ryanhamley commented Sep 20, 2019

vakila commented Sep 20, 2019

vakila left a comment

Choose a reason for hiding this comment

ryanhamley commented Sep 23, 2019

ahk commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley commented Sep 24, 2019

Choose a reason for hiding this comment

alexshalamov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley Nov 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley Nov 2, 2019 •

edited

Loading