Skip to content
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

Add support for binding textures directly from a TextureSequence #1770

Merged
merged 1 commit into from
May 10, 2024

Conversation

PathogenDavid
Copy link
Member

Fixes #1645

@PathogenDavid
Copy link
Member Author

Something I realized while applying pattern matching to the other path is that these two paths handle getting the texture ID at different times. (My path reads it in the shader.Update function, the other one reads it in the Observable.Defer and captures it for the closure passed to shader.Update.)

Not entirely sure if that has any observable behavior differences. My gut says that my implementation is probably slightly better since consumers of Texture probably shouldn't make assumptions about the ID since they don't own it, but let me know if you want me to make them match either direction.

@glopesdev
Copy link
Member

My path reads it in the shader.Update function, the other one reads it in the Observable.Defer and captures it for the closure passed to shader.Update

This can be an important point, the main difference is that the timing of the code outside the closure can be controlled by the input sequence, whereas running code inside the closure we cannot make any assumptions about when exactly it will be run. This can be important when Index is changed dynamically just before the call is made, e.g. using InputMapping. Sometimes we do this to emulate batch execution of render pipelines.

Because of this detail, if anything I would tend to err on the side of capturing outside the closure. The easiest way to do this would be to change the signature of the delegate to receive the captured nullable, e.g.:

        IObservable<TSource> Process<TSource>(IObservable<TSource> source, Action<int?, TSource> update)

@PathogenDavid
Copy link
Member Author

Because of this detail, if anything I would tend to err on the side of capturing outside the closure. The easiest way to do this would be to change the signature of the delegate to receive the captured nullable, e.g.:

That's sensible. Looking at it closer though, I ended up refactoring the function to take a getter for the texture ID instead of an update function.

Your suggestion would've only unified the capture of Index, but the actual texture ID was still different.

It's a bit more of a change, but I think it better matches the existing behavior exactly in both cases.

Bonsai.Shaders/BindTexture.cs Outdated Show resolved Hide resolved
Bonsai.Shaders/BindTexture.cs Outdated Show resolved Hide resolved
@PathogenDavid
Copy link
Member Author

Revisiting this again with a fresh mind, I realized we were both getting a little caught up on the update delegate approach of the previous implementation.

The latest iteration does away with the delegate entirely and instead has Process<TSourcce> handle the input being a texture when the texture name isn't specified.

This technically makes the Process(IObservable<Texture>) overload redundant, but I kept it so that it's still documented separately since I felt like that probably makes more sense from a user perspective. (Can remove and revise the documentation if you feel it's unnecessary though.)

The diff looks like I changed documentation but I actually just moved the body of the private Process(IObservable, Action) into the body of Process<TSource> since the helper is no longer useful after removing the Action.

As an aside, is it desirable that we just nothing when there's no texture? Seems like it should maybe be an error.

@PathogenDavid PathogenDavid requested a review from glopesdev May 8, 2024 18:31
@PathogenDavid PathogenDavid force-pushed the issue-1645 branch 2 times, most recently from 51a8822 to 5f8df5a Compare May 9, 2024 17:36
Copy link
Member

@glopesdev glopesdev left a comment

Choose a reason for hiding this comment

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

Sorry for the confusion in my previous comment, it took me a while to fully reload the context of this operator and while doing several edits to the comment I probably failed to explain myself correctly.

I think this will be the last iteration and happy to go back to having everything in one method. This is basically the same as your previous proposal except introducing an extra local instead of renaming the cache variable.

Indeed this operator is surprisingly nuanced, and I learned several things from this review so definitely worthwhile!

Bonsai.Shaders/BindTexture.cs Show resolved Hide resolved
@glopesdev glopesdev added the fix Pull request that fixes an issue label May 9, 2024
@glopesdev glopesdev added this to the 2.8.3 milestone May 9, 2024
@glopesdev glopesdev merged commit 88a2412 into bonsai-rx:main May 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BindTexture does not allow indexing dynamic TextureSequence
2 participants