Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Overzoomed image sources vanish #10677

Merged
merged 6 commits into from
Dec 13, 2017
Merged

Conversation

asheemmamoowala
Copy link
Contributor

RenderImageSource used ScreenCoordinates to determine the ideal zoom and tilecover for the bounds of the image. When the map is pitched, screen coordinate values for the LatLngs that are off-screen grow exponentially to INFINITY.

Using z0 tile coordinates instead allows computing the ideal zoom tile consistently for all zooms and pitches.

@asheemmamoowala asheemmamoowala added bug Core The cross-platform C++ core, aka mbgl labels Dec 12, 2017

// Don't bother drawing the ImageSource unless it occupies >4 screen pixels
enabled = (width * height > 4);
enabled = dMax * std::pow(2.0, transformState.getZoom()) > 2.0 / util::tileSize;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Number of pixels in a tile between the x or y extents of the tile coordinates at the current zoom level.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I understand what's going on here, a comment in the code would be nice.

Here's how I'd think of it: scale factor is std::pow(2.0, transformState.getZoom()). Assuming scale of 1 (i.e. z0 tile), extent is 8192 and tileSize is 512, so you'd expect one tile unit to be 1/16th of a pixel, so 4 pixels would be 64 tile units. I must be missing some conversion factor used in TileCoordinatePoint?

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Dec 12, 2017

Choose a reason for hiding this comment

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

a comment in the code would be nice.

👍

At any zoom level 1 geometry coordinate unit would be tileSize/extent pixels.

dMax is in tile coordinate units at z0, so we first scale it by util::EXTENT and then by 2^z (the scale factor) to get geometry coordinates at the current zoom. The long side then becomes :dMax * scale * extent * tileSize / extent pixels.

This will enable image sources when they span >2px on their long side.

@ChrisLoer
Copy link
Contributor

The approach of using the z0 tile coordinate plane instead of the screen coordinates seems reasonable (although it will change the zoom calculation in pitched maps, right?).

I don't think your changes here affect this at all, but what happens if you try to place an image over the anti-meridian? It doesn't seem like we have any support for handling the wrapping. Sorry I'm not very familiar with RenderImageSource...

Can you make a render test for the pitched case that was broken before this fix?

@asheemmamoowala
Copy link
Contributor Author

asheemmamoowala commented Dec 12, 2017

Can you make a render test for the pitched case that was broken before this fix?

👍

although it will change the zoom calculation in pitched maps, right?

Now that the computation of ideal zoom level is done in tile coordinates, it won't change based on current zoom, bearing, or pitch. The previous implementation resulted in different zoom levels when pitched or zoomed in, but that was unnecessary.

The anti-meridian case works as long as the points that cross as wrapped.
So far example

[-195.82031249999997, -20.96143961409684],
[-155.830078125, -20.96143961409684]
[-155.830078125, 19.476950206488414]
[-195.82031249999997, 19.476950206488414]

works as expected:
screen shot 2017-12-12 at 1 05 56 pm

// dMax is in tile coordinate units at z0, so scale by util::EXTENT and then
// by 2^z to get geometry coordinates at the current zoom.
// 1 gc unit = tileSize / extent pixels.
enabled = dMax * std::pow( 2.0, transformState.getZoom()) * util::tileSize > 2.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra space after std::pow(

// display of at least 2 x 1 px image
// dMax is in tile coordinate units at z0, so scale by util::EXTENT and then
// by 2^z to get geometry coordinates at the current zoom.
// 1 gc unit = tileSize / extent pixels.
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I think I understand how I got confused now.

To me GeometryCoordinate just means a coordinate represented as a pair of int16_ts. I didn't pick up that by "GeometryCoordinate" you meant what I'm used to thinking of as "tile unit coordinates" (ie 0 to EXTENT). I also didn't realize that the TileCoordinate units were the based on the width of a single tile -- I thought they were EXTENT based. How about something like:

// Only enable if the long side of the image is > 2 pixels, resulting in a
// display of at least 2 x 1 px image
// dMax is in TileCoordinate units at z0, so one unit represents the width of one z0 tile
// To convert to pixels, multiply by util::tileSize and then scale by 2^z to match current zoom 

@asheemmamoowala asheemmamoowala merged commit f545d21 into master Dec 13, 2017
@asheemmamoowala asheemmamoowala deleted the fix-overzoomed-imgsrc-vanish branch December 13, 2017 20:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants