Refactor visibility_detector to avoid forcing compositing. #367
Conversation
Instead, use methods on RenderObject and new framework API to listen to compositing and determine the transform/clip. Removes an O(N^2) algorithm from the ancestor walk. Deletes the custom layer that is no longer used. Minor update to main.dart to be more desktop friendly. Tests adjusted to use new API. Removed API has no usages in internal codesearch.
| @@ -74,6 +74,7 @@ class VisibilityDetectorDemo extends StatelessWidget { | |||
| Widget build(BuildContext context) { | |||
| return MaterialApp( | |||
| title: title, | |||
| scrollBehavior: const MaterialScrollBehavior().copyWith(scrollbars: false), | |||
There was a problem hiding this comment.
This makes the framework stop complaining when you run the example app in desktop mode. It's not essential for this change.
There was a problem hiding this comment.
(No, it's not related)
|
Ok. In some local benchmarking on a Pixel 4 this is showing some improvements over the old version now. I've made the following changes from initial:
The caching logic can probably be further improved. The lookups would benefit from using the layer tree, but that's currently blocked on flutter/flutter#103469 |
|
I'll mark this as ready for review once the framework PR has landed and I can put a semi-meaningful framework version into the SDK constraint. |
|
Moto g4 at head: Moto G4 with this patch: |
|
Wembley 1gb: Base (2nd run, try to avoid some shader comp): Patch: |
|
those are great numbers |
|
Looking into some internal test failures. |
|
flutter/flutter#103768 and flutter/flutter#103748 are blocking this right now |
|
Latest change is down to ~20 internal failures (not including minor scuba changes) as long as the linked flutter/flutter patches are patched in. Will keep looking into that. |
|
I'm down to one more issue on this, where it seemslike sometimes I'm losing track of a scroll offset or transform in the tree. |
|
This results in a number of small golden changes in g3, mainly because of removing layers/pixel snapping. There are a couple cases where images change: one is a semantics debugger where the colors are changing, and another looks like the widget acutally loads more - but all the visibility detection related tests are passing. |
|
I don' thave permission to add reviewers in this repo or to land. I'd appreciate any feedback from @jonahwilliams and @goderbauer on this. |
|
The internal CL and current golden changes are at cl/450011940 - you can see the associated tap run there if you are a Googler. |
| VisibilityDetectorLayer.forget(key); | ||
| super.paint(context, offset); | ||
| return; | ||
| if (onVisibilityChanged != null) { |
There was a problem hiding this comment.
Looks like paint here and in RenderSliverVisibilityDetector have a lot in common, the inly difference is the second argument passed to _scheduleUpdate? Maybe move paint to the base and have this class and RenderSliverVisibilityDetector implement just a method that returns the value for that second argument?
There was a problem hiding this comment.
Done, as mentioned elsehwere moved this to a Rect get bounds; on the mixin, which simplified things a bit.
| /// | ||
| /// This is used for testing, and always returns 0 outside of debug mode. | ||
| @visibleForTesting | ||
| int? get debugScheduleUpdateCount { |
There was a problem hiding this comment.
type as non-nullable int? (or make it return null in non-debug mode?)
| assert(!debugDisposed!); | ||
| final ContainerLayer? container = | ||
| layer is ContainerLayer ? layer : layer.parent; | ||
| _scheduleUpdate(container, semanticBounds); |
There was a problem hiding this comment.
Why semanticBounds over paintBounds or Offset.zero & size?
There was a problem hiding this comment.
It's what the original code used.
In the framework, semanticBounds and paintBounds are always the same, and for box classes they're Offset.zero & size :)
| } | ||
| } else if (_timer == null) { | ||
| // We use a normal [Timer] instead of a [RestartableTimer] so that changes | ||
| // to the update duration will be picked up automatically. |
There was a problem hiding this comment.
update duration = update interval?
How does this timer "automatically" pick up changes to that value?
There was a problem hiding this comment.
This is unchanged from the original.
I think the idea here is that if it were to use a RestartableTimer it'd be stuck with the originally supplied duration, as opposed to creating a new timer each time with the right duration.
| import 'package:flutter/foundation.dart'; | ||
|
|
||
| import 'visibility_detector_layer.dart'; | ||
| import 'render_visibility_detector.dart'; | ||
|
|
||
| /// A [VisibilityDetectorController] is a singleton object that can perform | ||
| /// actions and change configuration for all [VisibilityDetector] widgets. |
There was a problem hiding this comment.
(unrelated): The existence of this class is odd. A singleton instance that just calls static methods??
There was a problem hiding this comment.
I think this should be addressed in a separate PR
| // the markNeedsPaint above will never cause the composition callback to | ||
| // fire and we could miss a hide event. This schedule will get | ||
| // over-written by subsequent updates in paint, if paint is called. | ||
| _scheduleUpdate(null, semanticBounds); |
There was a problem hiding this comment.
Would the RenderSliverVisibilityDetector subclass have to substitute in something else for semanticBounds here? (It does so when this is called for regular paint)
There was a problem hiding this comment.
(could be more motivation to do the paint refactoring I mentioned elsewhere)
There was a problem hiding this comment.
Done - removed the parameter from this entirely and just have an abstract Rect get bounds on the mixin.
| // also ensures that they don't mutate the widget tree while we're in the | ||
| // middle of a frame. | ||
| if (isFirstUpdate) { | ||
| // We're about to render a frame, so a post-frame callback is guaranteed |
There was a problem hiding this comment.
I wonder if we could assert that "we're about to render a frame" just to make sure that there is no code pass where this doesn't get invoked.
There was a problem hiding this comment.
I'm not sure how we can do that - we might, for example, be in the middle of a frame and that still means this callback will fire, but hasScheduledFrame will be false.
This comment is legacy from the layer code. I suppose I could delete it?
| _lastVisibility.remove(key); | ||
| } | ||
|
|
||
| onVisibilityChanged?.call(info); |
There was a problem hiding this comment.
The VisibilityInfo object contains the newly added screenRect value. It appears that there is a code pass where the screenRect would change, but the onVisibilityChanged is not called (because of the early return above when matchesVisibility is true) to inform the owner about the callback about it, leaving them with potential outdated information. This could be surprising.
Not sure what the motivation was to add screenRect to that data class, but having an outdated value there could be a source of future bugs.
There was a problem hiding this comment.
Ahhh, I wanted to get rid of a global static dictionary.
The only thing that uses these are unit tests in this library. I suppose I could just drop that entirely. Otherwise we'd have to emit changes when just the screen rect changes, which maybe is fine but maybe is a small change in behavior that will cause problems for google consumers.
There was a problem hiding this comment.
So a case where I think you're right: if the widget resizes, but the resizing happens outside of the current clip. The visible bounds won't change but the widget bounds would.
I'm going to remove this as it's only used in tests.
There was a problem hiding this comment.
This remains my main concern as I can already see how people will rely on this and then get surprised if it doesn't update properly. I'd either remove this feature alltogether, or - if it is only used in test - make it a debug only feature that throws in release when accessed.
| @override | ||
| bool get alwaysNeedsCompositing => onVisibilityChanged != null; | ||
| void dispose() { | ||
| _compositionCallbackCanceller?.call(); |
There was a problem hiding this comment.
Do we need to "forget" this RO in dispose so the visibility logic is no longer triggered for it?
There was a problem hiding this comment.
Ah, never mind. You do want them to fire one more time after it got disposed it seems... Odd.
There was a problem hiding this comment.
Yes, that may be our only chance to let clients know that the RO is no longer visible (assuming it ever was). There are tests that fail if you add a forget here.
…r.dart Co-authored-by: Michael Goderbauer <[email protected]>
|
@jamesderlin I'm also interested in any feedback you have, if you have any to share :) |
|
@goderbauer the suggestion to initialize Unfortunately, CI is not set up for this repo... |
| // Remove all cached data so that we won't fire visibility callbacks when | ||
| // a timer expires or get stale old information the next time around. | ||
| forget(key); | ||
| _lastVisibility.remove(key); |
There was a problem hiding this comment.
Wondering, why does forget not also clear the lastVisibility for the key? Is there a use case where you want to forget, but still keep that historical information?
There was a problem hiding this comment.
Moving this to forget
| // the markNeedsPaint above will never cause the composition callback to | ||
| // fire and we could miss a hide event. This schedule will get | ||
| // over-written by subsequent updates in paint, if paint is called. | ||
| _scheduleUpdate(null); |
There was a problem hiding this comment.
nit: maybe make the parameters a required optional one so this call side reads a little better...
There was a problem hiding this comment.
Going to make it an optional optional :)
| /// The number of times the schedule update callback has been invoked from | ||
| /// [Layer.addCompositionCallback]. | ||
| /// | ||
| /// This is used for testing, and always returns 0 outside of debug mode. |
| }) : super(sliver) { | ||
| _onVisibilityChanged = onVisibilityChanged; | ||
| } |
There was a problem hiding this comment.
similar to above:
| }) : super(sliver) { | |
| _onVisibilityChanged = onVisibilityChanged; | |
| } | |
| }) : assert(key != null), | |
| _onVisibilityChanged = onVisibilityChanged, | |
| super(child); |
There was a problem hiding this comment.
Doesn't work - because the private field is on a mixin. Maybe this is a dart bug?
There was a problem hiding this comment.
I see. Interesting.
Maybe add the assert, though just for symmetry with the other constructor above..
| _lastVisibility.remove(key); | ||
| } | ||
|
|
||
| onVisibilityChanged?.call(info); |
There was a problem hiding this comment.
This remains my main concern as I can already see how people will rely on this and then get surprised if it doesn't update properly. I'd either remove this feature alltogether, or - if it is only used in test - make it a debug only feature that throws in release when accessed.
That IS unfortunate. Might be time to set that up? |
That's #364. |
|
Ahh actions aren't enabled. I'll revert my commit. |
This reverts commit 1ba8777.
Instead, use methods on RenderObject and new framework API to
listen to compositing and determine the transform/clip.
Removes an O(N^2) algorithm from the ancestor walk.
Deletes the custom layer that is no longer used.
Minor update to main.dart to be more desktop friendly.
Tests adjusted to use new API. Removed API has no usages
in internal codesearch.
Depends on flutter/flutter#103378