Optimize gathering of point cloud colors #3730
Conversation
before/after screenshot or didn't happen 😄 |
| if a == 255 { | ||
| // Common-case optimization | ||
| re_renderer::Color32::from_rgb(r, g, b) | ||
| } else { | ||
| re_renderer::Color32::from_rgba_unmultiplied(r, g, b, a) | ||
| } |
There was a problem hiding this comment.
now that you marked all color methods inline emilk/egui@38b4234 and given that from_rgba_unmultiplied already has this short-circuit, this shouldn't actually be here, amiright? :)
There was a problem hiding this comment.
It depends on when we update to egui, and wether or not the compiler will actually inline it, and what effect that will have, yadda yadda
Or better yet: add a regression benchmark. |
|
I'll see if I can create a high-level benchmark of |
|
The new benchmark will be very useful for: |
|
|
crates/re_log_types/src/data_row.rs
Outdated
| pub fn from_component_batches( | ||
| row_id: RowId, | ||
| timepoint: TimePoint, | ||
| entity_path: EntityPath, | ||
| as_components: &dyn AsComponents, | ||
| ) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
I would expect this to take an iterator of ComponentBatches: it's strictly more expressive, easier to build / come by and more consistent with the rest of our APIs.
| pub fn from_component_batches( | |
| row_id: RowId, | |
| timepoint: TimePoint, | |
| entity_path: EntityPath, | |
| as_components: &dyn AsComponents, | |
| ) -> anyhow::Result<Self> { | |
| pub fn from_component_batches( | |
| row_id: RowId, | |
| timepoint: TimePoint, | |
| entity_path: EntityPath, | |
| comp_batches: impl IntoIterator<Item = &'a dyn ComponentBatch>, | |
| ) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
How would we get the num_instances in that case? Take the max of all the batches? What if there are no batches, or if they are all splats?
There was a problem hiding this comment.
Take the max of all the batches?
Yes, that matches the behavior of our log methods (in all languages, even!):
- https://github.com/rerun-io/rerun/blob/main/crates/re_sdk/src/recording_stream.rs#L710-L720
- https://github.com/rerun-io/rerun/blob/main/rerun_py/rerun_sdk/rerun/_log.py#L206-L207
What if there are no batches
Then there's nothing in the row and there are no instances
or if they are all splats?
You cannot have "all splats", that would just result in a row with num_instances = 1.
There was a problem hiding this comment.
Oh, I thought we stored num_instances separately.
So what happens if I log a full point cloud first and then later want to update all the colors with a splat color - that would be a new row with num_instance = 1 then, even though it will affect several instances?
There was a problem hiding this comment.
That's where it becomes funky...
If you do this:
rr.log("random", rr.Points3D(positions, colors=colors, radii=radii))
rr.log_components("random", [rr.components.ColorBatch([255, 0, 0])])Then you're going to end up with the original colors being discarded, a single red point and the rest of the points using the default color for this entity path (because that ColorBatch is not a splat).
Now, there is a trick at your disposal... you could do this:
rr.log("random", rr.Points3D(positions, colors=colors, radii=radii))
rr.log_components("random", [rr.components.ColorBatch([255, 0, 0])], num_instances=2)And now you'll end up with only red points, because you explicitly said that the data was 2 instances wide, and so the log function considers the ColorBatch to be a splat...
Of course we could change things so that logging 1 thing is always considered a splat, but then you have the opposite problem, which might or might not be better depending on the situation 🤷.
And this is why I don't like that splats are a logging-time rather than a query-time concern: the view should get to decide what to do with the data that it has as its disposal, and that behavior should be configurable through blueprints and through the UI.
This instance key business is pretty similar to e.g. configurable texture clamping modes in gfx APIs after all.
Saves around 2ms of native CPU-time on OPF. On native this is mostly hidden by the parallelism, but on web it is a real saving.
Found by looking at the in-browser profiler.
What
Checklist