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

Add output merger UAV binding. #478

Merged
merged 2 commits into from Aug 29, 2019
Merged

Add output merger UAV binding. #478

merged 2 commits into from Aug 29, 2019

Conversation

@WhyPenguins
Copy link
Contributor

WhyPenguins commented Jun 24, 2019

PR Details

Previously, unordered access views could only be bound to the compute stage. This patch lets them be bound to the output merger stage, so pixel shaders can read and write to them.

Description

Much of the patch is based on how UAV binding for the compute stage works - there is an array of currently bound UAVs, and in SetUnorderedAccessView this is checked against and the UAV is bound if necessary.

However due to how Direct3D ties render targets and UAVs together (see here), its also necessary to keep track of the number of bound render targets, and to re-bind all the UAVs whenever one is changed. The patch prioritises render targets, in that if the targets and UAV registers are interleaved for whatever reason, it'll ensure all the render targets stay bound. Hopefully that makes sense haha.

Related Issue

I originally posted about this here: #471

Motivation and Context

There are several techniques that need the pixel shader to be able to write to arbitrary locations, such as voxelization, or order independent transparency.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Haha I'll admit I'm not really sure what to do regarding proper tests. At the very least I can say I've bound multiple UAVs (buffers and textures), and have successfully written and read from them. I imagine something like that'd be a good test to write? I'll look into getting that done.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jun 24, 2019

CLA assistant check
All committers have signed the CLA.

@xen2 xen2 self-assigned this Jul 19, 2019
@xen2 xen2 added the area-Graphics label Jul 19, 2019
/// <param name="shaderResourceView">The shader resource view.</param>
/// <param name="view">The native unordered access view.</param>
/// <param name="uavInitialOffset">The Append/Consume buffer offset. See SetUnorderedAccessView for more details.</param>
internal void OMSetSingleUnorderedAccssView(int slot, SharpDX.Direct3D11.UnorderedAccessView view, int uavInitialOffset)

This comment has been minimized.

Copy link
@xen2

xen2 Jul 25, 2019

Member

Typo: Accss => Access

uavInitialCounts[i] = -1;
uavInitialCounts[slot - currentRenderTargetViewsActiveCount] = uavInitialOffset;

outputMerger.SetUnorderedAccessViews(currentRenderTargetViewsActiveCount, uavs, uavInitialCounts);

This comment has been minimized.

Copy link
@xen2

xen2 Jul 25, 2019

Member

Note: double-checked into SharpDX code and it seems they use D3D11_KEEP_RENDER_TARGETS_AND_DEPTH_STENCIL which is what we want. All good!


int remainingSlots = currentUARenderTargetViews.Length - currentRenderTargetViewsActiveCount;

var uavs = new SharpDX.Direct3D11.UnorderedAccessView[remainingSlots];

This comment has been minimized.

Copy link
@xen2

xen2 Jul 25, 2019

Member

Let's preallocate this (as a class private field) to avoid allocation during typical frame.

var uavs = new SharpDX.Direct3D11.UnorderedAccessView[remainingSlots];
Array.Copy(currentUARenderTargetViews, currentRenderTargetViewsActiveCount, uavs, 0, remainingSlots);

var uavInitialCounts = new int[remainingSlots];

This comment has been minimized.

Copy link
@xen2

xen2 Jul 25, 2019

Member

Let's preallocate this (as a class private field) to avoid allocation during typical frame.

@WhyPenguins

This comment has been minimized.

Copy link
Contributor Author

WhyPenguins commented Aug 8, 2019

Ah sorry for replying so late! Thanks so much for the review, will correct the patch (that typo is embarrassing, sorry).

My C# skills being as rusty as they are though, do you have any tips on how to pre-allocate those arrays?

The reason I currently allocate, is so I can pass a slice of the currentUARenderTargetViews array to SetUnorderedAccessViews. The only way I know how to do this in C# without an allocation is with ArraySegment, but I can't pass one to SetUnorderedAccessViews. Any ideas?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Aug 14, 2019

@WhyPenguins No problem for the typo, we had/have many of them as well!

For preallocating, the idea is to create a field which preallocate the array:
private readonly SharpDX.Direct3D11.UnorderedAccessView[] temporaryUAVs = new SharpDX.Direct3D11.UnorderedAccessView[SimultaneousRenderTargetCount];
Do same for initial counts.
Then just use this array inside OMSetSingleUnorderedAccessView rather than creating a new one every call.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Aug 14, 2019

@WhyPenguins Concerning preallocation, it might not be even possible to do with current SharpDX API (it only takes start offset but not count -- which is not going to be equal to array length if start offset is not 0).

@WhyPenguins

This comment has been minimized.

Copy link
Contributor Author

WhyPenguins commented Aug 16, 2019

Yeah that was my concern, sorry if I wasn't clear enough. It's unfortunate since there's an internal method that does end up taking a pointer and count, but yeah it can't be used.

I suppose if the pre-allocation is important, a workaround could be an array of arrays with increasing sizes. Then the desired slice of the currentUARenderTargetViews array could be copied into the array with correct count, and that could be passed in?

Also would you prefer the commits to be cleaned up via rebasing (I'd squash the original and typo commits together and rebase to the current master) or do you prefer keeping the original history?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Aug 20, 2019

@WhyPenguins Let's keep it like this, it's fine in the meantime. We better fix it SharpDX side later rather than making a big ugly temporary workaround with array of arrays.
No problem for the commits, I can rebase+squash them directly from GitHub merge button.

@xen2 xen2 merged commit a503e90 into xenko3d:master Aug 29, 2019
2 checks passed
2 checks passed
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
phr00t added a commit to phr00t/FocusEngine that referenced this pull request Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.