Skip to content

Loading…

Split GraphicsDevice on a per-API basis #2243

Merged
merged 7 commits into from

6 participants

@JamesLupiani

This request cleans up the branching ifdefs and moves API-specific code from GraphicsDevice.cs into partial class definitions for each render API.

There are three goals at work here:
1. Eliminate confusing #ifdef-elif-elif branching
2. Separate platform-specific code while maintaining asserts/exceptions common to all
3. Avoid the performance hit of a base class and virtual methods

The first thing I did was to go through and eliminate nested #elif and catchall #else directives, with each API side by side in its own #if-#endif block. Since you would never have both OPENGL and DIRECTX defined, this is fine and it'd rightly fail to compile if you tried. This is probably worth doing even if we didn't go through with the other steps.

Next I isolated all in-method #if'd logic into methods called Platform{MethodName}. For example, PlatformDrawIndexedPrimitives. Assertions and exceptions common to all APIs stay in the main file to ensure runtime consistency.

Finally, I copied the file out three times, one for each API. Then I eliminated all platform-specific code from the main file, and all non-platform code from the copies. After switching the definition to a partial class in all files, everything compiles normally.

Using partial classes and chaining allows us to completely separate the APIs with only the overhead of a regular (non-virtual) method call. The process of porting is simply a matter of creating a new partial class file with the Platform* methods in it and adding it to the project file. If the ported file is missing or a Platform method isn't defined, the build fails and it's very easy to see where.

I recommend fetching the changes locally to inspect them side-by-side so it's easier to see what's going on.

@tomspilman
Mono Project member

@mgbot test

@mgbot

Can one of the admins verify this patch?

@mgbot

Can one of the admins verify this patch?

@tomspilman
Mono Project member

@mgbot test

@mgbot

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

@tomspilman
Mono Project member

We came up with a few guidelines in this process:

  1. If there is very little platform code in a function or class... stick with #ifs.
  2. Use the #if #endif #if #endif #if #endif pattern over #if #elseif #else #endif when possible.
  3. Always prefer partial classes over virtual interfaces when splitting out platform code.
  4. Split out code by common API and not platform... DirectX/OpenGL and not Android/iOS/WP8.
  5. Related partial classes should have the same file name prefix and reside in the same folder.
  6. Make sure your partial methods have a high "work" to function call cost ratio.
  7. The public API to the class should remain in the single shared source file.
@tomspilman
Mono Project member

@mgbot test

@mgbot

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

@tomspilman tomspilman commented on an outdated diff
MonoGame.Framework/Graphics/GraphicsDevice.DirectX.cs
((832 lines not shown))
+ catch (SharpDX.SharpDXException)
+ {
+ // TODO: How should we deal with a device lost case here?
+ /*
+ // If the device was removed either by a disconnect or a driver upgrade, we
+ // must completely reinitialize the renderer.
+ if ( ex.ResultCode == SharpDX.DXGI.DXGIError.DeviceRemoved ||
+ ex.ResultCode == SharpDX.DXGI.DXGIError.DeviceReset)
+ this.Initialize();
+ else
+ throw;
+ */
+ }
+
+#endif
+#if DIRECTX && !WINDOWS_PHONE
@tomspilman Mono Project member

I suspect this is a bug... WINDOWS_STOREAPP also defines DIRECTX.

You're right, on Windows 8 that would do a double present. It looks like the idea was to do it for Windows and not WindowsGL or WindowsPhone. How about I change it to WINDOWS && !OPENGL?

@tomspilman Mono Project member

How about I change it to WINDOWS && !OPENGL?

Well OpenGL won't ever be defined for this file... it is the DirectX implementation. I think maybe all you need is... #if !WINDOWS_PHONE. Right?

Works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomspilman tomspilman commented on an outdated diff
MonoGame.Framework/Graphics/GraphicsDevice.OpenGL.cs
((133 lines not shown))
+ GL.GetInteger(GetPName.MaxDrawBuffers, out maxDrawBuffers);
+ _drawBuffers = new DrawBuffersEnum[maxDrawBuffers];
+ for (int i = 0; i < maxDrawBuffers; i++)
+ _drawBuffers[i] = (DrawBuffersEnum)(FramebufferAttachment.ColorAttachment0Ext + i);
+#endif
+ _extensions = GetGLExtensions();
+ }
+
+ List<string> GetGLExtensions()
+ {
+ // Setup extensions.
+ List<string> extensions = new List<string>();
+#if GLES
+ var extstring = GL.GetString(RenderbufferStorage.Extensions);
+#endif
+#if !GLES
@tomspilman Mono Project member

For these near one like #if statements it probably isn't worth using the more explicit #if #endif #if pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomspilman
Mono Project member

@flibitijibibo @prollin

This is our take on the discussion started in #1944. It would be helpful to get your opinions if you have the time.

@prollin

I did a quick pass it seems in line with what I had in mind. A partial class is indeed better even if I doubt this is the main performance issue ppl using MonoGame run into :)

I would say that the next step would be to agree on a set of internal methods in GraphicsDevice for API specific graphics resource management. Something like this:


GraphicsDevice.CreateVertexBuffer(...);
GraphicsDevice.SetVertexBufferData(...);
GraphicsDevice.DeleteVertexBuffer(...);
...

Those method would be called by the GraphicsResource classes (VertexBuffer, IndexBuffer ... etc) and will allow to centralize all the API specific code to the corresponding GraphicsDevice class and keep all those GraphicsResource classes simple and platform independent.

@tomspilman
Mono Project member

A partial class is indeed better even if I doubt this is the
main performance issue ppl using MonoGame run into

I admit my concern over performance here was a bit overblown.

You would have to be doing 1000s of draw calls per frame to see a small change in frame rate from the additional partial function call we've added to each draw method here. The work done to perform the draw call, both in MonoGame and within OpenGL/DirectX, heavily outweighs the overhead of a function call.

Note we also investigated the .NET 4.5 "aggressive inlining" feature, but Mono seems to have some technical limits which make it unreliable for us.

I would say that the next step would be to agree on a set of
internal methods in GraphicsDevice for API specific graphics
resource management.

That is something to consider... we could move the code anywhere really.

I see the main drawback to moving it all into GraphicsDevice is one of the same reason that drove this split... it makes it a huge complex file and more difficult to work with. Do we want to scroll thru textures, render targets, swap chains, shaders, constant buffers, occlusion queries, and index buffer code when working on vertex buffers?

Our plan at the moment is doing the same split we did here to the other platform heavy classes. We will submit those in a separate PR after we get thru this one.

@MichaelDePiazzi - This would cause a conflict in your PR, but I see our changes here just making your changes easier to do.

@flibitijibibo

Looks cool!

Another thing that you might be able to do after this is pull in some of the GL code from the State classes, so in ApplyState, you can just get the properties from the state directly and do the GL work there:

https://github.com/flibitijibibo/MonoGame/blob/mgsdl2-opengldevice/MonoGame.Framework/Graphics/GraphicsDevice.cs#L1119

And it makes stuff like the Collections a wee bit smaller:

https://github.com/flibitijibibo/MonoGame/blob/mgsdl2-opengldevice/MonoGame.Framework/Graphics/SamplerStateCollection.cs
https://github.com/flibitijibibo/MonoGame/blob/mgsdl2-opengldevice/MonoGame.Framework/Graphics/TextureCollection.cs

Either way, this PR is really neat.

@tomspilman
Mono Project member

@flibitijibibo - I was browsing thru your repo the other day and saw that.

The state and collection classes are all pretty simple with under 300-500 lines of code each and have very little platform specific stuff. To me it seems like we got things right by keeping this split out from the main GraphicsDevice code.

What is the benefit of moving all the GL/DX code into one class?

@flibitijibibo

@tomspilman It's nice when debugging GL "stuff", since tracing the C# side is a little bit easier and you have an more compacted overview of what every little thing is doing. It also helps combine classes that might need to share data; for instance, Texture/SamplerStateCollection are best shared when doing OpenGL, since applying those two separately means performing multiple passes of glActiveTexture and possibly risks messing up the texparameters when avoiding redundant GL calls (something that actually did end up being a bug in EG2, but OpenGLTexture fixed this). It also compacts the per-platform behavior in a way that's easily found when trying to port; stuff like the CullMode having to be flipped around for OpenGL is one example. That could be buried somewhere in the RasterizerState, but if I can keep it in the GL device, that's something made easily visible if someone's wondering why the apitrace state output's different from their intention in the XNA code.

Then again, I do still have a lot of GL-specific code in GraphicsDevice, including GL calls, which I wish I could avoid. But that'll be FNA's problem, I guess.

That said, I'll totally admit to moving OpenTK GL code into an isolated location in order to eventually remove OpenTK/MiniTK as a dependency. That's definitely a personal thing at the moment.

@tomspilman
Mono Project member

It also helps combine classes that might need to share data;
for instance, Texture/SamplerStateCollection are best shared
when doing OpenGL,

That is a great point. We should look to bring texture and sampler state back together. While in DX it is pretty separate, that is not true of OpenGL and it does make things more error prone and less optimal.

@JamesLupiani JamesLupiani commented on an outdated diff
MonoGame.Framework/Graphics/GraphicsDevice.DirectX.cs
((832 lines not shown))
+ catch (SharpDX.SharpDXException)
+ {
+ // TODO: How should we deal with a device lost case here?
+ /*
+ // If the device was removed either by a disconnect or a driver upgrade, we
+ // must completely reinitialize the renderer.
+ if ( ex.ResultCode == SharpDX.DXGI.DXGIError.DeviceRemoved ||
+ ex.ResultCode == SharpDX.DXGI.DXGIError.DeviceReset)
+ this.Initialize();
+ else
+ throw;
+ */
+ }
+
+#endif
+#if !WINDOWS_PHONE

@tomspilman Okay, nevermind, something I'd thought of before made me look at the platform defines after the push. We can't just do !WINDOWS_PHONE because then WINDOWS_STORE will also pick it up, leaving us with the same problem--redundant calls to Present.

The point about OpenGL's right though, so that leaves us with just a straightforward #if WINDOWS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@prollin

@tomspilman

What is the benefit of moving all the GL/DX code into one class?

A lot of times there are some optimization that can be done more easily for platform that don't exactly map to the way XNA API was designed. If the GraphicsResource derived classes become "dumb" classes (a simple id mapped to an internal native handle), things can be made more efficient and it will be easier to add and maintain new or existing platforms. Actual creation/deletion of native resources could be delayed to the "GraphicsDevice.Present" to avoid most threading issue (or at least optimize it; this is actually how deletion of resources is done right now). All of this could be hidden away in one class that would have all the dirty platform details.

IMHO, somebody touching any of the GraphicsResource derived class should have a deep knowledge of the GraphicsDevice, this is one reason I think this would be the right thing to do in order to encourage quality contributions.

Back to some feedback, I would actually separate OpenGL from OpenGLES as those are quite different API with different behavior for what seems to be same API at first (fbo restriction, MSAA ... etc). This would also greatly simplify the OpenTK mess (I know this is changing, but the 2 API should not be considered the same) and allow mobile developer to contribute without affecting the Desktop folks (and vice versa) :)

@tomspilman
Mono Project member

@prollin - I see where you are coming from now.

For now I think this PR is ready... It is an improvement over where we were. I agree we will probably split OpenGL and OpenGL ES when we go SDL2, but I don't want to do that here.

@prollin

@tomspilman yeah I didn't expect my feedback about the next steps to be for this PR; one step at a time :)

@mgbot

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

@KonajuGames KonajuGames merged commit 460e68f into mono:develop

1 check passed

Details default Merged build finished.
@KonajuGames

Merged.

@JamesLupiani JamesLupiani deleted the SickheadGames:GraphicsDeviceSplit branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.