Skip to content

Argument buffers and Metal bump mapping support#1825

Open
colincornaby wants to merge 10 commits into
H-uru:masterfrom
colincornaby:metal-bump-mapping
Open

Argument buffers and Metal bump mapping support#1825
colincornaby wants to merge 10 commits into
H-uru:masterfrom
colincornaby:metal-bump-mapping

Conversation

@colincornaby
Copy link
Copy Markdown
Contributor

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.

  • Tier 2 argument buffers (Metal 3) give direct access to the underlying buffer. I can directly store texture/sampler/etc addresses into those buffers.
  • Tier 1 argument buffers (Metal 2) have to support hardware with an inconsistent memory layout. An encoder object must be created that manages those differences - which adds some overhead.

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.

Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/ShaderSrc/FixedPipelineShaders.metal Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalMaterialShaderRef.cpp Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.cpp Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.cpp Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalMaterialShaderRef.cpp Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalMaterialShaderRef.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/ShaderSrc/FixedPipelineShaders.metal Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/ShaderSrc/FixedPipelineShaders.metal Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalMaterialShaderRef.cpp Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.cpp Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalMaterialShaderRef.cpp Outdated
@colincornaby
Copy link
Copy Markdown
Contributor Author

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.

@colincornaby
Copy link
Copy Markdown
Contributor Author

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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
: fDevice(device), fNumElements(numElements), fEncoder(nullptr)
: fDevice(device), fNumElements(numElements), fEncoder()

Comment on lines +79 to +81
fBuffer[0] = nullptr;
fBuffer[1] = nullptr;
fBuffer[2] = nullptr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be done in the initializer?

Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
fBuffer[0]->release();
fBuffer[1]->release();
fBuffer[2]->release();
fEncoder->release();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
fEncoder->release();
if (fEncoder)
fEncoder->release();

This is initialized to nullptr.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds like we'd be depending on an implementation detail, and I can't say I'm a fan of that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is an overtly stated "feature" of the library:

Similar to Objective-C, it is legal to call any method, including retain() and release(), on nullptr "objects". While calling methods on nullptr still 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. :(

Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
Comment thread Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalArgumentBuffer.h Outdated
@colincornaby
Copy link
Copy Markdown
Contributor Author

There are still a few things left to address, I'll do that this afternoon.

@colincornaby
Copy link
Copy Markdown
Contributor Author

I've pushed a complete set of changes for the feedback. I did not add nullptr checks for the Obj-C types.

@colincornaby
Copy link
Copy Markdown
Contributor Author

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.

@colincornaby
Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants