feature: provide surface access via the Renderable

[mattkae]

Alternative to #3349

What's new?

• Now you can access the Surface via the Renderable

[RAOF]

So, I'm torn on this whole enterprise.

On one hand, this is the wrong spot to do this. Particularly, there's no guarantee that the compositor is actually going to call your Renderer; the MultiThreadedCompositor only calls into the renderer when it hasn't been able to place everything on a hardware plane¹.

On the other hand, even the Compositor only has access to a list of SceneElements, which doesn't include what you're after anyway.

On the third hand, the eternal scenegraph discussion. 🤷‍♀️

¹: Which, to be fair, it won't be able to at the moment, as that's not properly implemented.

[RAOF]

std::optional<pointer const*> is not super different to plain pointer const*; I could go either way on this; we should probably have an actual style guide decision on this.

[AlanGriffiths]

So long as we adhere to the Core Guidelines:

R.2: In interfaces, use raw pointers to denote individual objects (only)
R.3: A raw pointer (a T*) is non-owning

I'm also fine with a named Foo cont* provided that there's a clear indication (like _if_any) when nullptr is possible.

For reference, Sophie was keen on std::optional<>, so there's some of that around the codebase.

[mattkae]

I like std::optional personally because it shows intent. We don't null-check every shared_ptr in the repo, so I would assume that when we're accessing any pointer, we can assume that it is probably set.

[RAOF]

To be clear: this seems like a pragmatic solution to get something working; we've not prioritised getting a nice API for this so waiting for that is unreasonable.

[AlanGriffiths]

There are not problems in this code, but thinking on the usecase you mention...

Keeping pointer values in any sort of lookup needs care: a pointer is only valid until the memory it references is deallocated (typically part of deleting the referenced object). After that just accessing (not dereferencing) the pointer is undefined behaviour.

(Not merging pending discussion on std::optional<>)

[AlanGriffiths]

Suggested change

	return {};
	return std::nullopt;

[mattkae]

So, I'm torn on this whole enterprise.

On one hand, this is the wrong spot to do this. Particularly, there's no guarantee that the compositor is actually going to call your Renderer; the MultiThreadedCompositor only calls into the renderer when it hasn't been able to place everything on a hardware plane¹.

I was not aware that we wanted to do this! Would this mean that instead of going through the GL renderer, surfaces (somehow) just get blipped onto a hardware plane?

On the other hand, even the Compositor only has access to a list of SceneElements, which doesn't include what you're after anyway.

👍

On the third hand, the eternal scenegraph discussion. 🤷‍♀️

Yeah this might be the better solution, but I am afraid I would wait around a while for it, huh?

[codecov]

Codecov Report

Attention: Patch coverage is 18.75000% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 77.33%. Comparing base (25ade62) to head (4b3450a).

Files	Patch %	Lines
src/server/graphics/software_cursor.cpp	0.00%	2 Missing ⚠️
src/server/input/touchspot_controller.cpp	0.00%	2 Missing ⚠️
src/server/scene/basic_surface.cpp	60.00%	2 Missing ⚠️
src/server/shell/basic_idle_handler.cpp	0.00%	2 Missing ⚠️
tests/include/mir/test/doubles/fake_renderable.h	0.00%	2 Missing ⚠️
tests/include/mir/test/doubles/stub_renderable.h	0.00%	2 Missing ⚠️
tests/include/mir/test/doubles/mock_renderable.h	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3355      +/-   ##
==========================================
- Coverage   77.36%   77.33%   -0.04%     
==========================================
  Files        1075     1065      -10     
  Lines       68268    67883     -385     
==========================================
- Hits        52813    52494     -319     
+ Misses      15455    15389      -66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mattkae]

There are not problems in this code, but thinking on the usecase you mention...

Keeping pointer values in any sort of lookup needs care: a pointer is only valid until the memory it references is deallocated (typically part of deleting the referenced object). After that just accessing (not dereferencing) the pointer is undefined behaviour.

(Not merging pending discussion on std::optional<>)

For my use case, I won't be storing these references, but rather using them when they come to me to resolve back to the Window and its userdata. The pointer won't be invalidated by the time the render happens, so I should be okay on that front

[RAOF]

Yup. As I say, a pragmatic hack!