Added support DrawInstancedPrimitives on OpenGL platforms by Jjagg · Pull Request #4920 · MonoGame/MonoGame

Jjagg · 2016-06-10T18:08:28Z

I made an attempt to implement instanced drawing for OpenGL. There is probably room for optimization in this PR, so it would be nice if someone had a nice hard look at it. I tested this with the xna mesh instancing sample, but there were some minor modifications. Test project link: https://github.com/Jjagg/InstancedSample

Fixes #4875, fixes #913, fixes #5744.

Jjagg · 2016-06-10T18:18:43Z

Looks like the build script is failing?

tomspilman · 2016-06-10T18:21:17Z

Just syntax errors in the Android build...

Graphics\GraphicsCapabilities.cs(221,37): error CS0117: 'OpenTK.Graphics.ES20.GL' does not contain a definition for 'VertexAttribDivisor' [C:\BuildAgents\MonoGameWin1\work\f26e5a04a66fcbdb\MonoGame.Framework\MonoGame.Framework.Android.csproj]
Graphics\GraphicsDevice.OpenGL.cs(127,28): error CS0117: 'OpenTK.Graphics.ES20.GL' does not contain a definition for 'VertexAttribDivisor' [C:\BuildAgents\MonoGameWin1\work\f26e5a04a66fcbdb\MonoGame.Framework\MonoGame.Framework.Android.csproj]
Graphics\GraphicsDevice.OpenGL.cs(1078,16): error CS0117: 'OpenTK.Graphics.ES20.GL' does not contain a definition for 'DrawElementsInstanced' [C:\BuildAgents\MonoGameWin1\work\f26e5a04a66fcbdb\MonoGame.Framework\MonoGame.Framework.Android.csproj]
Graphics\Vertices\VertexDeclaration.OpenGL.cs(73,24): error CS0117: 'OpenTK.Graphics.ES20.GL' does not contain a definition for 'VertexAttribDivisor' [C:\BuildAgents\MonoGameWin1\work\f26e5a04a66fcbdb\MonoGame.Framework\MonoGame.Framework.Android.csproj]

tomspilman · 2016-06-10T18:24:18Z

Also errors on the MonoMac build...

MonoGame.Framework/Graphics/GraphicsCapabilities.cs(221,37) : error CS0117: `MonoMac.OpenGL.GL' does not contain a definition for `VertexAttribDivisor'
MonoGame.Framework/Graphics/GraphicsDevice.OpenGL.cs(127,28) : error CS0117: `MonoMac.OpenGL.GL' does not contain a definition for `VertexAttribDivisor'
MonoGame.Framework/Graphics/GraphicsDevice.OpenGL.cs(1078,16) : error CS0117: `MonoMac.OpenGL.GL' does not contain a definition for `DrawElementsInstanced'
MonoGame.Framework/Graphics/Vertices/VertexDeclaration.OpenGL.cs(73,24) : error CS0117: `MonoMac.OpenGL.GL' does not contain a definition for `VertexAttribDivisor'

prollin · 2016-06-10T19:41:04Z

You might want to cache the currently bound vbo(s) (with associated offest + attribs) in order to avoid calling glBindBuffer and glVertexAttribPointer redundantly, those are expensive calls.

Jjagg · 2016-06-10T21:39:32Z

@tomspilman Should I just set some preprocessor directives to ignore those parts for MonoMac/GLES? MonoMac could handle instanced drawing though, right? The bindings just aren't there... But since it gets dropped some time soon(?) anyway, are preprocessor directives to ignore the breaking changes in this PR for now fine?

Jjagg · 2016-06-10T21:40:32Z

@prollin Sounds like a good idea. I'll take a look when I have the time. Note that this wasn't cached before either.

tomspilman · 2016-06-11T05:34:11Z

Should I just set some preprocessor directives to ignore those parts for MonoMac/GLES?

Yeah... #if to hack around things GLES can't do there is fine.

MonoMac could handle instanced drawing though, right?

I don't know myself... @dellis1972 @cra0zy @prollin ?

dellis1972 · 2016-06-11T09:47:16Z

There are extensions we can check too I think "GL_NV_draw_instanced " and "GL_EXT_draw_instanced". I know GLES 3.0 supports instancing but it can probably wait until we remove OpenTK for that.

Jjagg · 2016-06-11T10:19:52Z

Instanced drawing with GL_EXT_draw_instanced, GL_NV_draw_instanced and GL ES 3.0 works a bit different, because you can't use VertexAttribDivisor. You do get an instance identifier in your shader and can use that to do instance specific stuff (for example lookup in an array), you also get that instance id if you draw using the API as it is now (in the state of this PR), but you don't need it because you can set the attribute divisor. IMO it would be weird to use the same API for instanced drawing if we draw with that, because your shader has to be setup to use that instance id. 2 concerns I have with this:

We'd have to find some syntax in our shaders to denote the instance id.
This would only work on OpenGL.

Instanced drawing in DX with an instance ID seems to be possible as well, but not for all feature levels we support (I didn't look into this very much and I don't really know anything about DX). This would probably get really messy. Could be nice to support this in a way though, since most iOS and Android devices could benefit from instanced drawing as well.

EDIT: GLES 3.0 supports VertexAttribDivisor! For GL ES 2.0 there is EXT_instanced_arrays that could help us out (if GL_EXT_draw_instanced is also available). Looks like Apple devices support it, but not sure about Android device support, I can't find a lot of information for that...

Jjagg · 2016-06-11T17:51:05Z

So I'll just fix the build with #ifs for now. We can fix instanced drawing on mobile when OpenTK is replaced, It will just be a matter of loading the right entry points (maybe checking extensions) and removing the #ifs at that point. Sounds good?

Jjagg · 2016-06-11T20:49:50Z

@prollin On second thought I don't think we can do much better. I use the vertexBuffersDirty flag to check if attributes need to be applied. Since you can't change attribute binding without changing a vertex buffer due to how things work in MG with semantics and stuff, the vertexBuffersDirty flag maps 1-1 to if attributes should be applied. And when binding other vertex buffers, the check is done to see if it's not the same one and the dirty flag won't get set if it is. So I don't think we can do more caching there. Correct me if I'm wrong though, because I have a gut feeling I missed some caching opportunity. EDIT: It doesn't take offset into account, so that's an issue right now, I'll fix that.

prollin · 2016-06-11T23:33:32Z

It definitely would need a bit more work to support proper caching. Having a dirty flag + currentOffset per slot might work. There is also the case where buffers are null (rendering without vbo).

Jjagg · 2016-06-12T21:38:20Z

@prollin What do you mean when buffers are null? If you mean the DrawUser* functions, I left that as is, they don't use the ApplyAttribs function.

Looks like VertexBufferBinding.VertexOffset was not taken into account in binding before... Is it alright if I fix that in this PR? It matters for caching too.

I was thinking about using glDrawElementsInstancedBaseVertex, since that's available in OpenGL 3.2 (which we need for VertexAttribDivisor anyway), but in GLES it requires 3.2 which is higher than what we need for DrawElementsInstanced and VertexAttribDivisor (3.0), so that might make a difference for some devices. If we did use the BaseVertex variant it would mean we wouldn't need to rebind attributes when a different basevertex is passed in sequential draw calls. Should we do the BaseVertex version if it's available? We would have to use the regular version on GLES < 3.2 when we get there, which will require to branch based on GraphicsCapabilities.SupportsInstancedBaseVertex or something.

Jjagg · 2016-06-16T16:40:20Z

Exams have me tied up a bit, I'll continue with this in a week or so. I assume there's no rush since an issue for this has been open since 2012 :p

Jjagg · 2016-06-24T10:32:35Z

Exams are done, so I can continue with this. @dellis1972 Could you take a look at my comment above? Should I use BaseVertex versions if available?

Jjagg · 2016-07-01T15:39:19Z

I think it's better now, can anyone review and/or test for performance penalties compared to develop?
@dellis1972 @prollin

Jjagg · 2016-07-09T18:59:21Z

Bump @dellis1972 @prollin @tomspilman

tomspilman · 2016-07-26T15:39:58Z

@Jjagg - FYI. This wasn't your build failure... retriggering the build.

Jjagg · 2016-07-26T16:53:24Z

I figured as much since I just rebased.

Jjagg · 2016-09-10T00:11:23Z

I just tested instanced rendering with a voxel engine I'm working on and performance with this is slightly better than with DirectX. For a 160 000 voxel model I'm getting ~60 fps with DirectX (it's not capped by fixed time step or anything in case anyone is wondering, just a coincidence) and ~65 fps with OpenGL and this branch. Here's a screenshot for interested peeps

I think we should also compare performance for a game that does regular rendering with and without the changes of this branch, but I don't have anything lying around that I can use. I'll add some other renderers to the engine, so maybe later I can use it to test that too. I also didn't test mobile, but it uses the same code except it's the OpenTK binding instead of our own so it should Just Work. I think I can easily test Android (with an emulator) with the setup I have.
@dellis1972 @tomspilman

tomspilman · 2016-09-10T00:34:17Z

Looking good.

Two things to consider here before we can merge.

We have some instancing unit tests in here:

https://github.com/MonoGame/MonoGame/blob/develop/Test/Framework/Graphics/GraphicsDeviceTest.cs#L187

You should probably remove/change the #if block around them and see if those unit tests pass under OpenGL.

It would also be good to verify we didn't break normal GL rendering in any way. If the graphics unit tests work under GL then that would be one good way to do that.

Jjagg · 2016-09-10T00:40:26Z

It would also be good to verify we didn't break normal GL rendering in any way. If the graphics unit tests work under GL then that would be one good way to do that.

Sounds good. I'll take another stab at getting GL unit tests running. I couldn't get SDL working last time... I kept getting BadImageFormatException (wrong bitness), and with the other bitness configuration it complained that it couldn't find suitable GL drivers...

james0x0A · 2016-11-02T23:36:08Z

With #5273 and #5275 I have OpenGL unit tests running on Windows except for the issue of the native libraries having to be in the root. I get identical runs with the PR and develop, 682 pass, 38 fail, 23 error.

Jjagg · 2016-11-02T23:47:25Z

@james0x0A Can you try running the instanced rendering tests? They're here: https://github.com/MonoGame/MonoGame/blob/develop/Test/Framework/Graphics/GraphicsDeviceTest.cs#L205-L333
You just have to remove the preprocessor defines.

james0x0A · 2016-11-02T23:57:55Z

I am trying that now, but its a bit more complicated. It seems that the test compiles the shader on the fly and all of that is bound up with the content pipeline and mojo shader and so on.

This gives GraphicsDevice.OpenGL the ability to use multiple vertex buffers when rendering. I also implemented DrawInstancedPrimitives, which was easy after fixing the buffers.

This makes GraphicsDevice on OpenGL support multiple vertex buffers and implements GraphicsDevice.DrawInstancedPrimitives.

…#5429

Jjagg · 2017-01-23T21:32:58Z

You should probably remove/change the #if block around them and see if those unit tests pass under OpenGL.
It would also be good to verify we didn't break normal GL rendering in any way. If the graphics unit tests work under GL then that would be one good way to do that.

@tomspilman
I made a branch from develop where I merged this and #5429. All tests that pass in #5429 and the two instancing tests pass, so this is good to go! (I think caching is done right too)

Jjagg · 2017-03-17T01:01:25Z

@tomspilman @KonajuGames This has been here for a while. With it passing the GL tests can we get this merged soon? Probably best to wait for after 3.6.1 though, unless the plan is to cherry pick some commits for that.

tomspilman · 2017-03-17T03:52:17Z

unless the plan is to cherry pick some commits for that.

Yeah that is the plan... I will just cherry pick a few fixes for 3.6.1.

Jjagg · 2017-05-10T10:16:09Z

@tomspilman Can this get merged? Kind of annoying to keep rebasing and it already passed the DesktopGL tests, including the instancing tests.

tomspilman · 2017-06-06T06:50:19Z

@Jjagg - Rebase this once more and i'll merge it ASAP!

Jjagg · 2017-06-06T09:00:29Z

@tomspilman I merged develop!

tomspilman · 2017-06-06T21:24:39Z

Thanks @Jjagg .... next step would be testing this against the new OpenGL unit tests.

Added support DrawInstancedPrimitives on OpenGL platforms

tomspilman added this to the 3.7 Release milestone Jun 13, 2016

Jjagg force-pushed the instancedGL branch from 482f3c6 to 1e12e73 Compare July 1, 2016 10:56

rtabbara mentioned this pull request Jul 23, 2016

Move particle system computation over to GPU mono/CocosSharp#377

Open

Jjagg force-pushed the instancedGL branch from ef87f69 to 696c45b Compare July 25, 2016 09:40

Jjagg force-pushed the instancedGL branch from 696c45b to 4cde8a9 Compare August 28, 2016 18:37

Jjagg force-pushed the instancedGL branch from 4cde8a9 to 690c73f Compare November 2, 2016 18:12

Jjagg force-pushed the instancedGL branch from f0c35af to 79ac561 Compare December 31, 2016 11:13

Jjagg added 6 commits January 23, 2017 21:20

Implement SetVertexBuffers for OpenGL

763946a

This gives GraphicsDevice.OpenGL the ability to use multiple vertex buffers when rendering. I also implemented DrawInstancedPrimitives, which was easy after fixing the buffers.

[GL] Multiple vertex buffers and instanced drawing

603d57c

This makes GraphicsDevice on OpenGL support multiple vertex buffers and implements GraphicsDevice.DrawInstancedPrimitives.

Fix setting divisor when not supported

a08c4d6

Fix ignore instancing for GLES and MonoMac

e4fb6ba

Improve caching

1316fff

Fix wrong conflict resolution, I always do this :'(

195362a

Jjagg force-pushed the instancedGL branch from 79ac561 to 195362a Compare January 23, 2017 20:59

Jjagg added 2 commits January 23, 2017 22:11

C#7.0, I'm too modern for the build bots

0f2a4f2

Revert making MaxMSCount a property to prevent conflict with MonoGame…

da20a03

…#5429

Jjagg added 2 commits February 10, 2017 00:40

Merge branch 'develop' into instancedGL

a8f1263

Merge branch 'develop' into instancedGL

88aecf5

Jjagg added 2 commits March 26, 2017 13:51

Merge branch 'develop' into instancedGL

70d2ea8

Merge branch 'develop' into instancedGL

d1d6564

Jjagg mentioned this pull request May 31, 2017

GraphicsDevice.DrawInstancedPrimitives is not yet implemented for OpenGL. #5744

Closed

Merge branch 'develop' into instancedGL

77aa4a0

tomspilman changed the title ~~Instanced drawing for OpenGL~~ Added support DrawInstancedPrimitives on OpenGL platforms Jun 6, 2017

tomspilman merged commit b221e63 into MonoGame:develop Jun 6, 2017

Jjagg deleted the instancedGL branch April 26, 2018 15:38

nkast pushed a commit to nkast/MonoGame that referenced this pull request Jun 26, 2018

Instanced drawing for OpenGL (MonoGame#4920)

a844888

Added support DrawInstancedPrimitives on OpenGL platforms

Uh oh!

Conversation

Jjagg commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jjagg commented Jun 10, 2016

Uh oh!

tomspilman commented Jun 10, 2016

Uh oh!

tomspilman commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prollin commented Jun 10, 2016

Uh oh!

Jjagg commented Jun 10, 2016

Uh oh!

Jjagg commented Jun 10, 2016

Uh oh!

tomspilman commented Jun 11, 2016

Uh oh!

dellis1972 commented Jun 11, 2016

Uh oh!

Jjagg commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jjagg commented Jun 11, 2016

Uh oh!

Jjagg commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prollin commented Jun 11, 2016

Uh oh!

Jjagg commented Jun 12, 2016

Uh oh!

Jjagg commented Jun 16, 2016

Uh oh!

Jjagg commented Jun 24, 2016

Uh oh!

Jjagg commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jjagg commented Jul 9, 2016

Uh oh!

tomspilman commented Jul 26, 2016

Uh oh!

Jjagg commented Jul 26, 2016

Uh oh!

Jjagg commented Sep 10, 2016

Uh oh!

tomspilman commented Sep 10, 2016

Uh oh!

Jjagg commented Sep 10, 2016

Uh oh!

james0x0A commented Nov 2, 2016

Uh oh!

Jjagg commented Nov 2, 2016

Uh oh!

james0x0A commented Nov 2, 2016

Uh oh!

Jjagg commented Jan 23, 2017

Uh oh!

Jjagg commented Mar 17, 2017

Uh oh!

tomspilman commented Mar 17, 2017

Uh oh!

Jjagg commented May 10, 2017

Uh oh!

tomspilman commented Jun 6, 2017

Uh oh!

Jjagg commented Jun 6, 2017

Uh oh!

tomspilman commented Jun 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Jjagg commented Jun 10, 2016 •

edited

Loading

tomspilman commented Jun 10, 2016 •

edited

Loading

Jjagg commented Jun 11, 2016 •

edited

Loading

Jjagg commented Jun 11, 2016 •

edited

Loading

Jjagg commented Jul 1, 2016 •

edited

Loading