Implement texture arrays for Direct3D #3654

Merged
merged 10 commits into from Mar 28, 2015

Conversation

Projects
None yet
4 participants
@tgjones
Contributor

tgjones commented Mar 23, 2015

Currently only implemented for Direct3D.

A prerequisite for OpenGL support is MojoShader support for texture array samplers in HLSL.

Fixes #3653.

@tgjones tgjones changed the title from Implement texture arrays to Implement texture arrays for Direct3D Mar 23, 2015

@mgbot

This comment has been minimized.

Show comment
Hide comment
@mgbot

mgbot Mar 23, 2015

Member

TeamCity MonoGame :: Develop (Win) Build 3.4.0.74 is now running

Member

mgbot commented on cf3327d Mar 23, 2015

TeamCity MonoGame :: Develop (Win) Build 3.4.0.74 is now running

This comment has been minimized.

Show comment
Hide comment
@mgbot

mgbot Mar 23, 2015

Member

TeamCity MonoGame :: Develop (Win) Build 3.4.0.74 outcome was SUCCESS
Summary: Tests passed: 466, ignored: 6 Build time: 00:14:24

Member

mgbot replied Mar 23, 2015

TeamCity MonoGame :: Develop (Win) Build 3.4.0.74 outcome was SUCCESS
Summary: Tests passed: 466, ignored: 6 Build time: 00:14:24

+ SetRenderTargets(new RenderTargetBinding(renderTarget, arraySlice));
+ }
+
+ // Only implemented for DirectX right now, so not in GraphicsDevice.cs
public void SetRenderTarget(RenderTarget3D renderTarget, int arraySlice)

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

I forget.... what is the difference here in RenderTarget3D and RenderTarget2D? Are we basically making the same types now?

@tomspilman

tomspilman Mar 23, 2015

Member

I forget.... what is the difference here in RenderTarget3D and RenderTarget2D? Are we basically making the same types now?

This comment has been minimized.

@tgjones

tgjones Mar 23, 2015

Contributor

This is from MSDN:

When a 3D texture mipmap slice is bound as a render target output (with a render-target view), the 3D texture behaves identically to a 2D texture array with n slices. The particular render slice is chosen from the geometry-shader stage, by declaring a scalar component of output data as the SV_RenderTargetArrayIndex system-value.
There is no concept of a 3D texture array; therefore a 3D texture subresource is a single mipmap level.

A RenderTarget3D, and a RenderTarget2D with arraySize > 1, are actually different things, and stored differently. But when you bind them as render targets, they're treated as the same thing.

@tgjones

tgjones Mar 23, 2015

Contributor

This is from MSDN:

When a 3D texture mipmap slice is bound as a render target output (with a render-target view), the 3D texture behaves identically to a 2D texture array with n slices. The particular render slice is chosen from the geometry-shader stage, by declaring a scalar component of output data as the SV_RenderTargetArrayIndex system-value.
There is no concept of a 3D texture array; therefore a 3D texture subresource is a single mipmap level.

A RenderTarget3D, and a RenderTarget2D with arraySize > 1, are actually different things, and stored differently. But when you bind them as render targets, they're treated as the same thing.

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

Interesting... so they are different things. I got concerned there for a second.

@tomspilman

tomspilman Mar 23, 2015

Member

Interesting... so they are different things. I got concerned there for a second.

This comment has been minimized.

@tgjones

tgjones Mar 23, 2015

Contributor

Actually... TextureCube, and Texture2D with arraySize > 1, are basically the same thing. The only difference, in Direct3D anyway, is in the BindFlags. (And same goes for RenderTargetCube and RenderTexture2D).

@tgjones

tgjones Mar 23, 2015

Contributor

Actually... TextureCube, and Texture2D with arraySize > 1, are basically the same thing. The only difference, in Direct3D anyway, is in the BindFlags. (And same goes for RenderTargetCube and RenderTexture2D).

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

Actually... TextureCube , and Texture2D with arraySize > 1 , are basically the same thing.

Except I think TextureCube is sampled by a normal and filters across the different faces where as Texture2D does not. Right?

@tomspilman

tomspilman Mar 23, 2015

Member

Actually... TextureCube , and Texture2D with arraySize > 1 , are basically the same thing.

Except I think TextureCube is sampled by a normal and filters across the different faces where as Texture2D does not. Right?

This comment has been minimized.

@tgjones

tgjones Mar 23, 2015

Contributor

Yep, in HLSL, there's certainly a TextureCube type. But on the D3D side, there's no TextureCube type - only a Texture2D with arraySize == 6 and BindFlags == BindFlags.TextureCube.

That said, I don't think it's a problem for MG to have both. The fact that one is a special case of the other is a D3D (and maybe actual hardware?) implementation detail. It's not the same in OpenGL. I don't know about Vulkan / D3D12, but we can cross that bridge later.

For now, I think having TextureCube / RenderTargetCube classes matches what users expect.

@tgjones

tgjones Mar 23, 2015

Contributor

Yep, in HLSL, there's certainly a TextureCube type. But on the D3D side, there's no TextureCube type - only a Texture2D with arraySize == 6 and BindFlags == BindFlags.TextureCube.

That said, I don't think it's a problem for MG to have both. The fact that one is a special case of the other is a D3D (and maybe actual hardware?) implementation detail. It's not the same in OpenGL. I don't know about Vulkan / D3D12, but we can cross that bridge later.

For now, I think having TextureCube / RenderTargetCube classes matches what users expect.

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

I think having TextureCube / RenderTargetCube classes matches what users expect.

I agree.

@tomspilman

tomspilman Mar 23, 2015

Member

I think having TextureCube / RenderTargetCube classes matches what users expect.

I agree.

@@ -18,6 +18,7 @@ internal protected enum SurfaceType
internal int width;
internal int height;
+ internal int ArraySize;

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

Make this _arraySize here. As eventually after we reformat code that is what it will be.

@tomspilman

tomspilman Mar 23, 2015

Member

Make this _arraySize here. As eventually after we reformat code that is what it will be.

This comment has been minimized.

@tgjones

tgjones Mar 23, 2015

Contributor

I made it pascal-case because it's internal, not private - it is referenced from RenderTargetBinding. (And it's a field, not a property, for efficiency.)

Should I still rename it?

@tgjones

tgjones Mar 23, 2015

Contributor

I made it pascal-case because it's internal, not private - it is referenced from RenderTargetBinding. (And it's a field, not a property, for efficiency.)

Should I still rename it?

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

Interestingly the MS C# style guide used for the new .NET core project doesn't mention internals:

https://github.com/dotnet/corefx/wiki/Coding-style

So looking thru their code base I was able to find one example case:

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/ThreadInfo.cs#L15

So I would say yeah... rename it.

@tomspilman

tomspilman Mar 23, 2015

Member

Interestingly the MS C# style guide used for the new .NET core project doesn't mention internals:

https://github.com/dotnet/corefx/wiki/Coding-style

So looking thru their code base I was able to find one example case:

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/ThreadInfo.cs#L15

So I would say yeah... rename it.

This comment has been minimized.

@tgjones

tgjones Mar 23, 2015

Contributor

Ha, 3 possibilities! I'm fairly sure I remember seeing internal field == Pascal case mentioned in an MS style guide some years ago, but I have no source to back me up. It's what ReSharper suggests... if I'm allowed to use that as a source :)

@tgjones

tgjones Mar 23, 2015

Contributor

Ha, 3 possibilities! I'm fairly sure I remember seeing internal field == Pascal case mentioned in an MS style guide some years ago, but I have no source to back me up. It's what ReSharper suggests... if I'm allowed to use that as a source :)

This comment has been minimized.

@tomspilman

tomspilman Mar 23, 2015

Member

Sure... leave it for now since things are unclear there.

@tomspilman

tomspilman Mar 23, 2015

Member

Sure... leave it for now since things are unclear there.

This comment has been minimized.

@tomspilman

tomspilman Mar 24, 2015

Member

You can still leave this as is, but I got MS to clarify the rules for internals in their style guide:

https://github.com/dotnet/corefx/wiki/Coding-style

3.We use _camelCase for internal and private members and use readonly where possible. Prefix instance fields with _ , static fields with s_ and thread static fields with t_.

@tomspilman

tomspilman Mar 24, 2015

Member

You can still leave this as is, but I got MS to clarify the rules for internals in their style guide:

https://github.com/dotnet/corefx/wiki/Coding-style

3.We use _camelCase for internal and private members and use readonly where possible. Prefix instance fields with _ , static fields with s_ and thread static fields with t_.

This comment has been minimized.

@tgjones

tgjones Mar 24, 2015

Contributor

That was quick :) I don't think much of the prefixes for static and thread-static fields, that feels quite old-school - but I guess MS get to decide...

@tgjones

tgjones Mar 24, 2015

Contributor

That was quick :) I don't think much of the prefixes for static and thread-static fields, that feels quite old-school - but I guess MS get to decide...

@tgjones

This comment has been minimized.

Show comment
Hide comment
@tgjones

tgjones Mar 24, 2015

Contributor

Let me know if there's anything more you'd like me to do here. No problem if you want to let it brew for a couple of days to give people time to have a look at it.

Contributor

tgjones commented Mar 24, 2015

Let me know if there's anything more you'd like me to do here. No problem if you want to let it brew for a couple of days to give people time to have a look at it.

@KonajuGames

This comment has been minimized.

Show comment
Hide comment
@KonajuGames

KonajuGames Mar 24, 2015

Contributor

As usual you've done a great job with it, @tgjones. I think the comments so far have covered everything I would have brought up.

Contributor

KonajuGames commented Mar 24, 2015

As usual you've done a great job with it, @tgjones. I think the comments so far have covered everything I would have brought up.

@mgbot

This comment has been minimized.

Show comment
Hide comment
@mgbot

mgbot Mar 26, 2015

Member

TeamCity MonoGame :: Develop (Win) Build 3.4.0.77 is now running

Member

mgbot commented on 3c4f3d4 Mar 26, 2015

TeamCity MonoGame :: Develop (Win) Build 3.4.0.77 is now running

This comment has been minimized.

Show comment
Hide comment
@mgbot

mgbot Mar 26, 2015

Member

TeamCity MonoGame :: Develop (Win) Build 3.4.0.77 outcome was SUCCESS
Summary: Tests passed: 466, ignored: 6 Build time: 00:14:24

Member

mgbot replied Mar 26, 2015

TeamCity MonoGame :: Develop (Win) Build 3.4.0.77 outcome was SUCCESS
Summary: Tests passed: 466, ignored: 6 Build time: 00:14:24

Add test for mipmapped array texture
and fix bug exposed by new test, where Texture2D.GetData returned
incorrect results for level > 0
@tgjones

This comment has been minimized.

Show comment
Hide comment
@tgjones

tgjones Mar 26, 2015

Contributor

I just fixed a bug that would have been rather embarrassing if it had been merged as it was. I added a test for that case. Then I fixed an existing bug (exposed by the new test) in Texture2D.DirectX.cs, in GetData, where it returned the wrong data for level > 0.

Contributor

tgjones commented Mar 26, 2015

I just fixed a bug that would have been rather embarrassing if it had been merged as it was. I added a test for that case. Then I fixed an existing bug (exposed by the new test) in Texture2D.DirectX.cs, in GetData, where it returned the wrong data for level > 0.

@mgbot

This comment has been minimized.

Show comment
Hide comment
@mgbot

mgbot Mar 26, 2015

Member

TeamCity MonoGame :: Develop (Win) Build 3.4.0.79 is now running

Member

mgbot commented on c0271d7 Mar 26, 2015

TeamCity MonoGame :: Develop (Win) Build 3.4.0.79 is now running

This comment has been minimized.

Show comment
Hide comment
@mgbot

mgbot Mar 26, 2015

Member

TeamCity MonoGame :: Develop (Win) Build 3.4.0.79 outcome was SUCCESS
Summary: Tests passed: 469, ignored: 6 Build time: 00:14:57

Member

mgbot replied Mar 26, 2015

TeamCity MonoGame :: Develop (Win) Build 3.4.0.79 outcome was SUCCESS
Summary: Tests passed: 469, ignored: 6 Build time: 00:14:57

@tgjones

This comment has been minimized.

Show comment
Hide comment
@tgjones

tgjones Mar 28, 2015

Contributor

@tomspilman This is ready to merge, from my point of view. I've tested it in a couple of projects.

Contributor

tgjones commented Mar 28, 2015

@tomspilman This is ready to merge, from my point of view. I've tested it in a couple of projects.

@tomspilman

This comment has been minimized.

Show comment
Hide comment
@tomspilman

tomspilman Mar 28, 2015

Member

Yeah... i'm good with this. Lets merge!

Member

tomspilman commented Mar 28, 2015

Yeah... i'm good with this. Lets merge!

tomspilman added a commit that referenced this pull request Mar 28, 2015

Merge pull request #3654 from tgjones/texture-arrays
Implement texture arrays for Direct3D

@tomspilman tomspilman merged commit 8e05231 into MonoGame:develop Mar 28, 2015

1 check passed

default Finished TeamCity Build MonoGame :: Develop (Win) : Tests passed: 469, ignored: 6
Details

@tgjones tgjones deleted the tgjones:texture-arrays branch Apr 4, 2015

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