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

Skip creating textures for empty atlases #2297

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

IvanSanchez
Copy link
Contributor

@IvanSanchez IvanSanchez commented Mar 23, 2023

Indirectly fixes most of the deprecation warnings from #2030.

By making the tile worker return undefined instead of empty 1x1 atlases, the main thread skips creating textures for those. Those empty textures were the source of most of the WebGL warnings, AFAICS.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@HarelM
Copy link
Collaborator

HarelM commented Mar 23, 2023

Thanks for taking the time to open this PR!!
If this is still not complete, please use a draft PR.
Otherwise, please add changelog and tests.
Also would be nice to simply not initialize the image atlas and glyph atlas, if this is not possible I would feel better if empty would be a calculated property (getter only) according to the state of the class.

@IvanSanchez IvanSanchez marked this pull request as draft March 23, 2023 15:21
@IvanSanchez IvanSanchez marked this pull request as ready for review March 23, 2023 15:28
@IvanSanchez
Copy link
Contributor Author

Initialization of the empty property of atlases should be fine now. I don't think it's possible to properly skip atlas initialization, as the worker code refers to the positions inside the (empty) atlases. Since this is my first foray into the MapLibre code, I'd rather make minimal changes.

Regarding tests, I'm unfamiliar with MapLibre's test scaffolding. I'm not sure if it's even possible to run an automated puppeteer/karma/whatever browser test on a headless Firefox instance and check for WebGL warnings in the console.

@IvanSanchez
Copy link
Contributor Author

...and of course the unit tests assume that there are atlas images and textures for every tile, despite the if (data.imageAtlas) { line in tile.ts 😠

file:///home/runner/work/maplibre-gl-js/maplibre-gl-js/src/render/draw_fill_extrusion.ts:77
            tile.imageAtlasTexture.bind(gl.LINEAR, gl.CLAMP_TO_EDGE);
                                   ^
TypeError: Cannot read properties of undefined (reading 'bind')

@IvanSanchez IvanSanchez marked this pull request as draft March 23, 2023 15:45
@HarelM
Copy link
Collaborator

HarelM commented Mar 23, 2023

There are browser tests, but I agree FF is problematic here. Unit tests would be enough at this point.

@IvanSanchez IvanSanchez marked this pull request as ready for review March 28, 2023 15:47
@IvanSanchez
Copy link
Contributor Author

The changes impact the behaviour of one specific render test: extrusion pattern with missing texture. The current expectation is a transparent image, but removing the texture (and running the shader with no bound texture) creates a black extrusion.

@HarelM Would this change in behaviour be OK? Otherwise it'd be possible to create an empty transparent texture just for this edge case.

@HarelM
Copy link
Collaborator

HarelM commented Mar 29, 2023

Can you fix the test so that I can see the change in the image?
I'm not sure I fully understand how it would look in this specific case.
Also maybe worth trying to find out when was this test added and the changes around it to maybe better understand the motivation behind this test and this behavior.

@HarelM
Copy link
Collaborator

HarelM commented Mar 29, 2023

The code you added is fairly simple and I don't see big issues with it.
There should be either failing unit tests that needs to be fixed as part of this PR or new unit tests that needs to be added - not just render tests.

@IvanSanchez
Copy link
Contributor Author

I'm not sure I fully understand how it would look in this specific case.

So the problematic render test is tests/fill-extrusion-pattern/missing. AFAICS, it checks that a faulty stylesheet (one referring to a pattern image that doesn't exist in the spritesheet) doesn't crash the library. It does call console.warn to inform about this, so I'm not worrying much about changing the behaviour (since IMO using a non-existing image pattern is undefined behaviour).

For reference, most of tests/fill-extrusion-pattern/ looks like:

image

...except for tests/fill-extrusion-pattern/missing, which currently is a blank image.

After my changes, that specific render test looks like:

image

...this difference happens because, before this PR, tiles with no image atlas would allocate a transparent 1x1 texture - drawing an extrusion pattern with an invalid image would use that transparent 1x1 texture.

Since this PR skips the creation of those empty textures, the shader uses no texture - and the texture sampler somehow defaults to solid black.

Hope that helps?

@HarelM
Copy link
Collaborator

HarelM commented Apr 4, 2023

Yes, it does! Sorry for the delay in response.
I don't think this edge case is interesting, but this is a breaking change so we need to document it in the changelog.

@HarelM
Copy link
Collaborator

HarelM commented Apr 4, 2023

Please add unit test to cover the new functionality (besides fixing the render tests).

Signed-off-by: Iván Sánchez Ortega <ivan@sanchezortega.es>
Signed-off-by: Iván Sánchez Ortega <ivan@sanchezortega.es>
Signed-off-by: Iván Sánchez Ortega <ivan@sanchezortega.es>
Signed-off-by: Iván Sánchez Ortega <ivan@sanchezortega.es>
Signed-off-by: Iván Sánchez Ortega <ivan@sanchezortega.es>
@@ -29,6 +30,9 @@
- Fix geolocate control permissions failure on IOS16 web view with fallback to `window.navigator.geolocation`
- _...Add new stuff here..._

### ⚠️ Note on missing images
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add that as part of the above item with a [Breaking] prefix, feel free to add the emoji to the other breaking items as well.

@HarelM
Copy link
Collaborator

HarelM commented Jun 28, 2023

Bummer, we didn't add it to version 3. :-(
Is there a way to keep this backward compatible somehow?
Also unit tests are still missing...

@handymenny
Copy link
Contributor

This is the PR that added tests/fill-extrusion-pattern/missing: mapbox/mapbox-gl-js#4687

I would say that trasparent texture is intended behaviour

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants