-
Notifications
You must be signed in to change notification settings - Fork 512
Add new setDepthClampEnable API for clamping depth values #2212
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
base: main
Are you sure you want to change the base?
Conversation
{ "getPointSize", w_getPointSize }, | ||
{ "setDepthMode", w_setDepthMode }, | ||
{ "getDepthMode", w_getDepthMode }, | ||
{ "setDepthClampEnable", w_setDepthClampEnable }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a matching hasDepthClamp
API exposed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it under getSupported that would suffice I guess?
// But the depth clamp enable is not part of the core dynamic state, so we need to use noDynamicState only for depth clamp enable. | ||
// Rest of the dynamic state is still set via extended dynamic state. | ||
configuration.noDynamicState.depthClampEnable = states.back().depthClampEnable; | ||
pipeline = s->getCachedGraphicsPipeline(this, configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling getCachedGraphicsPipeline(this, configuration);
rather than getCachedGraphicsPipeline(this, configuration.core);
means all draws are going to be slower than before, in situations where extendedDynamicState
is supported but extendedDynamicState3
is not.
(The hash map query is much slower with the larger struct than with the smaller one, it's a hot spot in user code so it's important to keep it as fast as possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah right, but since we are tracking a new variable, it's tracking more variants, so how about another structure noExtendedDynamicState passed alongside (smaller than noDynamicState), this way the code path is still hotter than original but no as hot as getCachedGraphicsPipeline(this, configuration);
? Any other ideas to keep it same and still track variants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could do something like getCachedGraphicsPipelineCoreWithDepthClamp
or getCachedGraphicsPipelineExtendedDynamicState
to maintain another set of variants with Core + dynamic_state3 variants?
You can pass in |
This PR is a implementation for #2185, I've added a new lua function called setDepthClampEnable. For OpenGL this is straighforward just a glEnable call and for Metal we can set this on the MTLRenderCommandEncoder using setDepthClipMode.
For vulkan we can either use the VK_EXT_dynamic_state3 extension or get a cached pipeline by tracking a new member on the GraphicsPipelineConfigurationNoDynamicState struct.
Depth.Clamp.Test.-.Vulkan.2025-08-15.22-02-48.mp4