Skip to content

Implement correct orthographic projection #22985

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

Merged
merged 5 commits into from
Oct 18, 2018
Merged

Implement correct orthographic projection #22985

merged 5 commits into from
Oct 18, 2018

Conversation

sbaranov
Copy link
Contributor

@sbaranov sbaranov commented Oct 12, 2018

This fixes gestures in InkWell and WebView (#6080).

@sbaranov sbaranov changed the title Implement correct orthographic projection. This fixes gestures in InkWell (#6080). Implement correct orthographic projection. This fixes gestures in InkWell and others (#6080). Oct 12, 2018
@sbaranov sbaranov changed the title Implement correct orthographic projection. This fixes gestures in InkWell and others (#6080). Implement correct orthographic projection. This fixes gestures in InkWell and WebView (#6080). Oct 12, 2018
@sbaranov sbaranov requested a review from Hixie October 12, 2018 00:48
@sbaranov sbaranov changed the title Implement correct orthographic projection. This fixes gestures in InkWell and WebView (#6080). Implement correct orthographic projection Oct 12, 2018
final Matrix4 transform = getTransformTo(ancestor);
final double det = transform.invert();
if (det == 0.0)
return Offset.zero;
return MatrixUtils.transformPoint(transform, point);
Copy link
Contributor

Choose a reason for hiding this comment

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

should transformPoint itself be fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like transformPoint is used by some code that doesn't expect unprojection (see localToGlobal). So we would probably keep transformPoint as is, and create new function perspectiveTransformPoint. We could do this change incrementally if people find more problems with various hit testing.

final Vector3 o = Vector3(0.0, 0.0, 0.0);
final Vector3 n = Vector3(0.0, 0.0, 1.0);
final Vector3 p = s + d * (n.dot(o - s) / n.dot(d));
return Offset(p.x, p.y);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the likely performance impact of this change? (This code is potentially hot in certain cases.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and optimized it slightly, but it shouldn't be bad either way since it's still relatively light weight (only four divisions) and is only called O(1) times per frame. It could be optimized further, but likely not worth it.

@Hixie
Copy link
Contributor

Hixie commented Oct 17, 2018

Consider updating https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/painting/matrix_utils.dart#L122 instead (or add an explanation there saying why it's different).

Assuming the performance implications aren't dire, LGTM modulo suggested changes.

@sbaranov
Copy link
Contributor Author

Doc comment updated to explain that.

@sbaranov sbaranov merged commit d5777b6 into flutter:master Oct 18, 2018
@sbaranov sbaranov deleted the projection branch October 18, 2018 18:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants