Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

scene: add RECT node type #3140

Merged
merged 5 commits into from
Sep 2, 2021
Merged

scene: add RECT node type #3140

merged 5 commits into from
Sep 2, 2021

Conversation

djpohly
Copy link
Contributor

@djpohly djpohly commented Aug 26, 2021

Adds a new scene-graph node type for rendering solid-colored rectangles, which may be useful for simple window decorations. A little reworking was needed in the scene-graph implementation, since it would break the assumption that every rendered node has an associated surface.

Thoughts/questions:

  1. Should there be a wlr_scene_rect_set_size() and/or wlr_scene_rect_set_color() function, or is it straghtforward enough to change the fields directly? (Maybe damage tracking considerations tip the scale toward functions?)
  2. How should RECTs interact with wlr_scene_node_surface_at()?
  3. Would it complicate the scene-graph example too much if I were to add a handler for wlr_surface.commit? It would allow us to get the surface dimensions and draw something less ugly than what I currently have.
  4. Is there a point at which it makes more sense to implement additional node types with a scene_node_impl rather than switching on a type field?

@emersion
Copy link
Member

Should there be a wlr_scene_rect_set_size() and/or wlr_scene_rect_set_color() function, or is it straghtforward enough to change the fields directly? (Maybe damage tracking considerations tip the scale toward functions?)

Indeed, I'd prefer to use setters. While the setters are trivial right now, they're about to get more involved with damage tracking and output-layers considerations.

How should RECTs interact with wlr_scene_node_surface_at()?

That's a good question. I think for most use-cases we'll want rects to intercept the click events?

wlr_scene_node_surface_at obviously can't return a scene node, maybe a new function which returns a node would be useful. Not sure whether we should keep wlr_scene_node_surface_at or whether the new function would completely supersede it.

Would it complicate the scene-graph example too much if I were to add a handler for wlr_surface.commit? It would allow us to get the surface dimensions and draw something less ugly than what I currently have.

It would just set the size on commit right? I think that'd be fine yeah.

Is there a point at which it makes more sense to implement additional node types with a scene_node_impl rather than switching on a type field?

An interface struct with function pointers is mainly useful to allow third-parties to extend wlroots with their own node types. If we expect all node types to be built-in in wlroots I'm not yet sure this is a good idea. I guess it depends whether or not we can come up with a good interface API.

Some node type implementation details may need special handling. If building an abstraction makes the code more complicated it wouldn't be worth it.

We don't necessarily have to expose the scene_node_impl interface in our public API. If we do, that's additional maintenance costs and may tie our hands when we try to improve/refactor the scene-graph. OTOH, putting it in our public API would allow compositors to use the scene-graph even if they have their own custom rendering logic ().

Some interactions with things like output-layers aren't yet clear to me if we go down the interface route. I think I'd prefer to keep the enum for now and re-consider once we have all of the fancy features.

@djpohly
Copy link
Contributor Author

djpohly commented Aug 28, 2021

Added setters, updated the example, and created a wlr_scene_node_at function.

I went ahead and removed wlr_scene_node_surface_at after thinking about it a bit. It could either treat rects as opaque and return NULL even if there are surfaces at the given coordinates further down the tree, or it could treat them as transparent and continue searching, in which case you couldn't tell whether the returned surface was obscured or visible. Given a scene_surface node, it's easy enough to retrieve the associated surface, so I'd contend that the new function subsumes existing functionality.

(Edit: technically it subsumes the "opaque" functionality, so if we were to revive wlr_scene_node_surface_at, it should implement the "transparent" option.)

examples/scene-graph.c Outdated Show resolved Hide resolved
types/wlr_scene.c Outdated Show resolved Hide resolved
types/wlr_scene.c Outdated Show resolved Hide resolved
types/wlr_scene.c Outdated Show resolved Hide resolved
types/wlr_scene.c Outdated Show resolved Hide resolved
types/wlr_scene.c Outdated Show resolved Hide resolved
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks pretty good!

I'm still wondering about wlr_scene_node_surface_at: it doesn't seem like compositors can re-implement it with wlr_scene_node_surface_at, or at least not without making scene_surface_from_node public.

@djpohly
Copy link
Contributor Author

djpohly commented Sep 2, 2021

Ah, you're completely right. Somehow in my mind scene_surface_from_node was already public. Fixed.

@emersion
Copy link
Member

emersion commented Sep 2, 2021

Looks good to me!

Can you squash the commits into logical commits? See https://github.com/swaywm/wlroots/blob/master/CONTRIBUTING.md#commit-log

e.g. "scene: use memcpy to copy color arrays" can be squashed into "scene: add RECT node type"

This will allow us to create node types which are rendered but not
surface-based, such as a solid color or image.
RECT is a solid-colored rectangle, useful for simple borders or other
decoration.  This can be rendered directly using the wlr_renderer,
without needing to create a surface.
With the addition of a non-surface node type, it was unclear how such
nodes should interact with scene_node_surface_at().  For example, if the
topmost node at the given point is a RECT, should the function treat
that node as transparent and continue searching, or as opaque and return
(probably) NULL?

Instead, replace the function with one returning a scene_node, which
will allow for more consistent behavior across different node types.
Compositors can downcast scene_surface nodes via the now-public
wlr_scene_surface_from_node() if they need access to the surface itself.
Add RECT nodes to the scene-graph demo to illustrate how they are used.
Here, we add a solid rectangle behind each surface as a quick-and-dirty
border, handling surface.commit in order to size it appropriately.
@djpohly
Copy link
Contributor Author

djpohly commented Sep 2, 2021

I left the simplification to render_texture() as a separate commit since it wasn't actually part of implementing rects - let me know if it'd be preferable to have that merged with one of the others.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect, thanks!

@emersion emersion merged commit 00c2bae into swaywm:master Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants