-
Notifications
You must be signed in to change notification settings - Fork 313
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
Add depth bias to wireframe sample #430
Conversation
This is great! I actually tried adding that too and when I didn't see a difference I gave up for a while. I'm glad you figure out why it wasn't working for me. Can you fix up the lint issues? |
Done! |
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.
lgtm with one question. up to you.
const gui = new GUI(); | ||
gui.add(settings, 'barycentricCoordinatesBased').onChange(addRemoveGUI); | ||
gui.add(settings, 'lines'); | ||
gui.add(settings, 'models'); | ||
gui.add(settings, 'animate'); | ||
gui.add(settings, 'depthBias', -1, 1, 0.05).onChange(rebuildLitPipeline); |
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.
AFAICT, these are not needed to make the barycentric edge version work. The triangles are the same so the depth test of less-equal perfectly matches. Do these settings appearing in the barycentric case make that more confusing, as in, should these settings be removed in the barycentric solution case?
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 wondered about that, but also observed that they DO have an effect in the barycentric mode (though as you pointed out aren't necessary to get a clean render) so I figured leaving them in was at the very least educational? I'm happy to suppress them in that case if you'd like.
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'm fine either way. Just don't want people to think they need to set them if they don't
As was pointed out by @rconde01 in the matrix chat, the wireframe sample suffers from Z fighting that gives the wireframes a very aliased, sparkly appearance. This PR adds an adjustable
depthBias
anddepthBiasSlopeScale
slider that can be used both to improve the appearance of the wireframe and help visualize for users the effect those values have.Before:
After:
Bias is applied to the lit mesh rather than the wireframe because apparently line and point primitives don't consider depth bias when rendering! Didn't know that previously. I've also added a checkbox to pause the scene's animation, as I found that helpful when specifically looking for the effects of the depth bias.