Distortion normalization improvement + fix "folding" on large distortions #3009

kjeppesen1 · 2021-05-22T00:34:47Z

Modifies how Brown's distortion equations are applied to better reflect real distortion. Image is projected from image plane to camera plane to apply distortion equations, then projected back to image plane.

Before/after focal scaling implementation:

Before/after extreme distortion edge case fix:

We use a tag <ignition:legacy_mode> in a distortion context to select this new behavior.

Regardless of if the new mode or legacy mode are used, this PR adds a bug fix for heavy distortions that prevents the distorted image from "folding back" on itself, as seen in the image above.

kjeppesen1 · 2021-05-22T00:35:03Z

@audrow

gazebo/rendering/Distortion.cc

audrow · 2021-05-28T17:47:56Z

One comment on the test we've added: We've added a more extreme barrel and pin cushion distortion to an existing test for distortion. This test is not very rigorous and may pass even before this PR or if the results are incorrect, for example because of parameter leaking #2527.

I think after #2527 is fixed, it would be good to add a stronger test to check that behavior is as expected. A possible work around for #2527 is to make environments with one camera per world and test their expected behavior individually.

scpeters · 2021-06-02T00:37:06Z

This test is not very rigorous and may pass even before this PR or if the results are incorrect, for example because of parameter leaking #2527.

I have run this test without the fixes in this branch and confirmed that it does not fail, reducing its usefulness.

for example because of parameter leaking #2527.

I believe that parameter leaking does not occur in this test because the individual cameras are each added dynamically using the SpawnCamera helper function. I think we don't need to wait for a fix to #2527 to improve this test and would prefer that we figure out how to do so in this pull request

scpeters · 2021-06-02T00:37:31Z

Before/after extreme distortion edge case fix:

how did you generate these images?

audrow · 2021-06-02T16:02:45Z

how did you generate these images?

We took them in Gazebo by placing a checkerboard directly in front of a camera with significant distortion (k1 = -0.5). Also, we're not doing border cropping here, so the gray around the edges is where there's no information.

scpeters · 2021-06-04T19:01:43Z

this is a behavior change, so I think we should use a namespaced SDFormat parameter to opt-in to it, to keep existing camera parameters functioning the same way. Something like <ignition:use_focal_length>? Please suggest an appropriate parameter name. If you want to disable cropping with this flag, you can do that as well

<distortion>
  <ignition:use_focal_length>true</ignition:use_focal_length>
  <k1>...</k1>
  ...
</distortion>

kjeppesen1 · 2021-06-04T20:36:59Z

this is a behavior change, so I think we should use a namespaced SDFormat parameter to opt-in to it, to keep existing camera parameters functioning the same way. Something like <ignition:use_focal_length>? Please suggest an appropriate parameter name.

I agree with this approach. Something that indicates that it will use the provided distortion coefficients directly would be ideal. Maybe <ignition:use_real_distortion>, <ignition:use_exact_coefficients>, or <ignition:directly_use_coefficients>, although those aren't great either.

If you want to disable cropping with this flag, you can do that as well

I am in favor of disabling cropping with this implementation since cropping invalidates the distortion parameters (and therefore defeats the purpose of using the tag).

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow · 2021-06-09T16:56:49Z

@scpeters and @kjeppesen1, I've added a <ignition:use_real_distortion> tag and extended existing tests to make sure that this works. I also rebased this branch.

audrow · 2021-06-09T17:01:18Z

I believe that parameter leaking does not occur in this test because the individual cameras are each added dynamically using the SpawnCamera helper function. I think we don't need to wait for a fix to #2527 to improve this test and would prefer that we figure out how to do so in this pull request

I'll think about a better test to add.

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow · 2021-06-10T17:06:46Z

I've applied the feedback from yesterday and added a test that I believe checks the feature.

Changes:

Use <ignition:legacy_mode> flag
Check for using legacy mode in the setCrop method
Add a test that checks for expected cropping and distortion behavior with legacy and non legacy modes

I spawned cameras in the test environment to get around #2527.

gazebo/rendering/Distortion.cc

scpeters · 2021-06-10T19:00:26Z

gazebo/rendering/Distortion.cc

+        distortedIdx = distortedRow * this->dataPtr->distortionTexWidth + distortedCol;
+
+        // check if the index has already been set
+        if (this->dataPtr->distortionMap[distortedIdx] != unsetPixelVector)


this logic block looks new. should we add && !this->dataPtr->legacyMode to the if statement? or we could reverse it:

if (this->dataPtr->legacyMode) { this->dataPtr->distortionMap[distortedIdx] = normalizedLocation; } // check if the index has already been set else if (this->dataPtr->distortionMap[distortedIdx] != unsetPixelVector) { ... }

if not, then we should clarify that this is a bug fix that applies in all cases

I think this can be considered a bug fix, since there will be the wrapping behavior shown in #3009 (comment) on very large barrel distortions with the legacy method.

I'm not sure what would be the best place to mention this. I can alter the PR to mention the bug fix and add a comment in this part of the code. Anywhere else?

I added a comment in the code (below) and modified the PR title and description. https://github.com/osrf/gazebo/blob/406b65e634b3ee6d45ffbd6f77d136caa048ca3a/gazebo/rendering/Distortion.cc#L240-L243

gazebo/rendering/Distortion.cc

test/integration/camera_sensor.cc

Signed-off-by: Audrow Nash <audrow@hey.com>

This reverts commit ba51bac. # Conflicts: # test/integration/camera_sensor.cc

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow · 2021-06-15T18:40:31Z

@mvespign, thanks for removing the duplicate method. This was done by 4fe9c4b (before 464adb1). I'm going to force push to remove the recent commits.

gazebo/rendering/Distortion.hh

worlds/test/issue_3003_distortion_implementation_correction.world

Signed-off-by: Audrow Nash <audrow@hey.com>

scpeters

the new test looks good. I think you've matched the style of other tests, though there is a fair bit of repeated code. Some lambda functions could probably reduce some code duplication, but it's fine as is

test/integration/camera_sensor.cc

Signed-off-by: Audrow Nash <audrow@hey.com>

scpeters

ok, I think this is almost there. I just have one minor nit, and I noticed some lines are longer than 80 characters. Our CI is not currently reporting style errors due to gazebo-tooling/release-tools#446, but I think avoiding trailing whitespace and long lines should be enough

test/integration/camera_sensor.cc

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow · 2021-06-17T00:36:37Z

I think that it should be good now.

scpeters · 2021-06-17T18:51:53Z

I've tested with libgazebo_ros_camera, and I think the logic is working properly now

scpeters

I just noticed some new clang warnings, but aside from that I approve

test/integration/camera_sensor.cc

Signed-off-by: Audrow Nash <audrow@hey.com>

gazebo/rendering/Distortion.cc

iche033 · 2021-06-18T00:21:26Z

gazebo/test/ServerFixture.hh

-                   double _cx = 0.5, double _cy = 0.5);
+                   double _cx = 0.5, double _cy = 0.5,
+                   bool _legacyMode = true,
+                   double _horizontalFov = 0.78539816339744828);


I think this breaks ABI?

ABI checker didn't show any issue here but I'm not sure if that's because it's not testing the ServerFixture library.

Also add \param[in] for _horizontalFov

I think this breaks ABI?

ABI checker didn't show any issue here but I'm not sure if that's because it's not testing the ServerFixture library.

I was wondering the same thing, but we only distribute ServerFixture in a static library, and I believe ABI is only a concern for dynamically loaded libraries

Signed-off-by: Audrow Nash <audrow@hey.com>

scpeters · 2021-06-21T17:41:33Z

Windows CI failed for an unrelated reason, and the failures in the other jobs look unrelated to me. Merging now...

kjeppesen1 commented May 22, 2021

View reviewed changes

gazebo/rendering/Distortion.cc Outdated Show resolved Hide resolved

kjeppesen1 mentioned this pull request May 22, 2021

Distortion implementation correction #3003

Closed

audrow self-assigned this May 22, 2021

audrow marked this pull request as ready for review May 27, 2021 00:32

kbjeppes and others added 6 commits June 8, 2021 14:15

Pixel coordinates projected to camera plane for distortion

9a3e7f8

Fixes wraparound issue for extreme distortions

ed7a417

Add test for extreme distortion

ba51bac

Signed-off-by: Audrow Nash <audrow@hey.com>

Refactor SetCamera

8a95b8d

Signed-off-by: Audrow Nash <audrow@hey.com>

Use old distortion method by default

eb4df36

Signed-off-by: Audrow Nash <audrow@hey.com>

Update distortion to test using real distortion too

77bb479

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow force-pushed the distortion_normalization branch from 47ad970 to 77bb479 Compare June 9, 2021 16:50

paudrow added 2 commits June 10, 2021 09:56

Apply feedback to distortion implementation

4a6781f

Signed-off-by: Audrow Nash <audrow@hey.com>

Test new and legacy distortion

bc4fd70

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow added the enhancement New feature or request label Jun 10, 2021

scpeters reviewed Jun 10, 2021

View reviewed changes

paudrow added 5 commits June 11, 2021 08:54

Remove empty line

519e6b4

Signed-off-by: Audrow Nash <audrow@hey.com>

Make new variables constant

f230f56

Signed-off-by: Audrow Nash <audrow@hey.com>

Add a comment about bugfix

774bab3

Signed-off-by: Audrow Nash <audrow@hey.com>

Remove duplication from distort methods

4fe9c4b

Signed-off-by: Audrow Nash <audrow@hey.com>

Revert "Add test for extreme distortion"

406b65e

This reverts commit ba51bac. # Conflicts: # test/integration/camera_sensor.cc

Update SpawnCamera's doc string

861f8eb

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow force-pushed the distortion_normalization branch from 1d51120 to 861f8eb Compare June 15, 2021 18:40

audrow requested review from scpeters and audrow and removed request for audrow June 15, 2021 18:42

scpeters reviewed Jun 15, 2021

View reviewed changes

paudrow added 2 commits June 15, 2021 14:53

Clean up white space

5de3259

Signed-off-by: Audrow Nash <audrow@hey.com>

Fix minecart urls

b0b627b

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow requested a review from scpeters June 15, 2021 21:56

scpeters reviewed Jun 15, 2021

View reviewed changes

Apply feedback

fa809d5

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow requested a review from scpeters June 16, 2021 00:40

scpeters reviewed Jun 16, 2021

View reviewed changes

test/integration/camera_sensor.cc Outdated Show resolved Hide resolved

paudrow added 2 commits June 16, 2021 17:30

Fix default lambda return

4fa5f30

Signed-off-by: Audrow Nash <audrow@hey.com>

Make lines less than 81 characters

d291950

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow requested a review from scpeters June 17, 2021 00:36

scpeters approved these changes Jun 17, 2021

View reviewed changes

test/integration/camera_sensor.cc Outdated Show resolved Hide resolved

Avoid capturing const variables

1c821cb

Signed-off-by: Audrow Nash <audrow@hey.com>

iche033 reviewed Jun 18, 2021

View reviewed changes

paudrow added 3 commits June 18, 2021 12:03

Change static variable to const

fd01e14

Signed-off-by: Audrow Nash <audrow@hey.com>

Fix curly brace location

c31c97e

Signed-off-by: Audrow Nash <audrow@hey.com>

Add docstring for _horizontalFov parameter

3caa105

Signed-off-by: Audrow Nash <audrow@hey.com>

audrow requested a review from iche033 June 18, 2021 19:08

iche033 approved these changes Jun 18, 2021

View reviewed changes

scpeters merged commit a708aa3 into gazebosim:gazebo11 Jun 21, 2021

scpeters mentioned this pull request Jun 28, 2021

Bump version to 11.7.0 #3034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distortion normalization improvement + fix "folding" on large distortions #3009

Distortion normalization improvement + fix "folding" on large distortions #3009

kjeppesen1 commented May 22, 2021 •

edited by audrow

Loading

kjeppesen1 commented May 22, 2021

audrow commented May 28, 2021

scpeters commented Jun 2, 2021

scpeters commented Jun 2, 2021

audrow commented Jun 2, 2021 •

edited

Loading

scpeters commented Jun 4, 2021

kjeppesen1 commented Jun 4, 2021 •

edited

Loading

audrow commented Jun 9, 2021 •

edited

Loading

audrow commented Jun 9, 2021

audrow commented Jun 10, 2021

scpeters Jun 10, 2021

audrow Jun 10, 2021 •

edited

Loading

audrow Jun 11, 2021

audrow commented Jun 15, 2021 •

edited

Loading

scpeters left a comment

scpeters left a comment

audrow commented Jun 17, 2021

scpeters commented Jun 17, 2021

scpeters left a comment

iche033 Jun 18, 2021

scpeters Jun 18, 2021

scpeters commented Jun 21, 2021

Distortion normalization improvement + fix "folding" on large distortions #3009

Distortion normalization improvement + fix "folding" on large distortions #3009

Conversation

kjeppesen1 commented May 22, 2021 • edited by audrow Loading

kjeppesen1 commented May 22, 2021

audrow commented May 28, 2021

scpeters commented Jun 2, 2021

scpeters commented Jun 2, 2021

audrow commented Jun 2, 2021 • edited Loading

scpeters commented Jun 4, 2021

kjeppesen1 commented Jun 4, 2021 • edited Loading

audrow commented Jun 9, 2021 • edited Loading

audrow commented Jun 9, 2021

audrow commented Jun 10, 2021

scpeters Jun 10, 2021

Choose a reason for hiding this comment

audrow Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

audrow Jun 11, 2021

Choose a reason for hiding this comment

audrow commented Jun 15, 2021 • edited Loading

scpeters left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

audrow commented Jun 17, 2021

scpeters commented Jun 17, 2021

scpeters left a comment

Choose a reason for hiding this comment

iche033 Jun 18, 2021

Choose a reason for hiding this comment

scpeters Jun 18, 2021

Choose a reason for hiding this comment

scpeters commented Jun 21, 2021

kjeppesen1 commented May 22, 2021 •

edited by audrow

Loading

audrow commented Jun 2, 2021 •

edited

Loading

kjeppesen1 commented Jun 4, 2021 •

edited

Loading

audrow commented Jun 9, 2021 •

edited

Loading

audrow Jun 10, 2021 •

edited

Loading

audrow commented Jun 15, 2021 •

edited

Loading