Add support for box shadows and out-of-geometry text items with the partial renderer #7450

tronical · 2025-01-24T15:28:11Z

tronical · 2025-01-24T15:31:09Z

internal/core/item_rendering.rs

+                *data.geometry()
            }
            None => {
                let cache_entry = crate::graphics::CachedGraphicsData::new(eval);
                let geom = cache_entry.data.clone();
                rendering_data.cache_index.set(cache.insert(cache_entry));
                rendering_data.cache_generation.set(cache.generation());
-                geom.geometry
+                *geom.geometry()


This function here, filter_item in the PartialRenderer is the reason why CachedItemGeometryAndTransform needs to store not only the translation but the entire geometry of an item.

As-is in this patch, CachedItemGeometryAndTransform uses 48 bytes on aarch64. If I change this code to fetch the geometry from the item directly, then I can reduce the geometry in CachedItemGeometryAndTransform to just a translation and the size goes to 40 bytes.

I see.
I think it would be better to re-query the geometry again (untracked).
The reason filter_item returns the geometry is because it had the geometry for free. But IMHO it is better to query it again (untracked) than keeping it in the cache.

We could optimize for the common case of

Geometry == bounding rect

bounding rect is empty

tronical · 2025-01-24T15:32:42Z

Marked as draft, as I'm not entirely sure yet that this is the correct approach. It does seem to work (yay), but it increases the size of the partial renderer cache quite a bit.

I could try to extend ItemRendererFeatures with SUPPORTS_DROP_SHADOW or so and then try to shrink CachedItemGeometryAndTransform.

internal/core/item_rendering.rs

ogoffart · 2025-01-24T16:44:42Z

internal/core/item_rendering.rs

+                *data.geometry()
            }
            None => {
                let cache_entry = crate::graphics::CachedGraphicsData::new(eval);
                let geom = cache_entry.data.clone();
                rendering_data.cache_index.set(cache.insert(cache_entry));
                rendering_data.cache_generation.set(cache.generation());
-                geom.geometry
+                *geom.geometry()


I see.
I think it would be better to re-query the geometry again (untracked).
The reason filter_item returns the geometry is because it had the geometry for free. But IMHO it is better to query it again (untracked) than keeping it in the cache.

We could optimize for the common case of

Geometry == bounding rect

bounding rect is empty

internal/core/items.rs

internal/core/items/image.rs

internal/core/items/text.rs

ogoffart · 2025-01-24T16:58:15Z

internal/core/items/text.rs

+        _self_rc: &ItemRc,
+        geometry: LogicalRect,
+    ) -> LogicalRect {
+        self.text_bounding_rect(window_adapter, geometry.cast()).cast()


I think that we register dependencies more than we need (eg, dependency on the text would be registed both by this call and by the render call)
dependencies cost in dependency nodes that are on the heap. We probably should make sure that bounding_rect_for_geometry is called without tracking.

You're right. For example a drop shadow's properties that affect the bounding rect are already tracked by the renderer's dirty tracker.

tronical · 2025-01-28T15:33:18Z

Ok, I think all the feedback is covered. I won't rebase this yet to facilitate there view, meanwhile the CI is red, but that's ok.

ogoffart

A test for the partial rendering would be nice with a shadow and some TouchArea (if the TouchArea moves it doesn't need to redraw the region)

ogoffart · 2025-01-28T18:37:45Z

internal/core/items.rs

    ) -> LogicalRect {
+        geometry.size = LogicalSize::zero();


Why not return Default::default()

I thought zero is a bit easier to read, emphasising that the size is zero. But I can change it to Default::default() if you feel stronger about it :)

no strong feeling.

internal/core/item_rendering.rs

internal/core/items.rs

tronical · 2025-01-29T08:11:00Z

A test for the partial rendering would be nice with a shadow and some TouchArea (if the TouchArea moves it doesn't need to redraw the region)

Good point, I'll try to add an automated test. I mostly used the manual one (my bad!), but now that it works this really makes sense.

cc #7247 Fix comment in partial renderer's filter_item() about dependency tracking Co-authored-by: Olivier Goffart <[email protected]>

cc #7247

…stureHandler These don't draw anything.

…respect the bounding rect Remove the geometry field and merely store the offset/transform. This brings the size down from 40 to 32 bytes on aarch64. Related, filter_item() now respects the item's bounding rect for the decision whether to draw the item or not.

…oxAndTransform

…ouch area's area to be redrawn

… the partial renderer

tronical marked this pull request as draft January 24, 2025 15:28

tronical requested a review from ogoffart January 24, 2025 15:28

tronical force-pushed the simon/bounding-rect branch from 1d734f1 to 88f9bdd Compare January 24, 2025 15:29

tronical commented Jan 24, 2025

View reviewed changes

tronical changed the title ~~Add support for box shadows with the partial renderer~~ Add support for box shadows and out-of-geometry text items with the partial renderer Jan 24, 2025

ogoffart reviewed Jan 24, 2025

View reviewed changes

ogoffart reviewed Jan 28, 2025

View reviewed changes

tronical force-pushed the simon/bounding-rect branch from 8ee5ad2 to cd6ef73 Compare January 29, 2025 11:30

tronical marked this pull request as ready for review January 29, 2025 11:30

ogoffart approved these changes Jan 30, 2025

View reviewed changes

tronical force-pushed the simon/bounding-rect branch from c134d34 to e908ec7 Compare January 30, 2025 09:14

tronical and others added 8 commits January 30, 2025 10:29

Add support for box shadows with the partial renderer

87baad8

cc #7247 Fix comment in partial renderer's filter_item() about dependency tracking Co-authored-by: Olivier Goffart <[email protected]>

Add support for bounding text rendering with the partial renderer

2ae05ac

cc #7247

Use a zero sized bounding rect for Empty/TouchArea/FocusScope/SwipeGe…

95b5a7e

…stureHandler These don't draw anything.

Cleanup: Rename bounding_rect_for_geometry to bounding_rect

9717b63

Cleanup: Rename CachedItemGeometryAndTransform to CachedItemBoundingB…

3571f9a

…oxAndTransform

Add a test that verifies that moving a touch area doesn't cause the t…

346a16a

…ouch area's area to be redrawn

Add a test that the box shadow's bounding rect is property tracked by…

001eb7a

… the partial renderer

tronical force-pushed the simon/bounding-rect branch from e908ec7 to 001eb7a Compare January 30, 2025 09:30

tronical merged commit 58a14fe into master Jan 30, 2025
37 checks passed

tronical deleted the simon/bounding-rect branch January 30, 2025 09:55

tronical mentioned this pull request Jan 30, 2025

Skia: Fix partial rendering #7247

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for box shadows and out-of-geometry text items with the partial renderer #7450

Add support for box shadows and out-of-geometry text items with the partial renderer #7450

tronical commented Jan 24, 2025

tronical Jan 24, 2025

ogoffart Jan 24, 2025

tronical commented Jan 24, 2025

ogoffart Jan 24, 2025

ogoffart Jan 24, 2025

tronical Jan 28, 2025

tronical commented Jan 28, 2025

ogoffart left a comment

ogoffart Jan 28, 2025

tronical Jan 29, 2025

ogoffart Jan 29, 2025

tronical commented Jan 29, 2025

Add support for box shadows and out-of-geometry text items with the partial renderer #7450

Add support for box shadows and out-of-geometry text items with the partial renderer #7450

Conversation

tronical commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tronical commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tronical commented Jan 28, 2025

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tronical commented Jan 29, 2025