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

Fix: issue #10653 Minimap go black when zoomed out #10680

Closed
wants to merge 1 commit into from

Conversation

Darshit42
Copy link

fixed the issue #10653 when we zoomed out minimap used to go black , now made a new variable clampedzoom to ensure minimap renderer zoom out is fixed so that it dont overflow and respect the boundaries

before

image

after

image

@tyrasd tyrasd changed the title Fix: issue #10653 Mnimaps go black when zoomed out Fix: issue #10653 Minimap go black when zoomed out Jan 15, 2025
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

It doesn't really fix the issue, unfortunately.

Also, have you thought this through? Can you please explain how this change is supposed to fix the issue in the first place?

@Darshit42
Copy link
Author

i think its due to background map as it was occuring in mapbox satellite and after this it wasnt occuring , i thought of fixing the issue by setting up a maximum limit for zoom out , before if we zoomed out of certain limit map would just stop rendering , i set the clampedzoom to be equal to maximum zoomout capacity whenever it crosses the limit , so in a way its fixed on up limit of zoom rather than overflowing , here is a screen recording you can cross check, i will try digging more , i will check files but if you can give a hint or guide me it will be very helpful.

Screen-Recording.1.1.1.mp4

@tyrasd
Copy link
Member

tyrasd commented Jan 16, 2025

Sure, here are some hints:

  • You're manipulating the value of the zoom level without adjusting the map projection itself. Some parts of the code are still using the original projection object which has the original zoom value: e.g. _tileOrigin is calculated based on _projection.scale(). Now the tile's x/y offsets are wrongly calculated in imageTransform because they partly use the clamped zoom value and partly the original one. You might got lucky in your test the cases, possibly because this error is small for zoom levels near 0.
  • The tiler module is already clamping zoom values to the available zoom range (see tiler.js line 37), no need to implement this again.
  • The main issue with the zoom clamping approach and the reason why we explicitly check for validZoom is the following: When using a clamped zoom level to "under-zoom" the map, one needs to draw a larger number of tiles to cover the same area. For example: If the layer is available only for zoom 12 to 18, and the minimap should be rendered for zoom 11, we need to request and draw 4 times as many tiles (because the zoom 12 tiles are only covering half the width and height of a zoom 11 tile that we would actually need). At zoom 11 this would not be a problem yet, but this number grows exponentially: at zoom 10, it would require 16 times as many tiles, at zoom 9 it would be 64 times as many, etc. – This means, we can only under-zoom a small amount of zoom levels until this becomes a serious problem.
  • So what we want is the following: allow the minimap to display tiles at zoom level 0 for tiles layers which only have tiles up to zoom level 1. But we don't want to render the millions of tiles a tile source with a minimum zoom of 12 would require to be rendered as zoom 0.
  • Here's one outline of an idea of a possible implementation: Introduce a property/flag in tile_layer.js that allows other modules to explicitly enable under-zooming by a certain specified number of zoom levels and pass this as an additional argument to validZoom(_zoom, allowedUnderZoomAmount). In map_in_map.js: set this number to a value of 1 or 2.

@tyrasd tyrasd added the map-renderer An issue with how things are rendered in the map label Jan 16, 2025
@Darshit42
Copy link
Author

Darshit42 commented Jan 17, 2025

@tyrasd ive made the changes you told me in this branch of my fork https://github.com/Darshit42/iD/tree/fixed-minimap-issue #10694

i double checked in all the maps and found that apart from open topo map background there is no error in any other map , can you confirm and tell me if there's some other file i need to change , i ran the tests and all of them passed , if there's no issue can i raise the pr??

(also there is another case , we can turn validZoom in background_source.js to always return true , but same problem persists, in open topo map background it shows different tiles when rendering )

@Darshit42 Darshit42 closed this Jan 21, 2025
tyrasd added a commit that referenced this pull request Jan 22, 2025
preventing minimap from blacking out "too soon" on low map zoom, fixes #10653. Closes #10694 and #10680
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map-renderer An issue with how things are rendered in the map
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants