Skip to content
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 ability to display depth buffer in editor 3d preview. #67735

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Joxno
Copy link

@Joxno Joxno commented Oct 22, 2022

This pull request implements the ability to quickly display the depth buffer in editor 3d preview and fixes a bug in the copy.glsl shader related to how linear depth is calculated.

image

image

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Good idea! I think this will be nice addition and it makes sense seeing as how we already have a display mode for the normal buffer.

One thing I am unsure about is whether we actually need to invert the depth. I left some comments on implementation assuming we keep the inverted depth code, but to me it seems like it may make more sense for the debug mode to show users something closer to the actual value instead of inverting the range. Did you do it this way because non-inverted depth was too difficult to read?

Comment on lines 247 to 249
depth = depth * 2.0 - 1.0;
depth = 2.0 * params.camera_z_near * params.camera_z_far / (params.camera_z_far + params.camera_z_near - depth * (params.camera_z_far - params.camera_z_near));

depth = params.camera_z_near * params.camera_z_far /
(params.camera_z_far + depth * (params.camera_z_near - params.camera_z_far));
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain in more detail what is wrong with this way of linearizing depth? This is the same method used throughout the engine, so if it changes here it should change there as well

Copy link
Author

Choose a reason for hiding this comment

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

I think I was a bit quick in editing that shader, I've restored line 248 in a commit I'll be pushing.
In terms of line 247, if I have understood the maths correctly that sort of conversion should not be needed in Vulkan compared to OpenGL, is it correct that it is no longer needed?

Copy link
Member

Choose a reason for hiding this comment

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

No, it is still needed. We aren't dealing with NDC values here, we are dealing with the contents of the depth buffer which are still values in the 0-1 range regardless of using Vulkan or OpenGL.

If you look at other parts of the codebase, they all use the same function to convert depth buffer depth into linear depth. I would stick with using the same code here as elsewhere so that everything matches.

servers/rendering/renderer_rd/shaders/effects/copy.glsl Outdated Show resolved Hide resolved
servers/rendering/renderer_rd/effects/copy_effects.cpp Outdated Show resolved Hide resolved
@RedMser
Copy link
Contributor

RedMser commented Oct 22, 2022

@Joxno You need to run godot --doctool to generate the XML documentation for the newly added enum entries.

@Joxno
Copy link
Author

Joxno commented Oct 22, 2022

Good idea! I think this will be nice addition and it makes sense seeing as how we already have a display mode for the normal buffer.

One thing I am unsure about is whether we actually need to invert the depth. I left some comments on implementation assuming we keep the inverted depth code, but to me it seems like it may make more sense for the debug mode to show users something closer to the actual value instead of inverting the range. Did you do it this way because non-inverted depth was too difficult to read?

Thanks!
Yeah, I can understand the argument to display closer to the truth values for debugging purposes although I think without an inversion it makes the visualisation harder to read.

Here's an example if you do not invert the values:
image

Obviously this is a trivial scene with an infinite background but I think in a more complex scene objects that occupy the middle-ground would be washed out. Possibly making it difficult to debug depth issues.

Maybe having 2 alternatives to choose from, one that displays the values as is, and a second one that is inverted could be a possibility?
"Depth Buffer"
"Depth Buffer (Inv)"

It could also be that I am biased to dark mode haha.

@Joxno Joxno requested a review from a team as a code owner October 22, 2022 23:15
@@ -436,54 +436,56 @@
</constant>
<constant name="DEBUG_DRAW_NORMAL_BUFFER" value="5" enum="DebugDraw">
</constant>
<constant name="DEBUG_DRAW_VOXEL_GI_ALBEDO" value="6" enum="DebugDraw">
<constant name="DEBUG_DRAW_DEPTH_BUFFER" value="6" enum="DebugDraw">
Copy link
Member

Choose a reason for hiding this comment

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

This should be filled in

Comment on lines 158 to 161
// Depth.
bool inverse_depth;
uint32_t pad3[3];
Copy link
Member

Choose a reason for hiding this comment

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

This should replace one of the pads above (I would replace pad) There is no reason to introduce 3 new pads when there is already so much padding

Comment on lines 247 to 249
depth = depth * 2.0 - 1.0;
depth = 2.0 * params.camera_z_near * params.camera_z_far / (params.camera_z_far + params.camera_z_near - depth * (params.camera_z_far - params.camera_z_near));

depth = params.camera_z_near * params.camera_z_far /
(params.camera_z_far + depth * (params.camera_z_near - params.camera_z_far));
Copy link
Member

Choose a reason for hiding this comment

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

No, it is still needed. We aren't dealing with NDC values here, we are dealing with the contents of the depth buffer which are still values in the 0-1 range regardless of using Vulkan or OpenGL.

If you look at other parts of the codebase, they all use the same function to convert depth buffer depth into linear depth. I would stick with using the same code here as elsewhere so that everything matches.

@clayjohn
Copy link
Member

Discussed in the PR meeting today. Consensus was that this looks mostly good, but both non-inverse and inverse result in far away objects being difficult to see. Lawnjelly helpfully suggested (and the other contributors agreed) that we should use the non-inverted linear depth, but we should set the far plane to a fixed color so that far objects contrast well against it.

I am thinking a dark blue colour from this page would be good https://davidmathlogic.com/colorblind/#%23D81B60-%231E88E5-%23FFC107-%23004D40. There are a few colours suggested here that have good visibility for many different types of colourblindness.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@clayjohn clayjohn modified the milestones: 4.1, 4.2 Jun 9, 2023
@Calinou
Copy link
Member

Calinou commented Aug 1, 2023

@Joxno Could you look into applying clayjohn's suggestions above? The PR should be good to merge afterwards.

If you don't have time, let us know and we'll mark this as salvageable for another contributor to pick up.

@clayjohn clayjohn modified the milestones: 4.2, 4.x Oct 25, 2023
Joxno added 3 commits February 14, 2024 10:28
Changed mode to push constant instead after feedback.
Changed method of separate copy function to boolean flag instead after feedback.
@Calinou
Copy link
Member

Calinou commented Feb 14, 2024

Tested locally (rebased on top of master 907db8e), there seems to be a mismatch between the UI in the Perspective menu and the actual debug draw mode you get:

simplescreenrecorder-2024-02-14_22.21.50.mp4

Testing project: test_shadowmask.zip

Notice how selecting Disable Mesh LOD shows directional shadow splits instead, for instance.

@Joxno
Copy link
Author

Joxno commented Feb 14, 2024

Ah, I made a complete blunder when I reordered the debug_draw_modes array but forgot to take into account the order in display_options as well. Should be resolved in the latest commit.

image

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected in Forward+. I can't get this debug draw mode to show up when using the Mobile rendering method though, even though it should be able to work according to what I see in the diff.
It would be nice to get it working in Compatibility too, but this can be left for a future PR.

I noticed there's quite a lot of banding in the debug view:

image

Even with camera near set to 1.0 and far set to 25.0 in the editor view settings, I still get a fair amount of banding:

image

It appears the stepping is actually correct (there are no shades of gray "missed" between the bands), but since the depth is displayed in a non-linear manner, the banding in near-black shades gets pretty obvious. This may be less noticeable depending on viewing conditions or the display you're using (I'm using a LG C2 42" at 120 nits brightness in SDR).

PS: This PR will likely need changes if #88328 is merged before this one.

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

Successfully merging this pull request may close these issues.

6 participants