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

Get a pixel perfect match WMS image #14719

Merged
merged 2 commits into from Jun 1, 2023
Merged

Get a pixel perfect match WMS image #14719

merged 2 commits into from Jun 1, 2023

Conversation

sbrunner
Copy link
Member

@sbrunner sbrunner commented May 2, 2023

To avoid having little blurry image

Fix #14718
Thanks @mike-000

@github-actions
Copy link

github-actions bot commented May 2, 2023

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

@sbrunner
Copy link
Member Author

sbrunner commented May 5, 2023

@mike-000 are you allowed to review this pull request?

Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks @sbrunner! I think I understand the change but maybe having a rendering test specifically for this would be good? The unit test does not provide much explicit guarantee of having a crisp final render.

.editorconfig Outdated Show resolved Hide resolved
To avoid having little blurry image
Copy link
Member

Choose a reason for hiding this comment

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

Does this result look worse before this change?

I'm not sure I would call this as a "pixel perfect match" compared to the input tile:

image

Copy link
Contributor

@mike-000 mike-000 May 12, 2023

Choose a reason for hiding this comment

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

Using a data url might be better test. Currently with a ratio which is an odd fraction of the frame size you get blurring - in https://codesandbox.io/s/simple-forked-qivyjj?file=/main.js red and green pixels are interpolated as #808000 or #7f7f00 but if the ratio is changed to an even fraction e.g. 384 / 256 it is a bright mosaic of the two colors. With the fix that should be seen with any ratio.

Copy link
Member

@tschaub tschaub May 12, 2023

Choose a reason for hiding this comment

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

I'm not sure what to make of that sandbox example.

image

I was anticipating that this fix would make it so a previously blurry map was now less blurry. The code change looks to me like it could accomplish that. I assume it would be possible to demonstrate this with a test. I don't understand why a data url would be required for this.

Copy link
Contributor

@mike-000 mike-000 May 12, 2023

Choose a reason for hiding this comment

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

The data url creates a pixel mosaic which makes the blurring very obvious when magnified

image

Copy link
Member

Choose a reason for hiding this comment

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

Is this change meant to fix a real-world example that can be represented in a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I've changed the rendering test for the original example in #14718

The image still looks blurry here on GitHub but it's not the case, see:
https://raw.githubusercontent.com/sbrunner/openlayers/blury/test/rendering/cases/source-imagewms-blurry/expected.png

@fredj fredj force-pushed the blury branch 2 times, most recently from 5acd4de to cd94884 Compare May 26, 2023 13:39
@sbrunner
Copy link
Member Author

sbrunner commented Jun 1, 2023

Hello,
with the test written by @fredj, is it OK to be merged?

Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

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

All good! I ran the test without the changes and indeed the result is blurry:

image

I think we spent enough time looking at the Bourdonnette post office so I'm approving, thanks @sbrunner @fredj and @mike-000!

@tschaub tschaub merged commit 4d6f0be into openlayers:main Jun 1, 2023
8 checks passed
@tschaub
Copy link
Member

tschaub commented Jun 1, 2023

I agree it looks good. Thanks all for the improvement.

@sbrunner sbrunner deleted the blury branch June 3, 2023 12:38
@sbrunner
Copy link
Member Author

sbrunner commented Jun 3, 2023

Thanks :-)

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.

With version 7, the WMS image gets blurry in many cases
5 participants