Add createTextureWithData function.#1307
Conversation
bogner
left a comment
bogner
left a comment
There was a problem hiding this comment.
I don't love add-only PRs that don't add tests. Since none of the new code is used, it's by definition untested. In this case I guess it's sort of okay as a staging strategy since it will presumably be followed up by a mostly remove PR when we adopt the generic API (and the existing tests will verify correctness), but in general it's nice to see the code that's being deleted along with the code that's being added to compare the two.
I agree with your point. My problem was that it basically turns into one mega PR that becomes difficult to review. To address part of the problem, all the introduced code has been tested and does pass all the tests in the refactor branch. |
| const D3D12_PLACED_SUBRESOURCE_FOOTPRINT Footprint{ | ||
| 0, | ||
| CD3DX12_SUBRESOURCE_FOOTPRINT( | ||
| getDXGIFormat(DXDst.Desc.Fmt), DXDst.Desc.Width, DXDst.Desc.Height, | ||
| 1, getAlignedTexturePitch(DXDst.Desc.Width, ElementSize))}; | ||
| const CD3DX12_TEXTURE_COPY_LOCATION DstLoc(DXDst.Resource.Get(), 0); | ||
| const CD3DX12_TEXTURE_COPY_LOCATION SrcLoc(DXSrc.Buffer.Get(), Footprint); |
There was a problem hiding this comment.
If the buffer was not created with the necessary padding, the Footprint will be bigger than the actual buffer size.
Let's say I want to define a 3x2 render target. I can't be expected to add the padding in the yaml file (as it's only a DX concern). Do we have something in the backend code that is adding that padding?
There was a problem hiding this comment.
I guess the calling code is protecting from the above case with
if (SizeInBytes < PackedRowStrideInBytes * Desc.Height)
return llvm::createStringError(
"Data upload is not enough for texture size.");
but I think as soon as you try to create a tiny 2d texture this might trigger 🤔
There was a problem hiding this comment.
This PR also introduces the getTextureUploadRowStrideInBytes function to acquire the required row stride to add the necessary padding. The caller of copyBufferToTexture is expected to set up their upload buffer correctly.
In the case of the textures we create from the yaml file, we will be using createTextureWithData, which memcpys the data with the required padding into the upload buffer.
…ly on the upload buffer before callign copyBufferToTexture.
|
CI is failing for reasons irrelevant to this PR. On top of that, it was passing CI on the previous commit, while only a comment was added in the latest commit. So I feel safe merging this code. |
3 participants
Another small PR as a step towards a fully generic API for creating resources and uploading data.
The function is not actually being called in this PR, since doing that would make the PR very large, create a lot of extra work, and make it difficult to review as well.
This all builds on top of building blocks that have been layed out while doing the same logic for buffers, so none of the logic here is really new other than just variants of already existing code.