Migrate surface frame shell code to DisplayList/Impeller geometry classes #173086
Conversation
github-actions
bot
added
platform-android
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a large but important refactoring to migrate from Skia's geometry types to the DisplayList/Impeller geometry classes. The changes look very thorough and consistent across the codebase. I've only found one minor point about an unnecessary include. Great work on this migration!
…ace-frame-shell-code-to-dl-geom
…ace-frame-shell-code-to-dl-geom
|
AFAICT the google testing is not something that I did so the Google testing failure can be ignored. I will set the status to passing after reviews. |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
So exciting to see those includes go away!
| transformation.pers0 = matrix[SkMatrix::kMPersp0]; | ||
| transformation.pers1 = matrix[SkMatrix::kMPersp1]; | ||
| transformation.pers2 = matrix[SkMatrix::kMPersp2]; | ||
| transformation.scaleX = matrix.m[0]; |
There was a problem hiding this comment.
Maybe we should have the same static constexprs for these indices too. In a later patch though.
There was a problem hiding this comment.
I agree in principle, but there is a practical issue that I've seen with names like that. In particular, for instance, the names are often ambiguous. What is SkewX? Is it the amount that X skews Y, or the amount that X is skewed by Y? Depending on how the developer interprets that name, it either goes in [0,1] or [1,0]...
I'd love to see some name suggestions that would make this foolproof.
There was a problem hiding this comment.
Making the names foolproof might be hard. But I often think "gosh, is this row or column major storage" and have very modest needs. The stuff about the skews can be documented (and just match Skia for least surprise). Perhaps perfect is the enemy of good when it comes to adding these names?
|
@reidbaker did you update the branch to try to fix the Google Test failure? Is there a pending commit that might have helped? The result of the update is that the PR went from one failure that is obviously unrelated to 2 failures, the same one as earlier plus a newer one that isn't so clear is unrelated. I still plan to just mark the G3 testing as passing manually, but was curious if you knew something about the failures that a branch update might fix...? |
|
No, I was investigating the Google testing failure here and then accidentally updated your PR with main branch when I intended to update my own PR with the main branch. |
flutter-dashboard
bot
removed
the
autosubmit
…sses (flutter#173086) Converting a large variety of classes involved in tracking surfaces, frames, surface damage, etc. to using the DisplayList/Impeller geometry classes. Addresses a bullet item in flutter#161456 --------- Co-authored-by: Reid Baker <[email protected]>
…sses (flutter#173086) Converting a large variety of classes involved in tracking surfaces, frames, surface damage, etc. to using the DisplayList/Impeller geometry classes. Addresses a bullet item in flutter#161456 --------- Co-authored-by: Reid Baker <[email protected]>
…sses (flutter#173086) Converting a large variety of classes involved in tracking surfaces, frames, surface damage, etc. to using the DisplayList/Impeller geometry classes. Addresses a bullet item in flutter#161456 --------- Co-authored-by: Reid Baker <[email protected]>
…sses (flutter#173086) Converting a large variety of classes involved in tracking surfaces, frames, surface damage, etc. to using the DisplayList/Impeller geometry classes. Addresses a bullet item in flutter#161456 --------- Co-authored-by: Reid Baker <[email protected]>
3 participants
Converting a large variety of classes involved in tracking surfaces, frames, surface damage, etc. to using the DisplayList/Impeller geometry classes.
Addresses a bullet item in #161456