Argument buffers and Metal bump mapping support#1825
Conversation
|
This PR will be updated to include the manual Metal 2 driver option so it can be sufficiently tested even on new hardware. Then I'll ask for another review. |
This shouldn’t have worked before on Metal 2 class hardware. Need to do a retest on older hardware.
Now tested
Adds support for running Metal 2 argument buffers on Metal 3 Mac
dbfdb3c to
cbd3238
Compare
|
Whoops - it doesn't look like the latest changes were pushed. I've pushed them now. |
| { | ||
| public: | ||
| plMetalArgumentBuffer(plMetalDevice* device, size_t numElements) | ||
| : fDevice(device), fNumElements(numElements), fEncoder(nullptr) |
There was a problem hiding this comment.
| : fDevice(device), fNumElements(numElements), fEncoder(nullptr) | |
| : fDevice(device), fNumElements(numElements), fEncoder() |
| fBuffer[0] = nullptr; | ||
| fBuffer[1] = nullptr; | ||
| fBuffer[2] = nullptr; |
There was a problem hiding this comment.
Shouldn't this be done in the initializer?
| fBuffer[0]->release(); | ||
| fBuffer[1]->release(); | ||
| fBuffer[2]->release(); | ||
| fEncoder->release(); |
There was a problem hiding this comment.
| fEncoder->release(); | |
| if (fEncoder) | |
| fEncoder->release(); |
This is initialized to nullptr.
There was a problem hiding this comment.
Calling functions on null pointer Obj-C types is a language feature. Metal-cpp inherits this behavior because it's just a thing wrapper over Obj-C. The if check is unnecessary.
(Annoying that Metal-cpp causes Obj-Cisms to become a thing in C++. But at least anywhere you see a retain/release it can be assumed that it's ok to call that on a nullptr.)
There was a problem hiding this comment.
That sounds like we'd be depending on an implementation detail, and I can't say I'm a fan of that.
There was a problem hiding this comment.
This is an overtly stated "feature" of the library:
Similar to Objective-C, it is legal to call any method, including
retain()andrelease(), onnullptr"objects". While calling methods onnullptrstill does incur in function call overhead, the effective result is equivalent of a NOP.
Unfortunately can't directly link to the readme in since it's inside a zip file. :(
|
There are still a few things left to address, I'll do that this afternoon. |
e272ab9 to
0c3b314
Compare
|
I've pushed a complete set of changes for the feedback. I did not add nullptr checks for the Obj-C types. |
0c3b314 to
3228c52
Compare
|
This branch remains outstanding and is blocking future work and performance optimizations. I'm going to suggest we still not implement the null checks on retain/release. That's an established pattern in the Metal code base and is allowed by Obj-C (even if that is being hidden by the C++ frontend to Metal.) I understand the distaste for a different set of rules. I would consider this similar to how calling delete on a null pointer is considered safe - even if the syntax for retain/release uses the function call syntax with the C++ bridge. |
|
I'm working through clearing my pull requests. This one is important because it finalizes bump mapping in the Metal renderer. It's also holding up another PR with performance improvements - getting concerned I'll lose my mental context on the changes I made in that PR. Hoping we can get this one through soon. |
This PR implements argument buffers in the Metal renderer - and implements them first through bump mapping. Eventually I would like argument buffers to replace the entire layer system in the Metal shaders. I'm actively working on that - will be a future PR. If anyone is interested in rendering - please take your time on this one. Any improvements/feedback here will be very helpful for future work.
This came about because @dpogue asked about multiple bump maps in the original PR. There aren't an unlimited number of blind points in the shaders and the bind points have to be predefined - so supporting multiple bump maps would have been complicated.
Argument buffers replace bind points in shaders. I bind resources to the argument buffers instead, and then pass the buffers to the shader. The advantage is that now the bind points for textures, samplers, and other resources don't need to be predefined in the shader. And because they don't need to be predefined I can pass an array of argument buffers of any length and the shader can iterate over them. That means I can effectively bind an unlimited number of resources within shaders. This solves the problem of how we deliver an indeterminate amount of bump maps to the shader.
There is also a performance pickup. I can leave the argument buffers on the GPU, and instead of having to bind textures and samplers individually, I can just rebind the buffer I have cached. I have to be careful doing this around features like push over layers and piggyback layers. I'm doing some state tracking and comparisons right now to guard against changes and update the argument buffers when necessary. This will get messier when layers themselves move over.
While this change lets us effectively support an unlimited number of bump maps - the structure passed between the vertex and fragment stages is still compile time defined. This has constrained the number of bump maps that can be supported by the shader to 8. (Which seems totally reasonable to me.)
Argument buffers work differently in Metal 3 and Metal 2. This code supports both approaches.
Not all contributor Macs will support Metal 3, Apple lists the Metal 3 requirements here:
https://support.apple.com/en-us/102894
Tier 1 and Tier 2 buffers are also a little different in the shader. Tier 1 buffers still need ids for each binding so the encoder knows what is being bound where. Tier 2 argument buffers give me direct access - so there is no need to binding ids.
All previous conversations about the quality of the bump map still apply here. The code is present for full quality bump maps. But we're emulating the D3D Plasma behavior of ignoring the "height" component of the normal map.