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

Improved readback data handling #1734

Merged
merged 9 commits into from
Mar 31, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Mar 30, 2023

Makes it much easier to handle readback data. There's a bunch of paradigm shifts in how we do administrate them now:

  • user or re_renderer is no longer forced to consume all data in one go, instead we'll search if the data you request is available and return it then
    • this leaves the possibility for stale data, which might cause heavy memory leaks, so we clean stale stuff up automatically! (this can happen easily in a healthy application! Think closing a view for which you request picking data)
  • identifiers are user-chosen and not required to be unique (space view id is a good & valid identifier!)
  • readback data can cary arbitrary user data
  • the readback belt itself is a strictly internal datastructure now. Higher level systems like ScreenshotProcessor (new! shifting further towards a class-per-draw-pass; this is still an ongoing evolution!) or PickingLayerProcessor wrap your userdata and provide everything you need to know about for their readback data
    • this keeps the readback belt agnostic while presenting high level constructs where needed!

Testing:

  • native: viewer, sample multiview & picking
  • special pass with extra logging on buffer allocs in picking demo, testing what happens if it never queries data back and what happens if it does
  • web: samples multiview & picking

Checklist

@Wumpf Wumpf added the 🔺 re_renderer affects re_renderer itself label Mar 30, 2023
/// While readback buffer starts are guaranteed to be aligned correctly, there might need to be extra padding needed between texture copies.
/// This method will add the required padding between the texture copies if necessary.
/// Panics if the buffer is too small.
pub fn read_multiple_texture2d(
Copy link
Member Author

@Wumpf Wumpf Mar 30, 2023

Choose a reason for hiding this comment

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

this became multiple for the sole purpose of handling depth textures later on. Struggled a lot with expressing the "I want several things to be done at once" and opted for the less user friendly way since I only have one usecase in mind right now and I think I know what I'm doing (wish me luck) ;)

@teh-cmc
Copy link
Member

teh-cmc commented Mar 30, 2023

I'll happily take that one first thing tomorrow morning

@teh-cmc teh-cmc self-requested a review March 31, 2023 06:43
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.

The new APIs are soooo smooth 😱 👏

crates/re_viewer/src/ui/view_spatial/screenshot.rs Outdated Show resolved Hide resolved
crates/re_renderer/examples/multiview.rs Outdated Show resolved Hide resolved
crates/re_renderer/examples/picking.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/allocator/gpu_readback_belt.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/view_builder.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/view_builder.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/allocator/gpu_readback_belt.rs Outdated Show resolved Hide resolved
}

{
let range = chunk.ranges_in_use.swap_remove(range_index);
Copy link
Member

@teh-cmc teh-cmc Mar 31, 2023

Choose a reason for hiding this comment

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

Wait, how is this legal? we're invalidating the iterator?! I assume I'm missing the obvious and need to go back to bed...

Copy link
Member Author

@Wumpf Wumpf Mar 31, 2023

Choose a reason for hiding this comment

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

it's legal because there's a return after it. When writing this I was hoping so badly that it gets that and then it did \o/

Copy link
Member

Choose a reason for hiding this comment

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

but... there is no return? the only return that follows is conditional?!

Copy link
Member Author

Choose a reason for hiding this comment

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

no, the return here is unconditional. It is after a conditional, but the return is not part of it

Copy link
Member Author

Choose a reason for hiding this comment

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

or rather, the return is still conditional but it is in the same condition as the line you commented on. I.e. both swap_remove happen only when the loop is definitely exited

@Wumpf Wumpf merged commit 6dbc5b9 into main Mar 31, 2023
@Wumpf Wumpf deleted the andreas/re_renderer/improved-readback-data-handling branch March 31, 2023 09:32
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.

2 participants