Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Bug] Correct the transformation calculation of views #11933

Merged
merged 17 commits into from
Nov 18, 2020
Merged

[Bug] Correct the transformation calculation of views #11933

merged 17 commits into from
Nov 18, 2020

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Aug 26, 2020 •
edited
Loading

Description of Change

When the translation offset for a view is calculated, it incorrectly uses the scale to multiply/divide the actual offset. Side bug, seems WPF does not scale at all.

This PR will correct the math, I hope.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS: the rotation now happens before the scale, and thus the scale matrix is no longer being rotated
  • UWP: the scale no longer affectes the translate
  • WPF: added support for scaling along a single axis, and rotate before the scale

Behavioral/Visual Changes

None

Before/After Screenshots

Android iOS UWP WPF
BEFORE BEFORE BEFORE BEFORE
android ios-before uwp-before wpf-before
AFTER AFTER AFTER AFTER
ios-after uwp-after wpf-after

Not applicable

Testing Procedure

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@mattleibow mattleibow changed the base branch from main to 4.8.0 August 26, 2020 17:38
@mattleibow
Copy link
Contributor Author

I may need help to see if this is actually UI testable... The scaling doesn't affect the bounds properties, so it is going to be a bit harder to check.

@mattleibow mattleibow marked this pull request as ready for review August 26, 2020 18:47
@samhouts samhouts added the p/UWP label Aug 26, 2020
@mattleibow
Copy link
Contributor Author

BONUS: Fix the WPF rendering and also the iOS transform order... See screenshots :)

@mattleibow mattleibow changed the title [Bug] [UWP] Correct the translation calculation of views [Bug] Correct the transformation calculation of views Aug 26, 2020
@mattleibow mattleibow requested review from jsuarezruiz, PureWeen and StephaneDelcroix and removed request for jsuarezruiz August 26, 2020 20:01
@mattleibow
Copy link
Contributor Author

mattleibow commented Aug 26, 2020 •
edited
Loading

I did not test Tizen, GTK and macOS yet.

However, macOS uses the same code as iOS and Tizen seems good, but the order might be off. @rookiejava are you able to confirm?

GTK... not sure if it supports anything.

@samhouts samhouts added a/layout retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. labels Aug 27, 2020
@samhouts samhouts changed the base branch from 4.8.0 to 5.0.0 August 31, 2020 23:13
@samhouts samhouts added retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. and removed retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. labels Aug 31, 2020
@PureWeen PureWeen added this to the 5.0.0 milestone Nov 5, 2020
@PureWeen PureWeen added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Nov 5, 2020
PureWeen
PureWeen suggested changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

@mattleibow is writing Unit Tests

@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Nov 10, 2020
@PureWeen PureWeen removed the request for review from StephaneDelcroix November 13, 2020 19:46
@PureWeen PureWeen removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Nov 13, 2020
jsuarezruiz
jsuarezruiz approved these changes Nov 16, 2020
View reviewed changes
Xamarin.Forms.Platform.UAP.UnitTests/TransformationTests.cs

[Test, Category("Transformation"), TestCaseSource(nameof(TransformationCases))]
[Description("View transformation should match renderer transformation")]
public async Task TransformationConsistent(View view)
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

PureWeen
PureWeen suggested changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Looks like it still needs a little bit more rounding love @mattleibow

image

@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Nov 16, 2020
@mattleibow
Copy link
Contributor Author

I am rounding to 3 decimal places... Maybe the library has a bug or something? Maybe it is the nfloat or something.

I'll check.

@mattleibow
Copy link
Contributor Author

I see lots more green, but still a bit of red...

@rmarinho
Copy link
Member

The iOS failure is not related.

@mattleibow
Copy link
Contributor Author

Another place were this actually shows up as what I think i an improvement is the "pinch gallery". Before, when you pinch, it would scale from the left edge. Afterwards, it scales from the center as expected.

@PureWeen PureWeen removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Nov 18, 2020
@mattleibow mattleibow merged commit 36741b9 into 5.0.0 Nov 18, 2020
@mattleibow mattleibow deleted the mattleibow/fix-11931 branch November 18, 2020 21:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PureWeen PureWeen PureWeen approved these changes

@jsuarezruiz jsuarezruiz jsuarezruiz approved these changes

Assignees

@PureWeen PureWeen

@jsuarezruiz jsuarezruiz

Labels
a/layout blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. p/iOS 🍎 p/Tizen p/UWP p/WPF t/bug 🐛
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

[Bug] [UWP] View translation is incorrectly calculated
6 participants
@mattleibow @rookiejava @rmarinho @PureWeen @jsuarezruiz @samhouts