Add a display list op to clear the transformation stack. #32050
Conversation
|
This patch has a friend in Impeller that I'll land at the same time. |
chinmaygarde
changed the title
| @@ -152,6 +152,11 @@ void SkMatrixDispatchHelper::transformFullPerspective( | |||
|
|
|||
| // clang-format on | |||
|
|
|||
| void SkMatrixDispatchHelper::transformReset() { | |||
| matrix_ = {}; | |||
| matrix33_ = {}; | |||
There was a problem hiding this comment.
Hmm, I think saved_ needs to be cleared as well. And, I think there is a bug here where calling an imbalaned restore will call pop_back on an empty vector which is undefined behavior.
Fixing both issues.
There was a problem hiding this comment.
Nevermind. I can't confirm this.
There was a problem hiding this comment.
SkMatrixDispatchHelper already has a reset() method. I'm not sure if it is used any more (I used to collect saveLayer bounds in their native coordinate space, but had to back off on that due to bugs).
You don't want saved_ to be cleared, if they restore() then they should undo the reset. The set to identity should not escape the enclosing save/restore.
There probably should be a protection here for over-restore, but the caller generally already does that. It would be redundant for existing use cases, but we shouldn't rely on that.
|
Right, I was attempting to be consistent with |
|
Looks like this needs an Impeller update. |
chinmaygarde
force-pushed
the
transformreset
branch
from
383c4bd to
3039cd3
Compare
chinmaygarde
added
the
waiting for tree to go green
This allows for the implementation of the DisplayListCanvasRecoder::didSetM44. Instead of another operation that clears the transformation stack along with setting a new transformation on it. A single no-payload operation that clears the stack is added. Existing ops to push to the stack are then reused.
chinmaygarde
force-pushed
the
transformreset
branch
from
3039cd3 to
ba26d52
Compare
|
Gold has detected about 18 new digest(s) on patchset 1. |
|
The gold failure can't possibly be related. There was an assertion there earlier. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
|
@mdebbar Do you know what's going on with the gold checks? |
|
These are golden changes from my PR: #31907. They are supposed to be uploaded during post-submit and considered as the new baseline. But something went wrong in post-submit. I'm looking into it now. |
|
This pull request is not suitable for automatic merging in its current state.
|
fluttergithubbot
removed
the
waiting for tree to go green
|
The test failure is a known flake flutter/flutter#95751 |
* 09d7bcc Optionally specify the target dir in tools/gn (flutter/engine#32065) * a00ba24 Fix done button click not blur in iOS keyboard (flutter/engine#31718) * 81547d1 Add a display list op to clear to transformation stack. (flutter/engine#32050) * 2309bcc Add WASM target in gn (flutter/engine#31670) * 852e800 [web] Remove the --passfail flag when calling goldctl in post-submit (flutter/engine#32071) * eb1c50d Fix issues with nested gradients in html renderer. (flutter/engine#31887) * fb0fd74 Update the magic number for JPEG to just FF D8 FF. (flutter/engine#32076) * 233c17c Wrap the global timeline event handler callback in a std::atomic (flutter/engine#32073) * dfde2aa Roll Dart SDK from 24bf86f16411 to 5bc905e69609 (9 revisions) (flutter/engine#32075)
This allows for the implementation of the
DisplayListCanvasRecoder::didSetM44. Instead of another operation that
clears the transformation stack along with setting a new transformation
on it. A single no-payload operation that clears the stack is added.
Existing ops to push to the stack are then reused.