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

Improve the depth backprojection feature #1690

Merged
merged 29 commits into from
Mar 24, 2023
Merged

Conversation

emilk
Copy link
Member

@emilk emilk commented Mar 23, 2023

Closes #1587

A bunch of fixes:

  • I fixed a bug that caused our points to always have a radius that was 0.5 pixels too much, making points too large.
  • I fixed a bug in the image hover code, causing the wrong RGBA values to be printed 😬
  • I fixed the gamma of the depth cloud color map
  • I fixed a bug in f32 depth maps
  • I made the color map range of the depth cloud the same as that for the 2D depth maps
  • The UI option is now for the same "meters" parameter as in the SDK. You can double-click it to reset it to what you logged.
  • The point radius scale is defines so that 1.0 means that the radius is the same as a pixel projected at that distance. This means a value of 0.5 creates balls that just touch horizontally and diagonally. The default is 2.0 to avoid Moiré patterns.

image

The colors of the 2D depth visualization and the 3D backprojected depth now line up perfectly both for u16 and f32 depth maps:

image

Checklist

@emilk emilk added ui concerns graphical user interface 🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself labels Mar 23, 2023
@emilk emilk force-pushed the emilk/emilk-backproject-options branch from 1cf0f56 to 8594073 Compare March 23, 2023 16:12
@Wumpf Wumpf self-requested a review March 23, 2023 16:20
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

lgtm overall!

crates/re_data_store/src/entity_properties.rs Outdated Show resolved Hide resolved
crates/re_log_types/src/data.rs Outdated Show resolved Hide resolved
crates/re_renderer/shader/utils/sphere_quad.wgsl Outdated Show resolved Hide resolved
crates/re_renderer/shader/utils/sphere_quad.wgsl Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
With a scale of one, diagonally adjacent pixels at the same depth are sized so that they are just touching, leaving no gaps.\
\nDouble-click to reset.",
);
if response.double_clicked() {
Copy link
Member

Choose a reason for hiding this comment

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

huh. yeah! I like that. Goes on the growing pile of "how to deal with defaults in a uniform manner everywhere" :)

@emilk emilk requested a review from Wumpf March 24, 2023 10:41
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

real good fixes but separate PRs would have been appreciated as this is super hard to discover for our release notes this way

The normalization logic gets a bit confusing. But I reckon the principle now is that from the outside it is supposed to look like the Renderer always works with UNorm?

All tested again on web?


// TODO(cmc): albedo textures
let color = Vec4(colormap_srgb(depth_cloud_info.colormap, norm_linear_depth), 1.0);
let color = Vec4(colormap_linear(depth_cloud_info.colormap, world_space_depth / depth_cloud_info.max_depth), 1.0);
Copy link
Member

Choose a reason for hiding this comment

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

max_depth being in world units catches me by surprise here

Comment on lines +30 to +32
// Add half a pixel of margin for the feathering we do for antialiasing the spheres.
// It's fairly subtle but if we don't do this our spheres look slightly squarish
modified_radius += frame.pixel_world_size_from_camera_distance * camera_distance;
modified_radius += 0.5 * frame.pixel_world_size_from_camera_distance * camera_distance;
Copy link
Member

Choose a reason for hiding this comment

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

thanks for following up on this. Given the previous change this should be safe, but have you checked if the point cloud in the multiview demo looks the exact same before after? (you can stop the movement with space and then take zoomed in screenshots if needed)

crates/re_renderer/src/renderer/depth_cloud.rs Outdated Show resolved Hide resolved
Co-authored-by: Andreas Reich <andreas@rerun.io>
@emilk emilk changed the title Improve the depth backproject options Improve the depth backprojection feature Mar 24, 2023
@emilk emilk merged commit 44b748e into main Mar 24, 2023
@emilk emilk deleted the emilk/emilk-backproject-options branch March 24, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backprojection parameters aren't descriptive
2 participants