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

Add radius boost for depth clouds on outline #1713

Merged
merged 8 commits into from
Mar 27, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Mar 27, 2023

Normal point clouds:
point-outline

Depth cloud point clouds:
depth-cloud-outline

(looks carefully - the outlines have almost the same color as the background)

Checklist

@emilk emilk added ui concerns graphical user interface 🔺 re_renderer affects re_renderer itself labels Mar 27, 2023
@Wumpf Wumpf self-requested a review March 27, 2023 08:39
@emilk emilk marked this pull request as draft March 27, 2023 08:42
@Wumpf
Copy link
Member

Wumpf commented Mar 27, 2023

@ picture: you could have clicked them to make it more obvious 😄. But they are way too subtle now in too many cases (or is our background "wrong" ;)) :/. We need to find a better compromise

@emilk emilk force-pushed the emilk/point-radius-boost-for-depth-clouds branch from 44cffaa to d1e3dee Compare March 27, 2023 09:02
@emilk emilk force-pushed the emilk/point-radius-boost-for-depth-clouds branch from dbd865e to 7a81a40 Compare March 27, 2023 09:07
@emilk emilk marked this pull request as ready for review March 27, 2023 09:12
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.

still unhappy about the uniform buffer/binding groups here. But not a hill to die on! Looking good otherwise, the renames and cleanups are appreciated!

"depth_cloud_bg".into(),
create_and_fill_uniform_buffer(
ctx,
"PointCloudDrawData::DrawDataUniformBuffer".into(),
Copy link
Member

Choose a reason for hiding this comment

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

I started naming those by their field names and I think that's better practice than what we had before. I.e it would be DepthCloudDrawInstance::bind_group_opaque here. Not consistent across everywhere though, jury is still out on this ;)

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 think you are in a better position to take a rename-pass over all of re_renderer

crates/re_viewer/src/ui/view_spatial/scene/primitives.rs Outdated Show resolved Hide resolved
@emilk emilk merged commit d42c13d into main Mar 27, 2023
@emilk emilk deleted the emilk/point-radius-boost-for-depth-clouds branch March 27, 2023 10:13
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 ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants