-
Notifications
You must be signed in to change notification settings - Fork 615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Texture as data source #2351
base: main
Are you sure you want to change the base?
Conversation
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
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 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 Personally I think we could go with a wrapper if that's better for maintainability, but in that case I would then make
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!
I see what you mean, but this is just a semantics problem. I mean, learning that |
Due to If we think about what we want the user to be able to do, there are two main concepts:
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. |
I think we're already doing quite complicated convenience stuff with the
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
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! |
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 |
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. |
Fair enough :P |
Co-authored-by: melonora <[email protected]>
Co-authored-by: melonora <[email protected]>
Co-authored-by: melonora <[email protected]>
Note that this PR includes #2350 to correctly cast dtypes.
This PR is an attempt at implementing a form of "data source" in vispy. This stems from previous discussions both on napari channels and on the vispy gitter.
This particular implementation does not introduce a new object; instead, it expands the
gloo.Texture
object to be able to keep track of the data it represents on the CPU side as well. This is a WIP implementation which currently works well with theImageVisual
, but not all tests pass and probably breaks all sorts of other things. To see how it works, try the following:Then, you can alter the texture data like this:
Or access it from the texture + chnage it from the
Image
level:The ultimate goal is to then be able to easily do things like sharing textures between images:
without ending up with broken states. The above code works here, but I haven't tested it properly yet, and I'm sure it needs some checks and protections (and maybe disallowing changing texture class).
@djhoese does this resemble what you had in mind? I found it was a lot easier to work on the
Texture
object, rather than creating a wrapper around it.Might be interested and/or have good insight: @almarklein, @rossant
cc @melonora