Properly scale radii for 2D parts shown in 3D space view's pinhole image plane #4196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abey79
changed the title
abey79
commented
Nov 10, 2023
emilk
reviewed
Nov 10, 2023
| @@ -96,3 +97,30 @@ where | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| fn get_radii_scale_factor( | |||
There was a problem hiding this comment.
emilk
reviewed
Nov 10, 2023
| @@ -23,6 +23,7 @@ use re_viewer_context::{ | |||
| /// Context objects for a single entity in a spatial scene. | |||
| pub struct SpatialSceneEntityContext<'a> { | |||
| pub world_from_entity: glam::Affine3A, | |||
| pub radii_scale_factor: Option<f32>, | |||
There was a problem hiding this comment.
This needs a docstring: what is it? And what is the difference between None and Some(1.0)?
Wumpf
requested changes
Nov 10, 2023
There was a problem hiding this comment.
Given that we don't apply scale factors from transforms this PR is not doing anything wrong per se. But the correct solution is ofc to, well, apply scales from arbitrary transforms!
Meaning, today we also have the bug that if you apply a scale transform to a pointcloud/lines it will just move points further apart/together (or scale length of lines), it however never changes the thickness of lines or points which is desriable whenever their radius is given in world units (remember: We do both)
This can be done fully inside the shader without hurting any rust code. Have it working now, just need to test it a bit more to see if I missed something
|
My upcoming version will also fix |
4 tasks
Wumpf
added a commit
that referenced
this pull request
Nov 13, 2023
…cale & projection via Pinhole (#4199) ### What Longstanding issue! * Fixes #1223 * Fixes #1219 * Fixes #2494 * Replaces #4196 Shader only fix, in the future the scale factor shouldn't be extracted on the fly for every vertex out of the transform and instead passed in, but I wanted to keep the change minimal. The added vertex shading cost is unlikely to matter all that much _short term_. (also was very nice iterating on this and get before/after screenshots ;)) Throw-away test script for this: ```py import numpy as np import rerun as rr rr.init("scale fix test!!!", spawn=True) ############################# # #2494 & #1219 ############################# rr.log("world/camera", rr.ViewCoordinates.RDF, timeless=True) rr.log( "world/camera/image", rr.Pinhole( image_from_camera=np.array([[500, 0, 250], [0, 500, 250], [0, 0, 1]]), width=500, height=500, ), ) rr.log("world/camera/image/rgb", rr.Image(np.ones((500, 500, 3)))) rr.log( "world/camera/image/points", rr.Points2D(np.random.uniform(0, 500, (30, 2)), radii=1), ) ############################# # #1223 ############################# rr.log( "scaling_stuff/points_unscaled", rr.Points3D( np.random.uniform(0, 1, (30, 3)), radii=0.1, ), ) rr.log( "scaling_stuff/points_scaled", rr.Points3D( np.random.uniform(0, 1, (30, 3)), radii=0.1, ), rr.Transform3D(scale=2.0, translation=[2, 2, 2]), ) rr.log( "scaling_stuff/lines_unscaled", rr.LineStrips3D([[0, 1, 0], [0, 1, 1], [0, 0, 3]], radii=0.1), ) rr.log( "scaling_stuff/lines_scaled", rr.LineStrips3D([[0, 1, 0], [0, 1, 1], [0, 0, 3]], radii=0.1), rr.Transform3D(scale=2.0, translation=[2, 2, 2]), ) ``` Result: Before:  After:  This not only fixes issues with 2D->3D but also with 3D->2D. Here we add the 3D points to the 2D camera and set a world space size for the points: Before:  After:  ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4199) (if applicable) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4199) - [Docs preview](https://rerun.io/preview/7465b1650dd9be5c712c994827ced405c142edad/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/7465b1650dd9be5c712c994827ced405c142edad/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
fix_radii_2d_pinhole.mp4
Checklist