-
Notifications
You must be signed in to change notification settings - Fork 29
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
Make TextureSequence a public class #1999
base: main
Are you sure you want to change the base?
Conversation
/// <returns> | ||
/// An enumerator that can be used to iterate through the texture objects | ||
/// in the sequence. | ||
/// </returns> | ||
public IEnumerator<ElementIndex<Texture>> GetEnumerator(bool loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RoboDoig I think this method signature is probably the main reason why I didn't make this class public in the original design.
Having a GetEnumerator
method in a class that doesn't implement IEnumerable
and having it take a custom extra parameter would probably make some people raise their eyebrows. The original idea was to provide an allocation-free path for looping through infinite texture sequences without having to reallocate an iterator at the boundaries.
Unfortunately to top it off, the method also mutates the class state, by changing the Id
property inside the loop, which is what allows it to pose as a regular Texture
object at the same time. This was to allow creating simple video textures that you allocate globally and are constantly looping (i.e. you can set them to an object using the normal BindTexture
).
There is something about this overall class design that feels very broken. It feels like I maybe ran out of time while implementing the infrastructure for BonVision video textures, had meant to come back to this but never did.
You can see there is another internal class TextureStream
with very similar behavior but where there is only one texture which is updated / mutated in place (by copying frame data) as the iteration progresses.
What these two share in common is that they both implement ITextureSequence
which specifies an interface for an object providing a generic streaming sequence of textures.
TextureSequence
implements both Texture
and ITextureSequence
, but essentially ideally you never want to use these two interfaces at the same time.
Maybe it would be cleaner to make ITextureSequence
public and leave the details of these concrete implementations internal, but I'm not sure even that interface would be enough for your case. Do you need the TextureArray
which is stored internally inside TextureSequence
?
If you need random access to the textures inside, another alternative might be to have TextureSequence
implement IReadOnlyList<Texture>
. This way we can expose an array-like interface to the internal textures which would allow indexing without getting tangled up in all these messy implementation details.
@PathogenDavid any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternately we could make TextureSequence
implement IReadOnlyList<int>
(or maybe better being explicit and make a new interface ITextureArray : IEnumerable<int>
mimicking TextureArray
).
An example implementation might be:
#region ITextureArray Members
int ITextureArray.this[int index] => textures[index];
int ITextureArray.Length => textures.Length;
IEnumerator<int> IEnumerable<int>.GetEnumerator()
{
return textures.GetEnumerator();
}
IEnumerator IEnumerable.GetEnumerator()
{
return textures.GetEnumerator();
}
#endregion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PathogenDavid any thoughts on this?
Mostly reiterating my stance from dev club, but yeah I agree with the interface implementation approach here.
This class is basically just a wrapper that adapts TextureArray
to Texture
so that seems like a good way to expose the functionality without broadening the public API surface with a redundantish type that has weird quirks.
Relates to issue #1968
This PR makes TextureSequence in Bonsai.Shaders a public class, which allows for using extensions to determine the length of the texture array inside TextureSequence, Also adds the docstrings required for the class.