Skip to content

Fix ImageTile/DataTile source hidpi reprojection #16280

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

Closed

Conversation

mike-000
Copy link
Contributor

@mike-000 mike-000 commented Oct 12, 2024

Fixes #16278

Fixes the ImageTile source problems described in #16278, which dated from #15905.
Updates the source setup in the example so it would work if reprojected.

Also fixes the hidpi DataTile problem described in #16278 which was introduced by #15860, adds a test and update results for two others where more pixels are made available in reprojection.

Incorrect reprojection when a hidpi ImageTile source is not defined as hidpi is also fixed.

Copy link

📦 Preview the website for this branch here: https://deploy-preview-16280--ol-site.netlify.app/.

@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch from 9a549ea to 03170aa Compare October 12, 2024 20:18
@mike-000 mike-000 marked this pull request as ready for review October 12, 2024 20:24
@mike-000 mike-000 marked this pull request as draft October 13, 2024 12:48
@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch 2 times, most recently from 8090cdc to 3f18bae Compare October 13, 2024 13:45
@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch from 3f18bae to 578d8b7 Compare October 14, 2024 19:00
@mike-000 mike-000 marked this pull request as ready for review October 14, 2024 19:05
@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch from b52f82c to 18791a2 Compare October 28, 2024 13:27
@mike-000 mike-000 changed the title Fix ImageTile source hidpi reprojection Fix ImageTile/DataTile source hidpi reprojection Oct 31, 2024
@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch from 9d42449 to 79ff6fa Compare October 31, 2024 11:09
@mike-000
Copy link
Contributor Author

@ahocevar This is ready for review as it fixes the immediate bugs.

In ReprojDataTile the pixel ratio was not included in the returned image dimensions (resulting in incomplete images). In some cases inconsistent rounding of fractional values resulted in corrupted images.

In glreproj the pixel ratio was not being included in the stitchTexture sizing, resulting in reduced quality (in the extreme case of half the rows and columns of black and white pixels being removed without interpolation the result was all black or all white).

@mike-000
Copy link
Contributor Author

In common with other hidpi rendering tests a27fba6 only demonstrates that when correctly configured the output is correct in a standard dpi screenshot (i.e. all of the source tile is used and all the reprojected data is exported in the reproj tile image). There are currently no rendering tests which demonstrate the map canvas is genuinely hidpi.

If pixelRatio: 2 is specified in the map options this is the map canvas export without d983fa2 blurred because the reprojection has not considered the tile pixel ratio in the stitch texture
image

and much sharper with d983fa2 particularly noticeable with the labels in the top right quadrant.
image

A regular rendering test would not detect the difference in a standard dpi screenshot. A test which scaled the map div might work if it was compliant with the limitations of #14199

const style = document.getElementById('map').style;
style.position = 'relative';
style.left = '-128px';
style.top = '128px';
style.transform = 'scale(2)';

Comment on lines 24 to 25
tileSize: 512,
tileGrid: createXYZ({tileSize: 256, maxZoom: 22}),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is equivalent to specifying tilePixelRatio: 2 in ol/source/XYZ. It is not needed in that example any more than tilePixelRatio was when the example used ol/source/XYZ as there is no reprojection, but as then it is the correct way to specify the source in case it is reprojected.

Since DataTile sources can have different pixel ratios in each dimension there is no tilePixelRatio option, see #13648. However, I do think having a tilePixelRatio option for ol/source/ImageTile would be a nice future enhancement, where unless both tileSize and tileGrid were specified they could be calculated and passed to super().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now added a tilePixelRatio option so the source can be set up in a simpler way consistent with ol/source/XYZ (based on the code from #16279 which ensures data sizes are integer even if tile grid sizes are not). The new hidpi map rendering test is using it.

@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch from 961f4c0 to 67a6d8b Compare November 17, 2024 13:38
@mike-000 mike-000 force-pushed the ImageTile-source-hidpi-reproj branch from 7d82d57 to 3e41540 Compare November 17, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect retina tile reprojections
2 participants