[Impeller] Off by default experimental canvas. #52035
Conversation
| SkScalar opacity) override { | ||
| save(); | ||
| display_list->Dispatch(*this); | ||
| restore(); |
There was a problem hiding this comment.
Maybe add a DCHECK that we returned to the same level we started at?? DL should never leave us hanging here, but it couldn't hurt to check?
This saves having to construct a new Dispatcher object for the sub-DL, which would technically be a "safer" implementation.
jonahwilliams
changed the title
jonahwilliams
changed the title
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| const Paint& paint); | ||
| virtual void DrawTextFrame(const std::shared_ptr<TextFrame>& text_frame, | ||
| Point position, | ||
| const Paint& paint); |
There was a problem hiding this comment.
I think the shared data and logic between the old and new canvas is going to get confusing real fast as we continue ramping up the new dispatcher.
Why not just make the new "Canvas" not share an interface with the original Canvas at all? That way, we can start diverging immediately where it makes sense, and we can incorporate @flar's changes in the new Canvas without having to mess with the original interface unnecessarily.
There was a problem hiding this comment.
This was your suggestion to make the migration more gradual without fully rewriting the renderer :)
There was a problem hiding this comment.
My suggestion was more to reuse Entities render methods, not the Aiks or Dispatcher logic. I think not extending Canvas would be a minor change here, since it doesn't look like you're actually using much Canvas stuff (unless I'm missing something)?
There was a problem hiding this comment.
Oh I was never going to rewrite entities, that s waaay too much work for one patch. I could do a one time copy/paste of the dispatcher and canvas logic but its quite non-trivial and I wouldn't want to maintain that for too long. If you are able to help me get this work finished quickly I think that would be OK, but I wouldn't want to maintain two dispatchers for very long.
There was a problem hiding this comment.
Ah, I totally misread this patch -- I just noticed that you're overriding ExperimentalCanvas::AddEntityToCurrentPass to call the render method, and it actually is using most of the original Canvas logic. Disregard. 😅
There was a problem hiding this comment.
current_pass_ and base_pass_ aren't really needed for the experimental canvas. It would be nice if we renamed "Canvas" to "CanvasBase" and then Canvas and ExperimentalCanvas would subclass from that with their differences and associated data hidden in their own sub-class...?
| const std::shared_ptr<ImageFilter>& backdrop_filter, | ||
| ContentBoundsPromise bounds_promise) { | ||
| // Can we always guarantee that we get a bounds? Does a lack of bounds | ||
| // indicate something? |
There was a problem hiding this comment.
The bounds parameter is kinda badly named. In Skia it's a "hint" for optimization that allows the user to promise they won't draw outside of the area. We could do away with this when we start querying computed SaveLayer bounds from the DL.
There was a problem hiding this comment.
This bounds value is already computed in the DL. So these are actual bounds, modulo some places where we seem to not get bounds.
There was a problem hiding this comment.
Will ExperimentalCanvas be used independently of a DL dispatch? The current aiks Canvas gets content from tests that don't always supply a bounds, but the DL dispatch will now always have a bounds. It has an additional hint that the bounds might (as in probably, most likely) clip the contents, but other than that distinction, the bounds should always be trustable if they came from a DL.
There was a problem hiding this comment.
It will not be used independently of a DlDispatcher. In fact, long term we'll probably just merge this class into the DL dispatcher.
| if (!bounds.has_value()) { | ||
| bounds = Rect::MakeSize(render_target_.GetRenderTargetSize()); | ||
| } | ||
| Rect subpass_coverage = bounds->TransformBounds(GetCurrentTransform()); |
There was a problem hiding this comment.
ExperimentalCanvas could probably FML_[D]CHECK(bounds.has_value())
jonahwilliams
added
the
autosubmit
| auto transform = entity.GetTransform(); | ||
| entity.SetTransform( | ||
| Matrix::MakeTranslation(Vector3(-GetGlobalPassPosition())) * transform); | ||
| entity.Render(renderer_, *render_passes_.back()); |
There was a problem hiding this comment.
Is the ExperimentalCanvas using the old clipping stencil buffer implementation? (the reason I ask is it isn't setting the "NewClipDepth") I didn't think that was still in there...
…146960) flutter/engine@725ebd7...07f7532 2024-04-17 [email protected] [Impeller] Off by default experimental canvas. (flutter/engine#52035) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#146960) flutter/engine@725ebd7...07f7532 2024-04-17 [email protected] [Impeller] Off by default experimental canvas. (flutter/engine#52035) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Canvas implementation that renders directly, requires ENABLE_EXPERIMENTAL_CANVAS to be set to true and only wired up for android/vulkan.
Currently missing:
This will eventually allow us to delete the aiks layer and cut out most conversion costs. Part of flutter/flutter#142054