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

Unify spatial picking & fix "3d in 2d" picking #663

Merged
merged 26 commits into from
Jan 4, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jan 3, 2023

This pr unified previously separate picking detection by introducing a more sophisticated, 3d based picking handling which is also aware of transparency.

Apart from being a necessary code cleanup this also fixes picking of 3d objects in 2d (which were made possible in #625) and allows to pick images in 3d with full zoomed in mini-view just like in 2d:

picking.mov

Even transparent images work in 3d just the same now!

Screen.Recording.2023-01-03.at.16.03.08.mov

Also paves the way for unifying 2d/ortho and 3d cameras in the viewer which will later allow fancy transitions from ortho to 3d (and simplify our code), but doesn't go very far there other than allowing Eye to not have a fov.

Other known behavior changes:

  • image picking was very special before and would always do hover react code in 2d mode. Now it is hidden by opaque objects
  • 2d bboxes are no longer handled like gui overlays, but just like the lines & labels they are

Missing:

  • de-duplicate code for picking reaction (i.e. what to show on hover, what to do on click), this pr gets very very close to it
  • pick 3d labels (still not working, easier to add now, but should come with unifying label handling)
  • a good story for picking data - Staging Belt & use in PointCloudBuilder #594 will require holding cpu accessible data separately

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

@Wumpf Wumpf marked this pull request as draft January 3, 2023 15:54
@Wumpf Wumpf marked this pull request as ready for review January 3, 2023 16:25
@teh-cmc teh-cmc self-requested a review January 4, 2023 09:07
@teh-cmc
Copy link
Member

teh-cmc commented Jan 4, 2023

First impressions after trying this out on different datasets:

  1. It's awesome
  2. The fact that picking only triggers when zoomed closely enough feels odd / overly-restrictive? I'm not sure whether that's the intended behaviour or not (haven't read the code yet...)
    Here's a video of me trying to desperately figure out an angle that would make picking work, until I realized it wasn't an issue with the angle, but rather the zoom-level (ignore the crazy stuttering at the beginning, that's just a record artifact)
23-01-04_10.20.33.patched.mp4

@Wumpf
Copy link
Member Author

Wumpf commented Jan 4, 2023

The fact that picking only triggers when zoomed closely enough feels odd / overly-restrictive?

Bug! I'll investigate. I think also that rectangles might not be double sided 🤔
Might be related to depth offsets, aka #647

Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Aside from the zoom issue from my earlier comment: 👍

crates/re_viewer/src/math.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/view_spatial/scene/picking.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf force-pushed the andreas/unify-spatial-picking branch from 8310949 to d6bc41a Compare January 4, 2023 14:20
@Wumpf Wumpf merged commit 6028712 into main Jan 4, 2023
@Wumpf Wumpf deleted the andreas/unify-spatial-picking branch January 4, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants