Repository navigation
fix: give each dx12 shadow caster its own transform constants - #186
Merged
Merged
Conversation
Shadow transforms now come from a per-frame linear upload ring instead of one shared 256 byte buffer, so each caster's draw reads its own LightVP * World.
4 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The DX12 shadow pass now gets a fresh 256 byte slice of upload memory for every caster's
LightVP * Worldinstead of writing them all into the singlem_shadowTransformCB. The slices come from a newDirectX12UploadRing, a small per-frame linear allocator over persistently mapped upload heap pages.m_shadowTransformCBandm_shadowTransformMappedare gone.Why
BindConstantBufferin shadow mode memcpy'd each caster's matrix into the same mapped buffer and recorded the same GPU VA for every draw. Nothing runs untilExecuteCommandListsinEndRender, so by the time the GPU reads it the buffer only holds the last caster's matrix, and every shadow draw rasterizes with that one. This is the Vulkan push constant path ported over as is, except push constants are baked into the command buffer per draw and a root CBV is just a pointer.The ring keeps one set of pages per frame in flight.
BeginRenderrewinds the currentframeIndex's pages right after the fence wait, in the same spot the command allocator for that slot gets reset, so it relies on exactly the same guarantee the allocator already does (the GPU is done with the last frame that used this slot). Allocations are aligned toD3D12_CONSTANT_BUFFER_DATA_PLACEMENT_ALIGNMENT. Pages are 64 KB (256 shadow transforms each) and a frame that runs out just grabs another page, which is kept and reused from then on, so there is no hard cap on caster count. Everything is freed inCleanupafterWaitForGPU.I kept it separate from the renderer on purpose so other transient per-draw constants can go through
m_uploadRing.Allocatelater. Only the shadow transform uses it in this PR.Not covered here: #165 (per-slot fence values signalled into one shared fence). I checked whether the ring needs it fixed first and it does not.
BeginRenderwaits on the max of both slots, and with the current counters that still always covers the previous frame that used the same slot (it is the allocator reset guarantee again), it just fails to also cover the other slot on every second frame. The ring only touches the current slot's pages, so it is safe either way. The races #165 describes on the other shared upload buffers are still there.Fixes #166
How it was verified
I do not have a Windows machine, so this was only checked by reading and by the Windows CI build. Nothing was run on D3D12. Worth a look on Windows with the debug layer on: a scene with several casters should now show all of them in the shadow map instead of one shape repeated.
Checklist
///comments (new header is private, documented anyway)src/orinclude/private/in public headers