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

Apply transformation for the stroke on the canvas so the skew is also… #2531

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

Mirko-Volkers
Copy link
Contributor

I noticed that the Android implementation had trouble with applying a skew on a stroke, so this attempts to fix that issue.

The issue was that since the transformation was applied to the path, it didn’t take any properties from the paint into consideration. As a result, the skew was not applied to the width of the line. By applying the transformation on the canvas, this issue is resolved.

Example Lottie which wasn’t rendered correctly (should have skewed lines):
Skewed-Stroke.json

@gpeal
Copy link
Collaborator

gpeal commented Aug 5, 2024

@Mirko-Volkers Thanks for taking a look at this. Could you add Skewed-Stroke.json to this folder? It'll get auto-included in snapshot tests if you do.

Copy link

github-actions bot commented Aug 5, 2024

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@gpeal
Copy link
Collaborator

gpeal commented Aug 5, 2024

@Mirko-Volkers Based on the snapshot tests, it doesn't look like these changes are correct. Several of the failing ones are in the same snapshot-test module so feel free to take a look at them locally to see what's going on.

Copy link

github-actions bot commented Aug 5, 2024

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@Mirko-Volkers
Copy link
Contributor Author

Yeah, I see some differences in the snapshots.
Found some changes because the scale was applied twice, going through it locally to check if I can fix it and make sure I'm not missing something.

Copy link

github-actions bot commented Aug 6, 2024

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

I just looked through the diffs and they look good to me. They're all either small aliasing differences or actually seem more accurate. Thanks for looking into this!

@gpeal gpeal merged commit 5a71516 into airbnb:master Aug 6, 2024
7 checks passed
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.

3 participants