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

Use Span for SFML.Net API #263

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Marioalexsan
Copy link
Member

Making this draft as a starting point for Span-related changes. This might also be a good place to look over some memory allocations happening in the library. The idea is as follows:

  • Use System.Memory to gain access to Span, which is usable in .NET Standard 2.0
  • Replace T[] with Span in input parameters where possible - this is a trivial change that just allows slicing
  • Replace T[] returns with Span where possible (i.e. when we want to just give a view into memory) - this reduces the number of allocations and solves some anti-patterns like Image.Pixels

In some cases it's harder to use Span due to lack of methods that are available only in .NET Core. One such example is StreamAdaptor where you cannot read into a Span.

I'm also thinking we could use stackalloc for places that use patterns like byte[] titleAsUtf32 = Encoding.UTF32.GetBytes(title + '\0'). There is an overload for GetBytes that can work with pointers, which could be used together with stackalloc for small-sized strings to avoid allocating small temporary strings on the heap.

@Marioalexsan Marioalexsan marked this pull request as draft June 18, 2024 15:49
{
RenderStates.MarshalData marshaledStates = states.Marshal();

unsafe
{
fixed (Vertex* vertexPtr = vertices)
{
sfRenderTexture_drawPrimitives(CPointer, vertexPtr + start, (UIntPtr)count, type, ref marshaledStates);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to FORCE using slices? This removes the ability to specify offsets and lengths during Draw()

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I see it, the second overload is just a span-like version of the first one, so users could just do vertices.Slice(0, 3) if they want to do offsets and lengths.

{
unsafe
{
fixed (byte* PixelsPtr = pixels)
fixed (byte* ptr = pixels)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally fine with the changes to ptr vs PixelPtr everywhere, but that is purely semantics and it might be beneficial to actually create some coding style rules for the project that we don't currently have and this would be relevant to that discussion.

{
RenderStates.MarshalData marshaledStates = states.Marshal();

unsafe
{
fixed (Vertex* vertexPtr = vertices)
{
sfRenderWindow_drawPrimitives(CPointer, vertexPtr + start, (UIntPtr)count, type, ref marshaledStates);
sfRenderWindow_drawPrimitives(CPointer, vertexPtr, (UIntPtr)vertices.Length, type, ref marshaledStates);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as RenderTexture

return Encoding.UTF32.GetString(sourceBytes);
unsafe
{
return Encoding.UTF32.GetString((byte*)source, (int)( length * 4 ));
Copy link
Contributor

Choose a reason for hiding this comment

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

While I know that UTF32 is going to yield 4 bytes per char, does it make more sense to use sizeof() for clarity on this and the other uses of UTF*.GetString()?

@@ -310,14 +309,17 @@ public void SetPixel(uint x, uint y, Color color)
/// </summary>
/// <returns>Array of pixels</returns>
////////////////////////////////////////////////////////////
public byte[] Pixels
public Span<byte> Pixels
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it reasonable to add a set method for Pixels?

if (mySamplesArray.Capacity < mySamplesArray.Count + samples.Length)
mySamplesArray.Capacity = mySamplesArray.Count + samples.Length;

for (int i = 0; i < samples.Length; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there not a way that we can use AddRange() with Span? Also, Audio is likely going to benefit from a significant rework, but that's beyond the scope of this draft.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no such overload for AddRange() if I remember correctly.


unsafe
{
return new Span<short>((void*)ptr, (int)count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we .NET doesn't own the memory for samples, should we be handling this this way? Our Span could be invalidated if CSFML moves things IIRC

Copy link
Contributor

@charleyah charleyah Sep 26, 2024

Choose a reason for hiding this comment

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

This will be safe for the lifetime of the SoundBuffer object instance for as long as all public members on SoundBuffer class don't produce iterator invalidation on the underlying std::vector containing the samples on sf::SoundBuffer (m_samples).

@charleyah
Copy link
Contributor

Was there any consideration to use ReadOnlySpan<T> for the cases where SFML functions are reading from a span argument and not writing back to a caller provided buffer? At a glance that looks like the majority of cases.

Properties like Image.Pixels and SoundBuffer.Samples still make sense as Span<T> to allow callers to modify underlying data.

@Marioalexsan
Copy link
Member Author

@charleyah I haven't given it much thought initially, but yeah, we could use read-only spans in those places I think. I'd prefer to redo this PR after implementing all of the SFML.Net 3 changes though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants