Fix UpdateBuffer not checking zero size #524
Conversation
| @@ -718,6 +718,10 @@ public void Unmap(MappableResource resource, uint subresource) | |||
| throw new VeldridException( | |||
| $"The data size given to UpdateBuffer is too large. The given buffer can only hold {buffer.SizeInBytes} total bytes. The requested update would require {bufferOffsetInBytes + sizeInBytes} bytes."); | |||
| } | |||
| if (sizeInBytes == 0) | |||
| { | |||
| return; | |||
There was a problem hiding this comment.
If one graphics device already fails (vulkan) when performing an "update buffer" call with zero bytes, then this should probably throw a VeldridException instead of just silently returning. Especially since this is at the surface of the API.
There was a problem hiding this comment.
I would disagree here. This check is already implemented for the other rather similar APIs like CommandList.UpdateBuffer, CopyBuffer etc. and was discussed in #117
For the user ignoring the call for sizeInBytes==0 is not an error, all data supposed to be updated was updated, just that all data is no data in this case.
There was a problem hiding this comment.
I agree with it not throwing an exception. I don't think one device failing is a good reason for all devices to fail, as Veldrid is itself opinionated on how operations are performed so that they conform to the requirements set by the device (the D3D buffer update process as an example).
Also, the D3D11 implementation duplicates this check.
Calling
GraphicsDevice.UpdateBufferwithsizeInBytes == 0on Vulkan with a recent validation layer (e.g. SDK 1.3.275.0) causes the validation errorVUID-VkBufferCopy-size-01988.This validation error is also reproduced by the
Veldrid.Tests.VulkanBufferTests.UpdateBuffer_ZeroSizeunit test.The
CommandList.UpdateBuffermethods have this check already in place so I guess this was a small oversight.