Applied the suggested changes for MonoGame#6728, and docs #8370
Conversation
|
Looks good to me, a second review @mrhelmut ? |
|
Can we get a more descriptive commit message/PR description if possible please? |
| /// <param name="vertexBuffer">The vertex buffer to set</param> | ||
| public void SetVertexBuffers(VertexBufferBinding vertexBuffer) | ||
| { | ||
| _vertexBufferBindings[0] = vertexBuffer; |
There was a problem hiding this comment.
Clear the other elements? (i.e. 1, 2, 3) here and below? Also a prior call to SetVB(a1, a2, a3) should clean up after calling _vertexBuffers.Set otherwise it's not gonna be GC'ed?
| /// <returns> | ||
| /// <see langword="true"/> if the input layout was changed; otherwise, | ||
| /// <see langword="false"/>. | ||
| /// </returns> | ||
| public bool Set(params VertexBufferBinding[] vertexBufferBindings) | ||
| public bool Set(VertexBufferBinding[] vertexBufferBindings, int bindingCount) |
There was a problem hiding this comment.
May be add doc on the negative effects of passing a binding (and its effect on per-frame heap alloc).
IMO, I feel like the API user can do all this without introducing _vertexBufferBindings[] array at the GraphicsDevice level - you can maintain it yourself, can't you? But I realize you already made some params changes so will let others chime in here.
It's a good idea though, but could mostly live inside a doc comment as opposed to prescribing/adding complexity..
|
After some internal conversation, could you add some Unit Tests to ensure these enhancements are covered in testing please @Mindfulplays |
I agree too, The original change + this one seems to add complexity that could really be avoided/pushed outside, but tests should help contain it imo. @itsBuggingMe |
|
Is it possible to temporarily do what @Mindfulplays has suggested, and switch to span params once #8194 has completed and C# 13 is done? |
|
Giving a second thought to this... ... what is the advantage of this? You can pass an array to the params for it to generate no garbage. I feel like we're solving a non-issue here. |
|
It is not strictly necessary but would make the api more streamlined and convenient. Takes away another pitfall and one less thing for the user to think about. C# itself does this too to avoid allocations such as in |
Follow-up to #8281