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

Fix arrows requiring a radius to be visible #1720

Merged
merged 6 commits into from
Mar 28, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Mar 27, 2023

Fixes #1708

.. that is the immediately user-facing problem this solves. But in trying to solve this properly I had to make the line renderer a bit more predictable: Previously, line caps would extend the line. Now they stay within the bounds of the previously determined line positions!

2D Demo before - note how caps make the lines longer:
image

and after - all the same length now:
image

rounded rectangles are still rounded (requires outward extending the rounded cap):
image

What an outline around it looks like (clock example):
image

Checklist

.. that is the immediately user-facing problem this solves. But in fact it makes the line renderer a bit more predictable: Previously, line caps would extend the line. Now they stay within the bounds of the previously determined line positions!
@Wumpf
Copy link
Member Author

Wumpf commented Mar 27, 2023

oops. not actually the same length yet on all combination. Thanks for pointing that out @nikolausWest 😅

@Wumpf Wumpf marked this pull request as draft March 27, 2023 15:05
@emilk
Copy link
Member

emilk commented Mar 27, 2023

For arrow tips I think it makes sense that the end-point is at the tip.

For rounded ends I find it more intuitive that the end-point is in the center of the hemisphere. That way one can easily combine multiple capsules together into a smooth line-strip with concentric hemispheres.

@Wumpf
Copy link
Member Author

Wumpf commented Mar 27, 2023

I actually broke rectangles I noticed a bit too late. I have now an option to configure the outward extensibility which addresses both that issue and your comment :)
(at least partially, as I don't allow mixing that yet, could easily extend it though)
View doesn't use rounded caps anywhere else yet, so punting on that question

@Wumpf Wumpf added 🪳 bug Something isn't working 🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself labels Mar 27, 2023
@Wumpf Wumpf marked this pull request as ready for review March 27, 2023 15:21
…arrows have a rounded base with outwards extension
@Wumpf
Copy link
Member Author

Wumpf commented Mar 27, 2023

image

there we go! overlapping could still be handled better, but let's not go there now

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.

Looks good, but I think the code could be simplified quite a bit!

crates/re_renderer/shader/lines.wgsl Outdated Show resolved Hide resolved
crates/re_renderer/shader/lines.wgsl Outdated Show resolved Hide resolved
crates/re_renderer/shader/lines.wgsl Outdated Show resolved Hide resolved
crates/re_renderer/shader/lines.wgsl Outdated Show resolved Hide resolved
crates/re_renderer/shader/lines.wgsl Outdated Show resolved Hide resolved
@Wumpf
Copy link
Member Author

Wumpf commented Mar 28, 2023

thanks for the input! Most of the stuff suggested doesn't work as-is because of some detail (typically on how cap triangles are flagged) but that does stress the point that this got a bit too hard to follow again. I'll try to make it simpler!

@Wumpf Wumpf force-pushed the andreas/fix-arrow-requiring-radius branch from a39c36a to a8787dc Compare March 28, 2023 09:11
@Wumpf Wumpf merged commit 0ff3191 into main Mar 28, 2023
@Wumpf Wumpf deleted the andreas/fix-arrow-requiring-radius branch March 28, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging Arrows without radius doesn't show anything
2 participants