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

GPU based picking with points #1721

Merged
merged 35 commits into from
Mar 29, 2023
Merged

GPU based picking with points #1721

merged 35 commits into from
Mar 29, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Mar 27, 2023

Major step towards GPU-based picking layer and retrieving thereof. (towards solving #1615)

  • a working re_renderer example for picking in point clouds
  • a debug overlay renderer
  • a (incomplete) picking draw phase, fully implemented for point clouds
  • the ability to schedule readback transfers for picking rects that are rendered on demand to a small texture
  • refactor draw phase processing out to its own module (we should formalize things things some more later!)
Screen.Recording.2023-03-28.at.20.33.33.mov

Recording of an earlier version of the demo running in a browser, with the debug view enabled (at this point the picking layer was still rendering out color)
https://user-images.githubusercontent.com/1220815/228007386-5c630db4-9ee3-4324-9ee2-e71f9ed3f140.mov

Our GPU based picking needs a separate draw pass for its picking layer. The flip side of this is that the picking layer is tiny! Only a small requested rectangle needs to be rendered.
Afaik this is somewhat unusual - picking layers, when done on the GPU, are often done as an additional render target during the main draw phase. However this has a few advantages for us:

  • Allow MSAA on WebGL: If we render color & picking at the same time we won’t get MSAA on WebGL (details are involved!)
  • Safe a lot of bandwidth, especially with MSAA & high resolutions
    • Care less about how big our instance ids are. We’re at 24bytes right now which means that if we don’t want to do cpu sided mapping, we need to use RGBAu32 render targets.
  • Easy to optimize later: E.g. points could be clever and not sample their instance id buffer when they’re not on the target

Disadvantages are the same as for e.g. an additional shadow-map pass:

  • pay for all vertex shaders again
  • send off draw-calls again

Next steps...

  • allow reading back depth & add utility to interpret it (needed by the viewer!)
  • extend all primitives with picking ids that can be rendered to the picking layer and used to identify points
  • start integrating picking layer into the viewer

Note that unlike previously with outlines we can't do a step-by-step update of rendering primitives as all primitives need to be present in the picking layer for a usable result.

Checklist

@Wumpf Wumpf added the 🔺 re_renderer affects re_renderer itself label Mar 27, 2023
@Wumpf Wumpf changed the title GPU based picking WIP GPU based picking with points Mar 28, 2023
@emilk
Copy link
Member

emilk commented Mar 29, 2023

It's subtle, and difficult to capture on video, but I think the hits might be off by a pixel or two. This is recorded on a retina screen:

pixel-imperfect.mp4

If the pointer enters from the top left, the thing is immediately "picked". If it enters from the bottom right, the cursor can penetrate ~2pixels before picking occurs

@emilk emilk self-requested a review March 29, 2023 07:09
@Wumpf
Copy link
Member Author

Wumpf commented Mar 29, 2023

Hmm could be that some calculation isn't entirely watertight yet! There is plenty of room for error here: The picking demo uses the "middle pixel" of the picking rectangle. That already is probably wrong since the rectangle has an even number of pixels.

Sources of errors that need to be checked:

  • placing the rectangle
  • querying the rectangle
  • calculating the projection adjustment for the rectangle
  • size of points without anti-aliasing / alpha-to-coverage

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

The lack of high-level comments makes the code quite hard to follow

crates/re_renderer/examples/multiview.rs Outdated Show resolved Hide resolved
crates/re_renderer/examples/picking.rs Outdated Show resolved Hide resolved

// TODO(andreas): Move this into a utility function?
let picking_data_without_padding =
picking_rect_info.row_info.remove_padding(data);
Copy link
Member

Choose a reason for hiding this comment

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

I feel already that we should have a helper layer between the user and gpu_readback_belt. If every users need to store the original rectangles so that they can remove the padding, then there should be a helper layer that does all that for us

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's what I meant with the comment :)

Copy link
Member Author

@Wumpf Wumpf Mar 29, 2023

Choose a reason for hiding this comment

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

although, not having the user store the originals is kinda hard I think. I didn't want to do dynamic dispatching on the level of the readbackbelt, i.e. not categorizing all the possible kinds of readbacks on the re_renderer side

there's some other work I have planned to deal with the problem of conjoined buffers. I'll have a look into higher level layers then.

crates/re_renderer/examples/picking.rs Outdated Show resolved Hide resolved
crates/re_renderer/examples/picking.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/draw_phases/picking_layer.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/draw_phases/picking_layer.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/renderer/debug_overlay.rs Outdated Show resolved Hide resolved
crates/re_renderer/shader/debug_overlay.wgsl Outdated Show resolved Hide resolved
crates/re_renderer/src/wgpu_resources/mod.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/wgpu_resources/mod.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/wgpu_resources/mod.rs Show resolved Hide resolved
crates/re_renderer/src/view_builder.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/view_builder.rs Show resolved Hide resolved
@Wumpf Wumpf force-pushed the andreas/re_renderer/gpu-picking branch from 0be31be to c0601f9 Compare March 29, 2023 09:43
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Awesome!

crates/re_renderer/examples/picking.rs Outdated Show resolved Hide resolved
crates/re_renderer/examples/picking.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/draw_phases/picking_layer.rs Outdated Show resolved Hide resolved
let picking_target = ctx.gpu_resources.textures.alloc(
&ctx.device,
&TextureDesc {
label: view_name.clone().push_str(" - PickingLayerProcessor"),
Copy link
Member

Choose a reason for hiding this comment

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

ah right. Maybe a .concat(…) helper or similar to make it slightly more obvious it is not a normal string

Suggested change
label: view_name.clone().push_str(" - PickingLayerProcessor"),
label: view_name.concat(" - PickingLayerProcessor"),

crates/re_renderer/src/draw_phases/picking_layer.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/draw_phases/picking_layer.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/renderer/debug_overlay.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/renderer/debug_overlay.rs Outdated Show resolved Hide resolved
@emilk
Copy link
Member

emilk commented Mar 29, 2023

Oh, and make sure you test on web!

@Wumpf
Copy link
Member Author

Wumpf commented Mar 29, 2023

I might be imagining this but I think the last commit fixes the accuracy issue you mentioned. I need to try on an low res screen later again, I have a hard time squinting at these high dpi screens and make out much difference

@Wumpf Wumpf merged commit d84d2d1 into main Mar 29, 2023
@Wumpf Wumpf deleted the andreas/re_renderer/gpu-picking branch March 29, 2023 13:58
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants