[ui] faster intersection computation for scale/translate.#170446
[ui] faster intersection computation for scale/translate.#170446jonahwilliams wants to merge 7 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
engine
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
LGTM, some nits that I think bear thinking about in the tests...
| state.transform2DAffine(2, 0, 10, 0, 2, 10); | ||
| EXPECT_TRUE(state.IsTranslateScale()); | ||
|
|
||
| state.transform2DAffine(1, 0.5, 0.5, 1, 0, 0); |
There was a problem hiding this comment.
I think I've done something similar myself in other tests, but looking at this from a reviewer's perspective, I think it would be better if each of these blocks starts with either setIdentity or EXPECT_TRUE(IsTranslateScale()) so that if anyone ever adds new code they can't disrupt the carry-over state from one mini-test to the other...? Something so that each block can be deleted, moved, or a new one added and the flow of the state won't break.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing test for FastIntersection.
There was a problem hiding this comment.
Removed this code
| std::optional<impeller::Rect> dl_intersected = | ||
| dl_mapped.Intersection(cull_rect_); | ||
| if (!dl_intersected.has_value()) { | ||
| *mapped = dl_intersected.value(); |
There was a problem hiding this comment.
i.e. value_or or Rect::IntersectionOrEmpty
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
LGTM, just some foods for thoughts
|
|
||
| template <class T, class U> | ||
| bool NotEquals(std::shared_ptr<const T> a, const U* b) { | ||
| bool NotEquals(std::shared_ptr<const T>& a, const U* b) { |
There was a problem hiding this comment.
I'll file an issue to do the others unless you want to just go through them.
| Scalar t = GetTop() * sy + ty; | ||
| Scalar b = GetBottom() * sy + ty; | ||
|
|
||
| return TRect<float>::MakeLTRB(std::min(l, r), // |
There was a problem hiding this comment.
Impl idea - another way to do this would be something like:
Scalar l,r,t,b;
if (sx > 0) {
l = left_ * sx + tx;
r = right_ * sx + tx;
} else {
l = right_ * sx + tx;
r = left_ * sx + tx;
}
if (sy < 0) {
...
}
not sure if it is any faster.
| Modifiers std::enable_if_t<std::is_floating_point_v<U>, Return> | ||
| #define ONLY_ON_FLOAT(Return) DL_ONLY_ON_FLOAT_M(, Return) | ||
|
|
||
| #define CHECK_INTERSECT(al, at, ar, ab, bl, bt, br, bb) \ |
There was a problem hiding this comment.
multi-line macros are scary, but this is effective, so ... eek?
|
hmm, looks like we have some different behavior with nan. let me double check that... |
|
Also, should the tr/sc transform method FML_DCHECK on the matrix actually being tr/sc? And, we could stand to add some new cases to |
|
@jonahwilliams Are you still planning on completing this :) Feel free to close if not. |
|
Someone else will need to carry the torch :) |
Taken partially from #170446 with only the changes to add a ScaleTranslate rectangle transform method brought over. Unlike the original PR, this PR will apply that optimized method for any caller who transforms a rectangle rather than just the matrix/clip tracker for the engine layer tree code. A benchmark for the new methods is also provided to verify their improved performance.
Taken partially from #170446 with only the changes to add a ScaleTranslate rectangle transform method brought over. Unlike the original PR, this PR will apply that optimized method for any caller who transforms a rectangle rather than just the matrix/clip tracker for the engine layer tree code. A benchmark for the new methods is also provided to verify their improved performance.
) Taken partially from flutter#170446 with only the changes to add a ScaleTranslate rectangle transform method brought over. Unlike the original PR, this PR will apply that optimized method for any caller who transforms a rectangle rather than just the matrix/clip tracker for the engine layer tree code. A benchmark for the new methods is also provided to verify their improved performance.
) Taken partially from flutter#170446 with only the changes to add a ScaleTranslate rectangle transform method brought over. Unlike the original PR, this PR will apply that optimized method for any caller who transforms a rectangle rather than just the matrix/clip tracker for the engine layer tree code. A benchmark for the new methods is also provided to verify their improved performance.
3 participants
Pulled out of #168019 which is stale.