Changed calculation for range clipping #325
Conversation
Signed-off-by: Tomas Lorente <[email protected]>
| @@ -139,7 +139,8 @@ void main() | |||
| } | |||
|
|
|||
| // clamp xyz and set rgb to background color | |||
| if (point.x > far - tolerance) | |||
| if (point.x * point.x + point.y * point.y > | |||
| (far - tolerance) * (far - tolerance)) | |||
There was a problem hiding this comment.
how about just length(point) > far - tolerance to be consistent with how the gpu lidar clips ranges?
Similarly for clipping points within near clip plane: length(point) < near + tolerance
There was a problem hiding this comment.
Makes sense, I had no idea there was a length function. Good catch
There was a problem hiding this comment.
Does 9bfe9d1 works?
There was a problem hiding this comment.
yeah that looks fine. INTEGRATION_depth_camera test caught this change and is failing. Can you update the test?
Can you also add a note about this change in the Migration.md guide?
There was a problem hiding this comment.
Where should I note the change in the migration guide? 5 -> 6 ?
There was a problem hiding this comment.
yeah, you can add a new section Ignition Rendering 5.0 to 6.0
There was a problem hiding this comment.
34267e4 should take care of this
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Tomas Lorente <[email protected]>
|
Given that the issue is about longer ranges, and that closer clipping is giving some issues that go farther than this PR, this is going to tackle only far clipping and not near clipping. The near clipping seems to be related to #356 (comment) , would need that fixed before near clipping can be done like far clipping |
Codecov Report
@@ Coverage Diff @@
## main #325 +/- ##
=======================================
Coverage 58.16% 58.16%
=======================================
Files 170 170
Lines 16788 16788
=======================================
+ Hits 9764 9765 +1
+ Misses 7024 7023 -1
Continue to review full report at Codecov.
|
| @@ -139,7 +139,7 @@ void main() | |||
| } | |||
|
|
|||
| // clamp xyz and set rgb to background color | |||
| if (point.x > far - tolerance) | |||
There was a problem hiding this comment.
can you make the same change to depth_camera_final_fs.glsl?
There was a problem hiding this comment.
If I change this, the tests start falling again. expecting -inf and getting inf https://github.com/ignitionrobotics/ign-rendering/blob/lobotuerk/depthRangeChange/test/integration/depth_camera.cc#L324-L326
There was a problem hiding this comment.
Changing both that and https://github.com/ignitionrobotics/ign-rendering/blob/lobotuerk/depthRangeChange/test/integration/depth_camera.cc#L309-L311 tomaxVal make all tests pass, but I don't think that makes sense
There was a problem hiding this comment.
I think the problem is that because point was previously clamped to -inf so length(point) becomes +inf. So in depth_camera_final_fs.glsl we can change to something like:
if (!isinf(point.x) && length(point) > far - tolerance)
which just skips this additional clamping if it's already been clamped in the previous pass.
There was a problem hiding this comment.
34267e4 should take care of this
Signed-off-by: Tomas Lorente <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Tomas Lorente [email protected]
🦟 Bug fix
Fixes osrf/subt#342
Summary
This changes clipping to be based on distance to the focal point instead of distance to the focal plane.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge