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

Integrate depth clouds into Rerun #1421

Merged
merged 26 commits into from
Mar 3, 2023
Merged

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Feb 27, 2023

This PR integrates depth clouds (#1415) into the main Rerun app.

Any tensor..:

  • ..shaped like an image
  • ..with meaning=depth
  • ..sitting below a pinhole transform

is eligible for backprojection.

The advantages from a user's standpoint are two-fold:

  • No need to manually do backprojection, log point clouds, sample an albedo texture, etc
  • Much, much more performant

The drawbacks:

  • No picking, no hovering

From a developer standpoint this is also an interesting exercise in dogfooding.

Support for (gpu) color maps and albedo textures will follow in subsequent PRs.


Manual point clouds vs. depth clouds:

23-03-02_15.00.41.patched.mp4

Fun bonus: infinite history depth clouds (something you cannot do otherwise!):

23-03-02_15.03.23.patched.mp4

@teh-cmc teh-cmc changed the base branch from main to cmc/renderer/depth_clouds February 27, 2023 17:17
@teh-cmc teh-cmc added ui concerns graphical user interface dogfooding 📉 performance Optimization, memory use, etc labels Feb 27, 2023
@teh-cmc teh-cmc force-pushed the cmc/depth_clouds_integration branch 3 times, most recently from 340b3fa to f4c6920 Compare March 2, 2023 08:10
Base automatically changed from cmc/renderer/depth_clouds to main March 2, 2023 09:26
@teh-cmc teh-cmc force-pushed the cmc/depth_clouds_integration branch from d87d6ef to 7cf4fd0 Compare March 2, 2023 13:58
@teh-cmc teh-cmc marked this pull request as ready for review March 2, 2023 14:24
teh-cmc added a commit that referenced this pull request Mar 2, 2023
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.

Looks good except the EditableAutoValue business - EditableAutoValue is clearly immature as such but something weird is going on there in your ui code or I don't get it ;)

The transform stuff is scary, but I think it all makes sense to me the way you written it down :)

still pondering the name "depth cloud", seems quite unconventional. Kinda makes sense tho and I don't have any other idea. @emilk maybe?

@@ -261,7 +253,8 @@ impl framework::Example for RenderDepthClouds {
Vec3::new(focal_length, 0.0, offset.x),
Vec3::new(0.0, focal_length, offset.y),
Vec3::new(0.0, 0.0, 1.0),
);
)
.transpose();
Copy link
Member

Choose a reason for hiding this comment

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

given that the way you wrote the matrix up seems to be the "normal way" and that you moved out the transpose from the shader, maybe the transpose should be on renderer/depth_cloud.rs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm writing the matrix as if the constructor accepted rows rather than columns (cause I find it easier to read), so i should be the one transposing back no?

Comment on lines +290 to +292
// TODO(cmc): re_renderer needs native interop with Arrow, we shouldn't need to
// allocate this massive buffer and do a memcpy when we could just carry arround the
// original `arrow::Buffer`.
Copy link
Member

Choose a reason for hiding this comment

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

but didn't you do this already on another PR that landed?

Copy link
Member Author

Choose a reason for hiding this comment

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

not quite landed yet, it requires this PR!

crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
ui.end_row();

ui.label("Backproject radius scale");
let speed = (radius_scale * 0.05).at_least(0.01);
Copy link
Member

Choose a reason for hiding this comment

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

When I tried it I found it a bit too sensitive still

Copy link
Member

Choose a reason for hiding this comment

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

[out of scope] Also I noticed that units in points might be nice as well after all. That requires much deeper changes tho and I don't know how to make the ui story for that ok - probably we need to make the setting thing we have on for point & line sizes on the space view "a thing" and re-use it here

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels pretty good with the new sensitivity I've just set

@teh-cmc teh-cmc requested a review from Wumpf March 3, 2023 10:19
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.

very nice, that's exactly how I thought EditableAutoValue should play out :)

@teh-cmc teh-cmc merged commit cb48497 into main Mar 3, 2023
@teh-cmc teh-cmc deleted the cmc/depth_clouds_integration branch March 3, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dogfooding 📉 performance Optimization, memory use, etc ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants