840979 - [layers-refactoring] Compositable classes should not depend on the texture's memory type

Status: RESOLVED INCOMPLETE

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

One of the ideas in the refactoring is that the TextureClient API should abstract away the use of direct texturing, shared memory, etc.

Yet we still have the CompositableTypes:
BUFFER_SHARED
BUFFER_DIRECT
BUFFER_CONTENT_DIRECT

and the CompositableClient classes that come with it (ImageClientShared, ImageClientDirect, etc.).

It may not be a priority to remove these specific compositables, but having them gets in the way of implementing some things cleanly and it makes the overall system complicated/ugly, especially on the client side.

So there's some serious cleanup to be done there, to ensure that we don't have Compositable classes that do the job of Texture classes.
It is not just a matter of aesthetics, I see quite a lot of unsafe downcasting on Textures in compositable classes, and having 3 classes where we should have 1 is bad for maintainability.

The aim of Compositable Host/Client is to handle all the {memory|backend}-agnostic work like buffer rotation or tiling. Not to do specific things for direct texturing or shared memory (which is the whole point of having TextureClient/TextureHost)

I'd say this bug is fixed when we stop having classes like ContentClientDirect, ImageClientDirect, or ImageClientShared (there are others).

The sooner we fix this, the easier it will bee to work with the refactoring, although I wouldn't make it a blocker for landing.

Comment 1

[Nick Cameron [:nrc]]

The trouble is that the goal of having a lightweight TextureClient/Host and have TextureClient/Host abstract away the kind of memory being used are in direct competition. In a way, Compositables are there to ease that conflict and know about the interface to Textures so that Layers don't have to.

I cannot see we can have lightweight Textures that do everything we want them to do and have a single interface. If we have more than one interface, then we need the Compositbale to be aware of which interface we use. Whether there is a cast or a GetType or implicit knowledge or a GetAs*(), we have the same problem but phrased differently.

I think we *can* make things a bit nicer, but we can never really solve this bug.

I guess we could debate whether to have a more heavyweight Texture class - that was my initial idea for what should happen and we didn't need Compositables, but I have grown to like the lightweight ones, mostly. There seems to be a lot of tension caused by having lightweight Textures. But now is not the time to think about that.

In any case, please lets not do any of this until we land.

Comment 2

[Nicolas Silva [:nical]]

I agree, though I think some of the classes can be factored into one, with only an additional 3 lines branch in UpdateImage rather than subclassing. 
I also totally agree that there is a compromise to be made between lightweight textures and less compositable classes, and that we should worry about this later.
I was kinda putting this bug here because at some point we were looking for simpler tasks that people not already involved in the refactoring could take to get used to the new architecture.

Comment 3

[Nicolas Silva [:nical]]

A lot of stuff has changed and this bug has become irrelevant.