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.

Sign up for GitHub

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

Jump to bottom

Make TextureSequence a public class #1999

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make TextureSequence a public class #1999

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e00b85
Make TextureSequence a public class and add docstrings.
RoboDoig Sep 3, 2024
97f9e22
Add missing docstring
RoboDoig Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion Bonsai.Shaders/TextureSequence.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,62 @@

namespace Bonsai.Shaders
{
class TextureSequence : Texture, ITextureSequence
/// <summary>
/// Represents a sequence of texture objects with an enumerator.
/// </summary>
public class TextureSequence : Texture, ITextureSequence
{
readonly TextureArray textures;

/// <summary>
/// Initializes a new instance of the <see cref="TextureSequence"/> class
/// with the specified number of texture objects in the internal <see cref="TextureArray"/>.
/// </summary>
/// <param name="length">
/// The total number of texture objects in the <see cref="TextureArray"/>.
/// </param>
public TextureSequence(int length)
: base(0)
{
textures = new TextureArray(length);
}

/// <summary>
/// Initializes a new instance of the <see cref="TextureSequence"/> class
/// with existing texture handles.
/// </summary>
/// <param name="textures">
/// The texture handles.
/// </param>
public TextureSequence(int[] textures)
{
this.textures = new TextureArray(textures);
}

/// <summary>
/// Gets the <see cref="TextureArray"/> associated with this texture sequence.
/// </summary>
public TextureArray Textures
{
get { return textures; }
}

/// <summary>
/// The target playback rate of the texture sequence.
/// </summary>
public double PlaybackRate { get; set; }

/// <summary>
/// Returns an enumerator that iterates through all texture objects in
/// the sequence.
/// </summary>
/// <param name="loop">
/// Boolean specifying whether the sequence should loop.
/// </param>
/// <returns>
/// An enumerator that can be used to iterate through the texture objects
/// in the sequence.
/// </returns>
public IEnumerator<ElementIndex<Texture>> GetEnumerator(bool loop)
Copy link
Member

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?

Copy link
Member

@glopesdev glopesdev Sep 23, 2024 •
edited
Loading

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

Copy link
Member

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.

{
var index = 0;
Expand Down