-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Merged by Bors] - add depth_bias to SpecializedMaterial #4101
Conversation
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.
Nice!
@robtfm - I think additionally, implementing this for |
added for Material and StandardMaterial. with the inlining it should be free for material, so no reason not to. |
This seems reasonable. Can we do a quick comparison to what this looks like in other engines? Godot's closest corollary is probably the "render priority" integer on materials. https://docs.godotengine.org/en/stable/tutorials/3d/spatial_material.html#render-priority. I think depth_bias (with a float representation) more accurately captures whats happening, so unless everyone is doing it the same way, I think we're good. |
Unity also does "priority integers": https://docs.unity3d.com/Packages/com.unity.render-pipelines.high-definition@6.7/manual/Renderer-And-Material-Priority.html However at least in godot, these numbers are "first" in the sort order, rather than being "tie breakers" like depth_bias is. I think there are use cases for both approaches. Resolving z-fighting is generally something that happens between "mesh entities" not between "materials". Maybe this should live on mesh entities to enable resolving z-fighting between entities with the same material? Additionally, this doesn't "resolve" z-fighting, it just "moves" it right? things might still z-fight if they walk "away" from their bias in the direction of the camera. |
thinking about it integers might well be better. for opaque (my motivating case) i think either way resolves it and they're basically the same. it only affects the render sort-order, it doesn't affect the depth written to buffers / "physical" location, so when you have two (or more) equal-depth meshes it will determine which is rendered first, and so which one gets to fill the depth buffer and prevent the others from rendering. for transparent, integers probably make more sense since all the materials will be rendered regardless, and with integers you can specify the order explicitly without having to worry about using floats "big enough" to overcome depth differences. i don't see any real value in forcing transparent material sort order "up to a specified depth difference" which is what floats allow.
i envisaged this being resolved by having copies of the material with different depth_bias values. i also think it's useful to have at the material level if you have multiple mats on the same mesh. |
ok now i agree with you - i think floats are good, but maybe integer priority (as the first component of the sort order) is useful as well. i don't think there's anything one method can do that the other can't, but one or the other is cleaner in some cases. for opaque and alpha-masked materials where we write to the depth buffer, the only use-case for depth_bias is to disambiguate equal depth fragments. bias or priority will both do this equally well by determining which material gets rendered first. for transparent materials, if we get order-independant transparency then it's all irrelevant. but otherwise there are two uses that i can think of
priority integers
bias
another possible consideration is instancing when a material is shared by multiple renderables, like unity's HDRP.
so basically i'd like to merge this as is, and possibly add priority integers later. i don't think depth_bias adds much maintenance burden or makes any future plans unworkable, and it's cleaner than priority integers in some cases. |
@cart I'll leave the final call to you. Personally, I'm in favor of merging this as is for now. |
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 I think this is probably worth merging as-is and we can add priorities later. They seem complimentary. Priority will be for ensuring certain material types always render before other material types. Depth bias is for ensuring specific instances of materials render before other specific instances of materials at the same depth.
bors r+ |
# Objective allow meshes with equal z-depth to be rendered in a chosen order / avoid z-fighting ## Solution add a depth_bias to SpecializedMaterial that is added to the mesh depth used for render-ordering.
# Objective allow meshes with equal z-depth to be rendered in a chosen order / avoid z-fighting ## Solution add a depth_bias to SpecializedMaterial that is added to the mesh depth used for render-ordering.
# Objective allow meshes with equal z-depth to be rendered in a chosen order / avoid z-fighting ## Solution add a depth_bias to SpecializedMaterial that is added to the mesh depth used for render-ordering.
Objective
allow meshes with equal z-depth to be rendered in a chosen order / avoid z-fighting
Solution
add a depth_bias to SpecializedMaterial that is added to the mesh depth used for render-ordering.