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

Added GPU instancing for DirectX #2047

Closed
wants to merge 9 commits into from
Closed

Conversation

ossaltu
Copy link

@ossaltu ossaltu commented Oct 2, 2013

Works on HiDef profile on DirectX projects. Has a difference with XNA framework interface: instead of doing this:
_device.SetVertexBuffers(
new VertexBufferBinding(vertexBuffer, 0, 0),
new VertexBufferBinding(instanceBuffer, 0, 1)
);
you should do it like this:
_device.SetVertexBuffer(vertexBuffer);
_device.SetInstanceBuffer(instanceBuffer);

Other API parts of instancing is same as in XNA.

@mgbot
Copy link
Member

mgbot commented Oct 2, 2013

Can one of the admins verify this patch?

1 similar comment
@mgbot
Copy link
Member

mgbot commented Oct 2, 2013

Can one of the admins verify this patch?

Change method names to match XNA naming
@tomspilman
Copy link
Member

@mgbot test

@tomspilman
Copy link
Member

This is great... thanks for contributing it!

Has a difference with XNA framework interface

I think that isn't really required to make it work. I suspect you missed the VertexBufferBinding.InstanceFrequency property which is what identifies a bound VertexBuffer as instance data.

/// <param name="primitiveCount">The number of primitives to render from the index buffer.</param>
/// <param name="instanceCount">The number of instance to render.</param>
/// <remarks>Note that minVertexIndex and numVertices are unused in MonoGame and will be ignored. The name of methos depicts the purpose of methos better then XNA's</remarks>
public void DrawIndexedInstancedPrimitives(PrimitiveType primitiveType, int baseVertex, int minVertexIndex, int numVertices, int startIndex, int primitiveCount, int instanceCount)
Copy link
Member

Choose a reason for hiding this comment

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

DrawIndexedInstancedPrimitives is not an XNA method... why have this instead of just DrawInstancedPrimitives?

@KonajuGames
Copy link
Contributor

I suspect you missed the VertexBufferBinding.InstanceFrequency property which is what identifies a bound VertexBuffer as instance data.

VertexBufferBinding is added as part of #1767 which is currently held up due to crashes in the OpenGL implementation.

@mgbot
Copy link
Member

mgbot commented Oct 2, 2013

Test PASSed.
Refer to this link for build results: http://build.monogame.net/job/PullRequestTester/451/

@tomspilman
Copy link
Member

VertexBufferBinding is added as part of #1767

Hum... looking at @KonajuGames implementation, i like how he internally it manages the vertex buffer bindings. It is more generalized.

@ossaltu - What are your thoughts?

@ossaltu
Copy link
Author

ossaltu commented Oct 2, 2013

True, @KonajuGames implementation is more general, but it does not conflict with my changes. I guess, it is ok to have several ways to use instancing api of monogame: simple one, I have implemented and more general/complex implemented by @KonajuGames which is based on VertexBufferBinding object and which can be used in other rendering scenarios.

DrawIndexedInstancedPrimitives is not an XNA method... why have this instead of just DrawInstancedPrimitives

I've added DrawInstancedPrimitives for XNA compatibility. DrawIndexedInstancedPrimitives is depicting the purpose of method better, because the method uses index buffer. DrawInstancedPrimitives feels like it does not use index buffer, while in XNA it actually it does. Latter DrawInstancedPrimitives override can be added which actually will not use index buffer. Hope it makes sense :)

@JonasSvegland
Copy link

When can one expect to see this in the developer installer or the develop branch? Or am I missing something?

@tomspilman
Copy link
Member

So this is heavily conflicted now and my issues were not answered or addressed.

Thanks for trying to contribute, I apologize we didn't get it merged.

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.

None yet

5 participants