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

Image layer disappears while zooming #3010

Closed
indus opened this issue Aug 16, 2016 · 5 comments · Fixed by #3222
Closed

Image layer disappears while zooming #3010

indus opened this issue Aug 16, 2016 · 5 comments · Fixed by #3222

Comments

@indus
Copy link
Contributor

indus commented Aug 16, 2016

mapbox-gl-js version: v0.22.0

Steps to Trigger Behavior

  1. open "Add an image" example
  2. zoom in and out arround the raster overlay

Expected Behavior

the raster overlay is visibile at all zoom levels

Actual Behavior

the raster overlay disappears at certain zoom levels

v.0.21.0 works as expected.

@jfirebaugh
Copy link
Contributor

This is a regression from #2667. @anandthakker, can you please take a look?

@anandthakker
Copy link
Contributor

anandthakker commented Sep 1, 2016

Took an initial look. The problem was introduced because of the inversion of TilePyramid (now SourceCache) and Sources. Here's what I think is happening:

ImageSource (and VideoSource) both work by yielding a single valid tile, using that to hold the image/video data. This was true before #2667 as well. However, when inverting the SourceCache/Source dependency, I moved the responsibility for listing the currently-visible tile coordinates (getVisibleCoordinates) from Source to SourceCache. (Before this, it was the responsibility of each Source, but pyramid-based sources delegated to TilePyramid#getRenderableIds.)

What's happening now is that the SourceCache is deciding which tiles are renderable and asking the ImageSource for them. Because some (or even most) of the image is currently getting rendered outside of the (non-clipped) tile, when the map is zoomed in, the single tile responsible for holding the image may end up outside of the view, so that SourceCache no longer includes it in the list of renderable tiles.

Possible ways to solve:

  1. Modify the Source interface to enable sources to provide a getVisibleCoordinates: Transform => Array<TileCoordinate> method. I don't love this: it feels like a hack, and it really seems like transform => visible tiles ought to be a straightforward geometry calculation.
  2. Choose the "one tile" that houses the image/video data more carefully, using a tile that fully contains the image. This way, if the image's coordinates are in the view, the the "one tile" should be included as a renderable ID. This is pretty clean, but it doesn't solve the problem of zooming out: if the map's zoom level goes below that of the "one tile", then SourceCache won't request the tile, because there's no concept of "underzooming" (true?).
  3. Change ImageSource/VideoSource to actually chop up the image into multiple, clipped tiles, so that it can just populate the ones that are being requested, as needed. I'm not familiar enough with the GL buffer stuff that's happening here to judge whether this is a reasonable thing to do or not. It does seem like it might get tricky for the video source...

@jfirebaugh @lucaswoj thoughts?

@lucaswoj lucaswoj added this to the Chattanooga milestone Sep 1, 2016
@jfirebaugh
Copy link
Contributor

I think ImageSource and VideoSource are fundamentally not tile-based sources and it's awkward to treat them as such (was before #2667, even more so now). I'm most interested in solutions that take "tiles" out of the equation for these source types.

@anandthakker
Copy link
Contributor

I certainly agree in principle / in the long run, but it does seem like this would require some pretty invasive changes in painter / rendering code. Admittedly, I'm very unfamiliar with that part of the codebase, so perhaps this is just my ignorance talking.

@lucaswoj
Copy link
Contributor

I captured some thoughts about refactoring per @jfirebaugh's comment above in #3186

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

Successfully merging a pull request may close this issue.

5 participants