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

Model::LoadStaticBuffers to use static VB/IB instead of dynamic #38

Merged
merged 6 commits into from
Jun 22, 2018

Conversation

walbourn
Copy link
Member

@walbourn walbourn commented Jun 21, 2018

In the original implementation, all Model vertex buffers and index buffers are allocated by GraphicsMemory. This means that they are in a upload heap which is accessible to both the CPU and GPU, equivalent to the Direct3D 11 version of USAGE_DYNAMIC. This is simple and easy to use, but has performance impacts--which on Xbox One can be quite noticeable due to the caching behavior.

This change adds an optional call to LoadStaticBuffers which uses a ResourceUploadBatch to create static VB/IBs for rendering copying the data from the loaded GraphicsMemory resources. This is equivalent to USAGE_DEFAULT in Direct3D 11.

The ModelMeshPart now contains four new members:

  • staticVertexBuffer: If set, then this vertex buffer is used for rendering as the VB in the view. Otherwise it uses vertexBuffer.
  • staticIndexBuffer: If set, then this index buffer is used for rendering as the IB in the view. Otherwise it uses indexBuffer.
  • vertexBufferSize: The size of the VB for rendering set in the view.
  • indexBufferSize: The size of the IB for rendering set in the view.

There is a small breaking change here in that all custom Model loaders must populate the new vertexBufferSize and indexBufferSize members of ModelMeshPart.

Custom Model renderers should be updated for all four new member variables.

To support this I added a new Upload method overload for ResourceUploadBatch. The existing Upload method copies the data into a staging upload heap resource for use in the copy to the GPU, but here the data is already in an upload heap managed by GraphicsMemory which we can use directly.

Also did a little SAL annotation cleanup in a few places, and added comparison operators for SharedGraphicsResource

@@ -53,10 +53,14 @@ namespace DirectX
uint32_t vertexOffset;
uint32_t vertexStride;
uint32_t vertexCount;
uint32_t indexBufferSize;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity, I require these values both be populated if you are using either dynamic or static VB/IB. It's a breaking change for custom loaders and renderers, but an easy one to resolve.

Inc/Model.h Outdated
_In_opt_z_ const wchar_t* texturesPath = nullptr,
D3D12_DESCRIPTOR_HEAP_FLAGS flags = D3D12_DESCRIPTOR_HEAP_FLAG_SHADER_VISIBLE) const;

// Load VB/IB resources for static geometry
void __cdecl LoadBuffers(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new method on Model that you can call to optimize your content.

_In_ uint32_t numSubresources);
uint32_t numSubresources);

void __cdecl Upload(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To support this, I modified ResourceUploadBatch to have an overload that will take data from a SharedGraphicsResource.

@@ -49,15 +52,33 @@ ModelMeshPart::~ModelMeshPart()
_Use_decl_annotations_
void ModelMeshPart::Draw(_In_ ID3D12GraphicsCommandList* commandList) const
{
if (!indexBufferSize || !vertexBufferSize)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation will ensure that developers moving to the new version won't get silent failures...

Src/Model.cpp Outdated
@@ -208,6 +229,135 @@ std::unique_ptr<EffectTextureFactory> Model::LoadTextures(
}


// Load VB/IB resources for static geometry
_Use_decl_annotations_
void Model::LoadBuffers(
Copy link
Member Author

@walbourn walbourn Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the algorithm which converts a Model using dynamic VB/IB to static VB/IB. If you pass keepMemory = true (it defaults to false) I will keep the original vertexBuffer and indexBuffer members set as well as setting the staticVertexBuffer and staticIndexBuffer values. The rendering will only use the static versions if both are set.

A usage scenario for keepMemory is that if you need the VB/IB data for other purposes (DirectX Raytracing or CPU collision/picking) you can keep the data in the upload heap for subsequent operations as well as have an optimized static buffer.

@@ -549,12 +549,14 @@ std::unique_ptr<Model> DirectX::Model::CreateFromSDKMESH(const uint8_t* meshData
// Vertex data
auto verts = bufferData + (vh.DataOffset - bufferDataOffset);
auto vbytes = static_cast<size_t>(vh.SizeBytes);
part->vertexBufferSize = static_cast<uint32_t>(vh.SizeBytes);
Copy link
Member Author

@walbourn walbourn Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I update the SDKMESH and VBO loaders for the minor breaking change

uploadBatch->CommandList = mList;
uploadBatch->Fence = fence;
uploadBatch->GpuCompleteEvent = gpuCompletedEvent;
uploadBatch->TrackedObjects.insert(std::end(uploadBatch->TrackedObjects), std::begin(mTrackedObjects), std::end(mTrackedObjects));
std::swap(mTrackedObjects, uploadBatch->TrackedObjects);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the logic here, it makes more sense to swap than it does to copy then reset...

@@ -92,6 +92,9 @@ namespace DirectX

explicit operator bool () const { return mSharedResource != nullptr; }

bool operator == (const SharedGraphicsResource& other) const { return mSharedResource.get() == other.mSharedResource.get(); }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that comparison operators of std::shared_ptr have the same semantics...

@walbourn walbourn changed the title Model::LoadBuffers to use static VB/IB instead of dynamic Model::LoadStaticBuffers to use static VB/IB instead of dynamic Jun 21, 2018
@walbourn walbourn merged commit 9d52704 into master Jun 22, 2018
@walbourn walbourn deleted the modelstaticbuffers branch June 22, 2018 22:17
@walbourn walbourn added the xbox Related to Xbox One and/or Xbox Series X|S label Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement optimization xbox Related to Xbox One and/or Xbox Series X|S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant