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

Debounce force center after view animation. #9541

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

ger-benjamin
Copy link
Member

@ger-benjamin ger-benjamin commented Nov 26, 2024

For https://camptocamp.atlassian.net/browse/GSSCHWYZ-429
And camptocamp/GeoMapFish#92

But will break (origin): https://camptocamp.atlassian.net/browse/GSGMF-2009
Who looks to be a issue on a particular GMF.

This workaround was making the map less blurry in case of map height with a not round number of pixels, but it cause the map zoom to "jump" if the view.contrainResolution is set to true.

My idea is to trash this function, and let the concerned GMF implement it on need.

Another possibility would be keep the function in the code but not active by default (let the user choose if he want to use it).

Or to add a options in the vars to activate it or not.

If we keep it we could also add a debounce to to not recenter too quickly.

Examples
Storybook
API help
API documentation

@sbrunner
Copy link
Member

This code is required to don't get bury tiles, this should be activated by default, we can add an option to be able to disable it.

@ger-benjamin
Copy link
Member Author

Ok, I've more contexte now, so all the PSC asked for this feature because at some point, OL changes has made the WMTS layers sometimes blury.
In this case, I'll try first to check if the WMTS still are blurry (maybe that's not anymore the case with latest OL).
If not, I'll keep this function activated by default, but add a possibility to disable it (via js) and I'll try to add a debounce or some new code to not have this ugly "jump" behavior.

@ger-benjamin ger-benjamin force-pushed the remove-force-set-center branch from 0200a62 to b8815c3 Compare November 27, 2024 07:49
Cleaner code and allow a js way to remove/reset/update this
function if needed.

Keep the function to be sure the layers are not blurry. But also
add a debounce to not have a map "jumping around" on zoom-in/out.
@ger-benjamin ger-benjamin force-pushed the remove-force-set-center branch from b8815c3 to 241db69 Compare November 27, 2024 07:51
@ger-benjamin
Copy link
Member Author

I've tried several on different configuration to reproduce the "blurry effect", even with the commit before the original fix, but I was not able to see any difference. If we had a map quality issue with OpenLayers 7, there are good chance that it's fixed now. Therefor, I would still recommend to remove this function "setCenter" function.
And with this new code, it's now possible to remove this function at implementation if wanted/needed.

However, as I can't give guaranty that the blur effect is gone, I've added a debounce around the original fix. It allows to keep this fix, and avoid to recenter too frequently during the zoom (avoid the jumping-around effect).

@ger-benjamin ger-benjamin requested review from sbrunner and removed request for sbrunner November 27, 2024 08:01
Copy link
Member

@sbrunner sbrunner left a comment

Choose a reason for hiding this comment

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

Nice thanks :-)

@maltaesousa
Copy link

Thanks @ger-benjamin for your tests! ping @gnerred

@ger-benjamin
Copy link
Member Author

Don't hesitate to open again the issue if needed (but I'm pretty confident on my fix).
You can take the new version of ngeo 2.8, 2.9 and master with the fix in some hours (tomorrow to be sure).

@gnerred
Copy link

gnerred commented Nov 28, 2024

Thanks @ger-benjamin and @maltaesousa , I will test it asap.

@ger-benjamin ger-benjamin changed the title Remove force center after view animation. Debounce force center after view animation. Nov 28, 2024
@gnerred
Copy link

gnerred commented Dec 6, 2024

I've upgraded our Cartoriviera instance to 2.8.1.170, and the problem is fixed.

https://map.cartoriviera.ch

Thanks a lot !

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

Successfully merging this pull request may close these issues.

5 participants