I expressed some of my fears about the casting changes in my review of #2350. For this PR specifically, I'm concerned about the lack of separation of responsibilities. The low-level Texture classes are meant to be low-level. If this can't be done as a subclass or a new wrapper object that uses a low-level Texture class then fine, but I'm worried about the maintainability. Note the similar-in-purpose PR for buffers: #1607

As far as how I expected this to work, it is close, but I was hoping for more separation. I'd like the average user wanting to do something like to not have to know the term "texture" at all, hence the wrapper class idea. I'm also not sure a user should be getting the data source (texture) from another Visual to pass it to another. There is the more obvious ._texture private attribute usage that should be fixed long term, but I was hoping for something closer to this:

rand_data = np.random...
shared_data = DataSource(rand_data)
image2 = Image(shared_data, ...)
image.set_data(shared_data)

If we really want users to know about textures then we need to think hard on where the line is drawn between vispy high-level and GL. Knowing about vertices and meshes is one thing, but knowing about textures in your actual usage of a Visual is a much lower level concept.

The low-level Texture classes are meant to be low-level. If this can't be done as a subclass or a new wrapper object that uses a low-level Texture class then fine, but I'm worried about the maintainability. Note the similar-in-purpose PR for buffers: #1607

I think this is definitely doable either way. I went in this direction because It seemed less disruptive to how visuals interact with the underlying data. If we instead make a wrapper or subclass (such as in #1607), we need to effectively replace Textures and Buffers all over the codebase with the new wrappers (so at that point, why keep the Texture objects separate in the first place, if they're never used), or do like in #1607 and add everywhere the logic to choose if one wants to "use a cpu buffer". Note that this is not trivial, because every single visual uses and manages its buffers and textures very differently.

Personally I think we could go with a wrapper if that's better for maintainability, but in that case I would then make Texture basically a private thing that's there just for code separation.

I'm also not sure a user should be getting the data source (texture) from another Visual to pass it to another. There is the more obvious ._texture private attribute usage that should be fixed long term, but I was hoping for something closer to this:

rand_data = np.random...
shared_data = DataSource(rand_data)
image2 = Image(shared_data, ...)
image.set_data(shared_data)

Oh yeah I 100% agree on this; my example was like that just because that's a quick way to create a Texture with the correct setup, but your example is definitely how it should work!

As far as how I expected this to work, it is close, but I was hoping for more separation. I'd like the average user wanting to do something like to not have to know the term "texture" at all, hence the wrapper class idea.

If we really want users to know about textures then we need to think hard on where the line is drawn between vispy high-level and GL. Knowing about vertices and meshes is one thing, but knowing about textures in your actual usage of a Visual is a much lower level concept.

I see what you mean, but this is just a semantics problem. I mean, learning that Texture is the object that holds a reference to your data is not really harder than learning that it's called DataSource... And if you do know what a texture is, I don't think it's too confusing to learn that vispy keeps track of the data on CPU as well.

Personally I think we could go with a wrapper if that's better for maintainability, but in that case I would then make Texture basically a private thing that's there just for code separation.

Due to gloo being a public API we can't actually make Texture private. I think I'm mostly thinking code separation. The Texture class is a "dumb" wrapper around the GL concept with only a few conveniences implemented inside. If what you plan on adding are just more conveniences or the argument can be made that they are, then I'm OK with that. The other reason I was hoping for a separate class was that it would be easier to define it as a Protocol that can be re-implemented for many different data sources. I fear I might be combining two separate ideas again though. Or maybe not...

If we think about what we want the user to be able to do, there are two main concepts:

1. Allow the user to tell the Visual to share low-level data storage to improve performance and reduce waste.
2. Provide data to the Visual in an abstract way so the Visual doesn't care what it is getting, just that it knows how to get the data it wants from this object. This idea though is a black hole of possibilities (automatically updating GPU textures with dask data or tiles as they are made available, etc). This could also be thought of as a TextureWrapper or TextureProxy class that doesn't need to be implemented now and could provide the same interface as the Texture class for high-level stuff. If we assume that the Visuals will only modify the data from the Texture properties and not through .set_data then this may be easy to implement in the future.

I see what you mean, but this is just a semantics problem. I mean, learning that Texture is the object that holds a reference to your data is not really harder than learning that it's called DataSource... And if you do know what a texture is, I don't think it's too confusing to learn that vispy keeps track of the data on CPU as well.

Good point.

I guess if none of my ramblings above make you think of something else then stick with the Texture class modifications. Once the tests pass then we can have more discussions.

If what you plan on adding are just more conveniences or the argument can be made that they are, then I'm OK with that.

I think we're already doing quite complicated convenience stuff with the Scaled mixins. They are very cool and make working with Textures more intuitive, except they force you to do what the Image and Volume visuals are doing now and keep track of the "original" data yourself. In my mind moving this logic to the Texture not only makes working with the normal Texture itself easier (you can intuitively do texture[:2] = 2), but makes the Scaled mixins really work as intended.

This idea though is a black hole of possibilities (automatically updating GPU textures with dask data or tiles as they are made available, etc). This could also be thought of as a TextureWrapper or TextureProxy class that doesn't need to be implemented now and could provide the same interface as the Texture class for high-level stuff. If we assume that the Visuals will only modify the data from the Texture properties and not through .set_data then this may be easy to implement in the future.

Yeah this would be awesome, and it's along the idea of what we tried to do in napari with the TiledImageVisual, just more low-level and transparent. I agree that it's a bit out of scope for this; I would probably use a wrapper to Texture for that.

I guess if none of my ramblings above make you think of something else then stick with the Texture class modifications. Once the tests pass then we can have more discussions.

So far, I still think this is the right path :) I'll slowly chip away at all the weird issues and come back to you with a proper PR!

I think we're already doing quite complicated convenience stuff with the Scaled mixins. They are very cool and make working with Textures more intuitive, except they force you to do what the Image and Volume visuals are doing now and keep track of the "original" data yourself. In my mind moving this logic to the Texture not only makes working with the normal Texture itself easier (you can intuitively do texture[:2] = 2), but makes the Scaled mixins really work as intended.

Hm holding onto the input data array is not great when it comes to holding on to memory on the CPU when it doesn't need to be. Like uploading a texture that goes on a Mesh. Right now aren't we allowed to delete the numpy array that was given to the texture since the data now lives on the GPU?

I see what you mean... I guess in that case it's convenient to have a way to flush the CPU buffer, and disable part of the functionality. Or like in #1607, make it opt-in, and otherwise automatically "disable" all this (unless it's a CPU scaled mixin, which requires holding onto the original data).

Even if it CPU scaled it isn't required. I mean, only if the limits are changed and it needs to be re-scaled.

The other thing about doing complicated convenience stuff in the scaled mixins is that none of that was supposed to be seen by the user. It was just combining two shared functionalities between the ImageVisual and VolumeVisual.

Even if it CPU scaled it isn't required. I mean, only if the limits are changed and it needs to be re-scaled.

Fair enough :P