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

Adding render tests for image fallback with feature constant expressions and addImage operation #11391

Merged
merged 2 commits into from Mar 10, 2022

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Jan 12, 2022

As pointed out by @alexshalamov, the tests added in #11049 and #11359 contain no feature constant expressions, but are all using ["image", ["get", "icon"]]. So in this PR I add two tests that use constant values, i.e. ["image", "fav-bicycle-18"]

Since data and constant expressions are evaluated in different codepaths, these additional tests should improve test coverage in JS and Native and ensure that image fallback expressions work in all expected conditions.

I also add two tests ensuring image fallback works with sprites added through addImage (as opposed to images in the spritesheet) and make a small improvement to organization.

Launch Checklist

  • briefly describe the changes in this PR
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@SnailBones SnailBones added the skip changelog Used for PRs that do not need a changelog entry label Jan 12, 2022
@SnailBones SnailBones changed the title Adding tests for image fallback with feature constant expressions Adding render tests for image fallback with feature constant expressions Jan 12, 2022
@SnailBones SnailBones changed the title Adding render tests for image fallback with feature constant expressions Adding render tests for image fallback with feature constant expressions and addImage operation Jan 14, 2022
@SnailBones
Copy link
Contributor Author

Added two more tests: /add-image and feature-state/add-image to test with the addImage operation. GL JS tests fail unless the layer is moved to addLayer, this suggests that the map doesn'trerender on addImage.

This is not an issue in Native, where both these tests pass, as do variants without using addLayer.

These tests pass even before this fix and without triggering a breakpoint in the second VertexVectors overload here.

"wait"
],
[
"addLayer",
Copy link
Contributor

Choose a reason for hiding this comment

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

addLayer will trigger tile re-layout. Would it be possible to move layer definition to style definition? List of operations may be:

  1. wait
  2. add image
  3. set feature state

Test will verify that updated set of images is used for property evaluation in the feature-state code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @alexshalamov!

I tried the test with the layer definition in the style definition as you describe (in place of the current "layers": []. This causes the test to pass in GL Native but fail in GL JS. I think this is from an issue unique to render tests in GL JS.

I see two ways around this:

  • resolve this issue in GL JS to get consistent render test behavior
  • add the test in GL Native only (and perhaps open an issue to address this in GL JS at a later date).

I think the latter in the simplest approach for now, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested out the changes you suggest, and they still don't seem to use the intended updateVertexVectors function in Native.

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Per chat with @SnailBones approving this PR as even though it's not a strict requirement for gl-native, it still has a fair amount of render test coverage increase.

@karimnaaji karimnaaji merged commit daf0967 into main Mar 10, 2022
@karimnaaji karimnaaji deleted the aidan/image-fallback-constant-test branch March 10, 2022 21:32
ahocevar pushed a commit to ahocevar/mapbox-gl-js that referenced this pull request Mar 23, 2022
…ons and `addImage` operation (mapbox#11391)

* Added tests for image fallback with constant expressions

* Adding tests for 'addImage' operation and rearranging feature-state tests
@SnailBones SnailBones restored the aidan/image-fallback-constant-test branch April 28, 2022 21:32
@SnailBones SnailBones deleted the aidan/image-fallback-constant-test branch April 28, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants