Created attachment 261117 [details] [review]
window-group: Decompose culling operations into two methods

We will add an interface for this soon, so we can recursively cull like this...

Created attachment 261118 [details] [review]
window-actor: Move the operations we need to do when we cull out here

Soon, we'll move this into a generic MetaCullable interface, but for
now, just put hardcoded knowledge in MetaWindowGroup.

Created attachment 261119 [details] [review]
cullable: Turn cull_out / reset_culling into a separate interface

Instead of hardcoded knowledge of certain classes in MetaWindowGroup,
create a generic interface that all actors can implement to get parts
of their regions culled out during redraw, without needing any special
knowledge of how to handle a specific actor.

When MetaWindowGroup is painted, we walk over every actor and ask
themselves to cull out the opaque parts of the region. A MetaCullable
implementation should do this by subtracting the opaque parts of its
actor out of the passed in region. We actually pass along two regions:
the "unobscured" region, which starts off with the full stage window,
and the "clip" region, which only contains the stage's paint clip.

Actors should copy the "clip" region and clip themselves to it during
painting, as painting won't automatically be culled...

After the painting is done, we call a reset function. MetaCullable
implementations should clear their copied clip regions at this part.

MetaCullable also has a simple recursive default implementation that is used
by MetaBackgroundGroup and friends. MetaBackgroundGroup is still needed as
a 'tagging' class to determine whether it is a background when syncing the
actor stacking order, but this may go away entirely in the future.

Review of attachment 261119 [details] [review]:

Mostly seems like a good reorganization.

I'd like to see docs for the interface (including things like coordinate systems) in the interface, and not in the commit message.

I would at least suggest that perhaps the default implementations should be helper functions and not default implementations - I'm not completely comfortable with the idea that interfaces can have default implementations :-)

Would also a) save trickiness with chaining up b) potentially allow culling through containers that don't implement MetaCullable - if you e.g., got rid of the MetaBackgroundGroup class.

Review of attachment 261118 [details] [review]:

Seems fine. (Assuming that docs get added to MetaCullable about the interface contract for these functions.)

Review of attachment 261117 [details] [review]:

Seems fine

Created attachment 261242 [details] [review]
cullable: Turn cull_out / reset_culling into a separate interface

Instead of hardcoded knowledge of certain classes in MetaWindowGroup,
create a generic interface that all actors can implement to get parts
of their regions culled out during redraw, without needing any special
knowledge of how to handle a specific actor.

Created attachment 261469 [details] [review]
window-group: Decompose culling operations into two methods

We will add an interface for this soon, so we can recursively cull like this...

Created attachment 261470 [details] [review]
window-actor: Move the operations we need to do when we cull out here

Soon, we'll move this into a generic MetaCullable interface, but for
now, just put hardcoded knowledge in MetaWindowGroup.

Created attachment 261471 [details] [review]
cullable: Turn cull_out / reset_culling into a separate interface

Instead of hardcoded knowledge of certain classes in MetaWindowGroup,
create a generic interface that all actors can implement to get parts of
their regions culled out during redraw, without needing any special
knowledge of how to handle a specific actor.

The names now are a bit suspect. MetaBackgroundGroup is a simple
MetaCullable that knows how to cull children, and MetaWindowGroup is the
"toplevel" cullable that computes the initial two regions. A future
cleanup here could be to merge MetaWindowGroup / MetaBackgroundGroup so
that we only have a generic MetaSimpleCullable, and move the "toplevel"
cullability to be a MetaCullableToplevel.

Review of attachment 261471 [details] [review]:

I like the improvements here over the last version. The only problem I see are in one area of code that doesn't get exercised because when we clone the window group we either:

 - Scale it
 - Show it at 0x0

But never show it at an offset without scaling.

::: src/compositor/meta-window-group.c
@@ +122,3 @@
 
+  cairo_region_translate (unobscured_region, paint_x_offset, paint_y_offset);
+  cairo_region_translate (clip_region, paint_x_offset, paint_y_offset);

There's actually a bug in the current code - unobscured_region shouldn't be affected by paint_x_offset, paint_y_offset. (I didn't catch this when unobscured_region was introduced.) Plus, the sign here is wrong.

Say that we're painting the window group with paint_x_origin/paint_y_origin at +10+10, and the clip_region is +10+10x100x100 - then paint_x_offset/paint_y_offset are 10,10 - and we want to offset clip_region *the other way* to get a clip in actor coordinates of +0+0x100x100.

I also think this should be done in the wrapper code not in cull_out - because it's in essence a fixup on the way we compute clip_region.

Review of attachment 261471 [details] [review]:

I like the improvements here over the last version. The only problem I see are in one area of code that doesn't get exercised because when we clone the window group we either:

 - Scale it
 - Show it at 0x0

But never show it at an offset without scaling.

::: src/compositor/meta-window-group.c
@@ +122,3 @@
 
+  cairo_region_translate (unobscured_region, paint_x_offset, paint_y_offset);
+  cairo_region_translate (clip_region, paint_x_offset, paint_y_offset);

There's actually a bug in the current code - unobscured_region shouldn't be affected by paint_x_offset, paint_y_offset. (I didn't catch this when unobscured_region was introduced.) Plus, the sign here is wrong.

Say that we're painting the window group with paint_x_origin/paint_y_origin at +10+10, and the clip_region is +10+10x100x100 - then paint_x_offset/paint_y_offset are 10,10 - and we want to offset clip_region *the other way* to get a clip in actor coordinates of +0+0x100x100.

I also think this should be done in the wrapper code not in cull_out - because it's in essence a fixup on the way we compute clip_region.

Created attachment 261483 [details] [review]
window-group: Decompose culling operations into two methods

This also fixes a bug in the translation of clip_region.

We will add an interface for this soon, so we can recursively cull like this...

Created attachment 261484 [details] [review]
window-actor: Move the operations we need to do when we cull out here

Soon, we'll move this into a generic MetaCullable interface, but for
now, just put hardcoded knowledge in MetaWindowGroup.

Created attachment 261485 [details] [review]
cullable: Turn cull_out / reset_culling into a separate interface

Instead of hardcoded knowledge of certain classes in MetaWindowGroup,
create a generic interface that all actors can implement to get parts of
their regions culled out during redraw, without needing any special
knowledge of how to handle a specific actor.

The names now are a bit suspect. MetaBackgroundGroup is a simple
MetaCullable that knows how to cull children, and MetaWindowGroup is the
"toplevel" cullable that computes the initial two regions. A future
cleanup here could be to merge MetaWindowGroup / MetaBackgroundGroup so
that we only have a generic MetaSimpleCullable, and move the "toplevel"
cullability to be a MetaCullableToplevel.

Review of attachment 261483 [details] [review]:

Looks good

Review of attachment 261484 [details] [review]:

Should be OK

Review of attachment 261485 [details] [review]:

Looks good

Attachment 261483 [details] pushed as 59a0113 - window-group: Decompose culling operations into two methods
Attachment 261484 [details] pushed as d8c6607 - window-actor: Move the operations we need to do when we cull out here
Attachment 261485 [details] pushed as 4714425 - cullable: Turn cull_out / reset_culling into a separate interface