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

Make showTileBoundaries prefer a vector source #1395

Merged
merged 12 commits into from
Sep 10, 2022
Merged

Make showTileBoundaries prefer a vector source #1395

merged 12 commits into from
Sep 10, 2022

Conversation

jleedev
Copy link
Contributor

@jleedev jleedev commented Jul 26, 2022

Fixes #833

@jleedev
Copy link
Contributor Author

jleedev commented Jul 26, 2022

I don't know why the render test doesn't trip. Manual test:

Screenshot 2022-07-26 07 24 36

The render test doesn't trip because it's a raster maxzoom of 1 and a vector maxzoom of 14.

This adds a test with raster maxzoom of 17. Old behavior was to show tile boundaries for the raster source, and 0 kB. New behavior is to show tile boundaries and byte sizes for the vector tiles.

@jleedev jleedev marked this pull request as ready for review July 26, 2022 11:29
@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2022

Bundle size report:

Size Change: +52 B
Total Size Before: 204 kB
Total Size After: 204 kB

Output file Before After Change
maplibre-gl.js 195 kB 195 kB +52 B
maplibre-gl.css 9.03 kB 9.03 kB 0 B
ℹ️ View Details No major changes

@HarelM
Copy link
Collaborator

HarelM commented Jul 26, 2022

Can you please add tests and update the changelog.md file?

@jleedev
Copy link
Contributor Author

jleedev commented Jul 26, 2022

Done, done.

I note that https://github.com/maplibre/maplibre-gl-js/blob/main/CONTRIBUTING.md#changelog-conventions indicates to put changelog entries in the pull request.

@HarelM
Copy link
Collaborator

HarelM commented Jul 26, 2022

Any change for a unit test?

@jleedev
Copy link
Contributor Author

jleedev commented Jul 26, 2022

Could factor out the logic to an exported function in either painter.ts or draw_debug.ts, then invoke it with various Maps and assert that it returns the desired SourceCache.

The logic is currently inside a single 155-line function named render.

@HarelM
Copy link
Collaborator

HarelM commented Aug 1, 2022

Exporting the show tile boundaries to a different method/file and testing it would be great, thanks!

@HarelM
Copy link
Collaborator

HarelM commented Aug 21, 2022

Any updates on this?

@jleedev
Copy link
Contributor Author

jleedev commented Aug 21, 2022

I think I can find some time this week.

@HarelM
Copy link
Collaborator

HarelM commented Aug 29, 2022

Any updates on this? I would really like to push this forward...

@HarelM
Copy link
Collaborator

HarelM commented Sep 8, 2022

@jleedev any news?

src/render/painter.ts Outdated Show resolved Hide resolved
@jleedev
Copy link
Contributor Author

jleedev commented Sep 10, 2022

Thanks for the code review.

@HarelM HarelM merged commit cfbc39f into maplibre:main Sep 10, 2022
cns-solutions-admin pushed a commit to CNS-Solutions/maplibre-gl-js that referenced this pull request Sep 21, 2022
* Make showTileBoundaries prefer a vector source

Fixes maplibre#833

* Add render test

* Add changelog entry

* Add render test

* Add test for showTileBoundaries

* eslint fix

* typecheck fix

* pass Style & zoom instead of Painter
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.

showTileBoundaries fails to ignore raster sources as documented
2 participants