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

Use .transform() instead of .setTransform() when rendering image or text #14483

Merged
merged 2 commits into from Aug 14, 2023

Conversation

mike-000
Copy link
Contributor

@mike-000 mike-000 commented Feb 5, 2023

Fixes #14478

Allows rotated images and text, and line placement text to be displayed offset as expected following a context translate() or scale() in a prerender event. No rendering tests broken by this change.

Also fix similar issue in immediate renderer.

@github-actions
Copy link

github-actions bot commented Feb 5, 2023

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

@kevmcgrath
Copy link

Hi @mike-000,

We're eager for this fix, which also addresses #14478 and see that it didn't make it into v7.3. Do you know if there are plans to include it in the next release? The issue is unfortunately holding up one of our projects.

Kind Regards,
Kevin

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

@mike-000 Thanks for this fix. I think what changes for users now is that postrender needs to reset any transforms that prerender sets. It would be good to test if this is indeed the case, and if so, add at least a paragraph in the release notes.

@mike-000
Copy link
Contributor Author

mike-000 commented Mar 7, 2023

@mike-000 Thanks for this fix. I think what changes for users now is that postrender needs to reset any transforms that prerender sets. It would be good to test if this is indeed the case, and if so, add at least a paragraph in the release notes.

In most cases that would already been required - in drawImageOrLabel() setTransform was called inside a save/restore just as transform is now. Only if the immediate renderer had called context.setTransform(1, 0, 0, 1, 0, 0) to revert a previous setTransform - which only happened if the style had scale or rotation - would it not have been necessary, but in that case the transform set by the application would not have had the expected effect on the rendering. For example set the CircleStyle scale to anything other than 1 in https://codesandbox.io/s/feature-move-animation-forked-6vt3o9 and the translated copy of the feature does not appear.

@mike-000 mike-000 requested a review from ahocevar March 30, 2023 15:42
@kevmcgrath
Copy link

May I get an update on this? We're still hoping this fix gets included in a future release.

@ahocevar
Copy link
Member

@mike-000 Thanks for this fix. Now that we've some backwards incompatible changes lined up already, could you maybe rebase this pull request and move the upgrade notes into the "backwards incompatible changes" section? Thanks in advance!

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks, @mike-000

@ahocevar ahocevar merged commit de71b05 into openlayers:main Aug 14, 2023
8 checks passed
@kevmcgrath
Copy link

Thank you, @mike-000 and @ahocevar!

@tschaub
Copy link
Member

tschaub commented Aug 14, 2023

@kevmcgrath - you may know this already, but we create a dev tagged release after every merge to the default branch. So you can get this fix with npm install ol@dev. As mentioned above, we have rolled in some other backwards incompatible changes, so the next stable release will be 8.0. See the upgrade notes for changes you might need to know about that aren't yet in a stable release.

And the dev releases pass all the same tests that our stable releases do, so you shouldn't really think about it as "unstable." But it does mean that the version number might not reflect the nature of the changes since the previous dev release.

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.

Rotated vector layer text styles not displaying at correct location when context is clipped and translated
4 participants