Support for RenderTarget2D multisampling under DirectX. #5538

Jure-BB · 2017-03-03T01:56:45Z

Added RenderTarget2D multisampling for DirectX. Code is based of this post.

Changes in first commit are

Multisampled resource is now created
Added RenderTarget2D field to RenderTarget2D class. It serves as non-multisampled destination for resolving multisampled RT.
Basic resolving logic

First commit is just to get it working with little consideration to performance. Resolving probably needs to be optimized (resolve only if needed).

This depends on #5477

Conflicts: MonoGame.Framework/Graphics/GraphicsCapabilities.DirectX.cs MonoGame.Framework/Graphics/GraphicsDevice.DirectX.cs

Applied cleanup and fixes that were suggested. PresentationParameters.MultiSampleCount is now updated when clamped.

Jure-BB · 2017-03-03T12:02:09Z

Is there any way of knowing if RT has been written to? So it could be marked as dirty and resolved only in that case.

Right now RTs are resolved when they are unset. It would be possible to improve performance in some cases if RT was resolved only when its data is used: when it is used as a source texture for drawing or on GetData. And of course only if its data has changed since last resolve.

Any thoughts on this?

KonajuGames · 2017-03-03T14:20:43Z

Is there any way of knowing if RT has been written to? So it could be marked as dirty and resolved only in that case.

If a render target has been set, assume it is dirty. It will lose contents when being set anyway, unless RenderTargetUsage.PreserveContents has been used (do we even support flag properly?). If a developer sets and unsets a render target without drawing to it, that is a bad move on their part and the render target now has undefined contents.

KonajuGames · 2017-03-03T14:27:00Z

MonoGame.Framework/Graphics/Texture.DirectX.cs

@@ -27,33 +28,33 @@ public IntPtr GetSharedHandle()

        internal abstract SharpDX.Direct3D11.Resource CreateTexture();

-        internal SharpDX.Direct3D11.Resource GetTexture()
+        internal virtual SharpDX.Direct3D11.Resource GetTexture()


This method was made virtual, but I don't see anything that overrides it. We try to keep the use of virtual to a minimum because it takes longer to call a virtual than a regular method.

Its a leftover from debugging. I forgot to remove it.

Jure-BB · 2017-03-03T14:33:21Z

If a render target has been set, assume it is dirty.

Then resolving logic is done too, I think.

do we even support flag properly?

Check here It seems OK to me.

hach-que · 2017-05-11T03:48:20Z

Can this get merged? We have a need for multisampled render targets in Protogame because of how our render pipeline works.

I'm going to temporarily merge this changes into our patched version of MonoGame, but it'd be great to have this fixed properly upstream.

hach-que · 2017-05-11T04:09:45Z

(I have a rebased version of this PR that no longer has merge conflicts if anyone is interested)

Jure-BB · 2017-05-11T09:43:12Z

There are no known issues, so I think it can be merged. (I'll just rebase it)

Jure-BB · 2017-05-11T11:46:33Z

Hmm does anyone have any idea why there is a change "Submodule Dependencies updated from 3aee60 to 979abf" under "Files Changed" tab? I didn't make any submodule dependencies changes for this PR.

hach-que · 2017-05-12T01:51:20Z

I can confirm that this change works as intended. The way I rebased it was I hard reset to the RenderTarget2D_Multisampling branch (before that merge that just occurred), then soft reset to the parent on the develop parent so that it would only contain the branches changes. Then I committed it, and rebased that single commit, which gave me just the four files that were changed (rebasing the whole history turned out to be hard with lots of conflicts).

KonajuGames · 2017-05-13T12:43:52Z

There has been no response to my code review comments re. GetShaderResourceView().

KonajuGames · 2017-05-13T12:44:27Z

And unnecessary changing of field names.

Jure-BB · 2017-05-13T12:50:42Z

@KonajuGames where can I see those review comments? I can only see comment about virtual GetTexture() method

Jure-BB · 2017-05-13T12:55:12Z

And unnecessary changing of field names.

Some fields were renamed because they were changed from private to protected. Should I revert to old names with "_" prefix for protected fields?

KonajuGames · 2017-03-23T12:55:59Z

MonoGame.Framework/Graphics/Texture.DirectX.cs

@@ -35,25 +36,25 @@ internal SharpDX.Direct3D11.Resource GetTexture()
            return _texture;
        }

-        internal SharpDX.Direct3D11.ShaderResourceView GetShaderResourceView()
+        internal virtual SharpDX.Direct3D11.ShaderResourceView GetShaderResourceView()


Rather than making this commonly used property a virtual, create a new virtual method for the allocation of the ShaderResourceView and override that method in the RenderTarget2D class. Then we only pay the cost of the virtual call when the ShaderResourceView needs to be allocated.

In Texture.DirectX.cs

internal SharpDX.Direct3D11.ShaderResourceView GetShaderResourceView() { if (_resourceView != null) return _resourceView; return CreateShaderResourceView(); } protected internal virtual SharpDX.Direct3D11.ShaderResourceView CreateShaderResourceView() { _resourceView = new SharpDX.Direct3D11.ShaderResourceView(GraphicsDevice._d3dDevice, GetTexture()); return _resourceView; }

In RenderTarget2D.DirectX.cs

protected internal override SharpDX.Direct3D11.ShaderResourceView CreateShaderResourceView() { if (MultiSampleCount <= 1) return base.CreateShaderResourceView(); _resourceView = new SharpDX.Direct3D11.ShaderResourceView(GraphicsDevice._d3dDevice, _resolvedTexture.GetTexture()); return _resourceView; }

And please don't change the naming of existing fields. We have the leading underscore for a reason. See the coding standards.

The number of changes would be a far bit smaller if the naming was reverted back to what it was. A smaller PR is a lot easier to accept.

Is it OK if I do it like this, as I changed _resourceView back to private?

internal SharpDX.Direct3D11.ShaderResourceView GetShaderResourceView() { if (_resourceView == null) _resourceView = CreateShaderResourceView(); return _resourceView; } protected internal virtual SharpDX.Direct3D11.ShaderResourceView CreateShaderResourceView() { return new SharpDX.Direct3D11.ShaderResourceView(GraphicsDevice._d3dDevice, GetTexture()); }

KonajuGames · 2017-03-23T13:01:56Z

MonoGame.Framework/Graphics/Texture2D.DirectX.cs

-
-        private SampleDescription _sampleDescription;
+        protected internal bool shared;
+        protected internal bool mipmap;


Another unnecessary change of existing field names.

Microsoft's design guidelines don't actually allow public or protected instance fields, so they don't have a naming convention for them. Though R# likes to PascalCase them by default. That's what I always do too. I thought the most applied convention was to only prefix private fields with an underscore.

I'd be fine with those fields staying private and with the underscore prefix (as they were before this PR), and a adding new protected PascalCase property used by derived classes that returns the private field.

KonajuGames · 2017-05-13T13:30:01Z

Ahh, it appears I had to click another button to actually submit the review comments. Learning to use the new GitHub features.

Yes, those fields should remain named as they were.

Jure-BB · 2017-05-13T13:53:59Z

OK, I see review comments now. I'll make the changes :)

Jure-BB · 2017-05-14T08:35:37Z

Any ideas why Package Mac and Linux fails, as only changes made are to .DirectX.cs files?

KonajuGames · 2017-05-14T08:54:26Z

It build all platforms regardless of what changed. We're having issues with the Mac build machine at the moment. It's not behaving nicely.

Jure-BB · 2017-06-11T10:06:13Z

Resolved new merge conflicts. Added dispose call for _resolvedTexture.
Can this PR be merged, so I don't waste time resolving new conflicts?

Jure-BB · 2017-06-22T12:05:50Z

I think I discovered a possible bug, so I'll need to test it before this PR is ready.

Jure-BB · 2017-06-23T20:35:45Z

I just found out that bug had already been fixed in #5766. So this is good to go IMO.

hach-que · 2017-06-26T07:48:13Z

@tomspilman @KonajuGames Can this please be merged so I don't have to keep rebasing and squashing it on the Redpoint fork?

tomspilman · 2017-06-26T10:02:26Z

MonoGame.Framework/Graphics/RenderTarget2D.DirectX.cs

+
+        internal void ResolveSubresource()
+        {
+            GraphicsDevice._d3dContext.ResolveSubresource(this._texture, 0, _resolvedTexture._texture, 0,


GraphicsDevice._d3dContext needs to be lockd here.

Should I also add null and/or IsDisposed checks for _texture & _resolvedTexture, before calling _d3dContext.ResolveSubresource() method? I am not sure if there can be any scenario where null/disposed can happen, but I also don't want to add unnecessary checks.

Should I also add null and/or IsDisposed checks

I don't think we have to do that right now.

tomspilman · 2017-06-27T07:37:49Z

Thanks @zigzag312 ... merging.

Funny enough I implemented this same feature for XB1 a week or two ago.

Support for RenderTarget2D multisampling under DirectX.

zigzag312 added 15 commits February 15, 2017 21:31

DirectX: Fixed multisample clamping logic

f9e85b4

DirectX: Fixed multisample clamping logic

e271274

Merge remote-tracking branch 'refs/remotes/origin/develop'

a5a52fd

Conflicts: MonoGame.Framework/Graphics/GraphicsCapabilities.DirectX.cs MonoGame.Framework/Graphics/GraphicsDevice.DirectX.cs

Merge conflict resolution

ce220bb

Windows Phone build fix

9bdcd47

Applied cleanup and fixes that were suggested

7a6d888

Applied cleanup and fixes that were suggested. PresentationParameters.MultiSampleCount is now updated when clamped.

Removed unnecessary comment

037bc26

Merged branch develop into develop

81b3f8e

Added unit tests

9ad081d

Refactored to make some methods reusable

824ab3b

Bugfix

d855f63

More refactoring

2d94fe3

Merged branch develop into RenderTarget2D_Multisampling

7bcf79f

Implemented RenderTarget2D Multisampling

c41a585

Fixed wrong check if RT is multisampled

8bf2cca

Jure-BB changed the title ~~RenderTarget2D Multisampling~~ [DirectX] RenderTarget2D Multisampling Mar 3, 2017

KonajuGames reviewed Mar 3, 2017

View reviewed changes

Removed virtual from GetTexture()

a428c3a

Jure-BB mentioned this pull request Mar 24, 2017

Multisampling not working for RenderTarget2D #1162

Open

Merge branch 'MonoGameDevelop' into RenderTarget2D_Multisampling

dbf29a6

ThirdParty Dependencies update

6e60e9c

KonajuGames reviewed May 13, 2017

View reviewed changes

zigzag312 added 2 commits May 13, 2017 20:33

Refactoring

ea42991

More refactoring

986fa49

zigzag312 added 3 commits June 11, 2017 10:35

Merge branch 'MonoGameDevelop' into RenderTarget2D_Multisampling

61d43ea

Resolve conflict

a9fca88

Dispose for _resolvedTexture

412a0ed

tomspilman reviewed Jun 26, 2017

View reviewed changes

Added lock to _d3dContext before resolving

2489a65

tomspilman changed the title ~~[DirectX] RenderTarget2D Multisampling~~ Support for RenderTarget2D multisampling under DirectX. Jun 27, 2017

tomspilman merged commit fe465e1 into MonoGame:develop Jun 27, 2017

roy-t mentioned this pull request Dec 28, 2017

Enabling MSAA on RenderTargets on Windows breaks rendering after #5538 #6124

Closed

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

[DirectX] RenderTarget2D Multisampling (MonoGame#5538)

b384d1b

Support for RenderTarget2D multisampling under DirectX.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for RenderTarget2D multisampling under DirectX. #5538

Support for RenderTarget2D multisampling under DirectX. #5538

Jure-BB commented Mar 3, 2017

Jure-BB commented Mar 3, 2017

KonajuGames commented Mar 3, 2017

KonajuGames Mar 3, 2017

Jure-BB Mar 3, 2017

Jure-BB commented Mar 3, 2017

hach-que commented May 11, 2017

hach-que commented May 11, 2017

Jure-BB commented May 11, 2017

Jure-BB commented May 11, 2017 •

edited

hach-que commented May 12, 2017

KonajuGames commented May 13, 2017

KonajuGames commented May 13, 2017

Jure-BB commented May 13, 2017

Jure-BB commented May 13, 2017 •

edited

KonajuGames Mar 23, 2017

KonajuGames Mar 23, 2017

KonajuGames Mar 23, 2017

Jure-BB May 13, 2017

KonajuGames Mar 23, 2017

Jjagg May 13, 2017

KonajuGames May 13, 2017

KonajuGames commented May 13, 2017 •

edited

Jure-BB commented May 13, 2017

Jure-BB commented May 14, 2017

KonajuGames commented May 14, 2017 via email

Jure-BB commented Jun 11, 2017

Jure-BB commented Jun 22, 2017

Jure-BB commented Jun 23, 2017 •

edited

hach-que commented Jun 26, 2017

tomspilman Jun 26, 2017

Jure-BB Jun 26, 2017

tomspilman Jun 26, 2017

tomspilman commented Jun 27, 2017

Support for RenderTarget2D multisampling under DirectX. #5538

Support for RenderTarget2D multisampling under DirectX. #5538

Conversation

Jure-BB commented Mar 3, 2017

Jure-BB commented Mar 3, 2017

KonajuGames commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jure-BB commented Mar 3, 2017

hach-que commented May 11, 2017

hach-que commented May 11, 2017

Jure-BB commented May 11, 2017

Jure-BB commented May 11, 2017 • edited

hach-que commented May 12, 2017

KonajuGames commented May 13, 2017

KonajuGames commented May 13, 2017

Jure-BB commented May 13, 2017

Jure-BB commented May 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KonajuGames commented May 13, 2017 • edited

Jure-BB commented May 13, 2017

Jure-BB commented May 14, 2017

KonajuGames commented May 14, 2017 via email

Jure-BB commented Jun 11, 2017

Jure-BB commented Jun 22, 2017

Jure-BB commented Jun 23, 2017 • edited

hach-que commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomspilman commented Jun 27, 2017

Jure-BB commented May 11, 2017 •

edited

Jure-BB commented May 13, 2017 •

edited

KonajuGames commented May 13, 2017 •

edited

Jure-BB commented Jun 23, 2017 •

edited