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

Fix immediate renderer image and text rotation in rotated canvas #14333

Merged
merged 2 commits into from Nov 24, 2022

Conversation

mike-000
Copy link
Contributor

Fixes the inconsistent rotations observed in #14308 (comment)

Subtract canvas rotation from image or text rotation so it does not automatically rotate with the view (or double that rotation if rotateWithView is specified).

Replace circle style with triangle in rendering tests so any incorrect image rotation can be seen.

@github-actions
Copy link

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

* @private
* @type {number}
*/
this.transformRotation_ = transform
Copy link
Member

Choose a reason for hiding this comment

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

Something looks wrong here. transform is a transform matrix, not a number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rotation is the number. I first check that the transform is not undefined or null (which is should never be) as several browser tests which create a CanvasImmediateRenderer without using drawImages_ or drawText_ fail if they attempt to access entries from a missing matrix.

@@ -83,6 +84,14 @@ class CanvasImmediateRenderer extends VectorContext {
*/
this.transform_ = transform;
Copy link
Member

Choose a reason for hiding this comment

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

Here you apply the transform...

* @private
* @type {number}
*/
this.transformRotation_ = transform
Copy link
Member

Choose a reason for hiding this comment

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

... and here you apply the same transform, but the type says number. If you want the rotation here, you need to extract it from the transform, or assign null initially and note that in the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not the end of the statement. I check if the transform exists before extracting from it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Sorry, my bad.

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 518e315 into openlayers:main Nov 24, 2022
@mike-000 mike-000 deleted the image/text-rotation branch November 24, 2022 19:47
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.

None yet

2 participants