Bugfix for IntegralImageNormalEstimation #191

taketwo · 2013-07-13T22:18:56Z

This pull request fixes the bugs in IntegralImageNormalEstimation reported by James Stout in this pcl-dev tread and also introduces a new minimum normal smoothing size parameter.

The introduced fixes seem to play well with

COVARIANCE_MATRIX
SIMPLE_3D_GRADIENT
AVERAGE_DEPTH_CHANGE

(the green borders is an okay thing for this method)

As for AVERAGE_3D_GRADIENT, there are weird artifacts. Unfortunately I did not have time to dig deeper into this problem.

This commit implements the fixes proposed by James Stout in pcl-dev mailing list (http://www.pcl-developers.org/Bugs-in-IntegralImageNormalEstimation-distance-map-computation-td5708055.html). Specifically: - In depth change map computation the diagonal neighbors are now considered. - In distance map computation Chessboard distance is used instead of an approximation of Euclidean distance. - Use distance map values as smoothing window radius (instead of width).

This commit implements the change to IntergalImageNormalEstimation proposed in pcl-dev mailing list (http://www.pcl-developers.org/Bugs-in-IntegralImageNormalEstimation-distance-map-computation-td5708055.html).

wolfmanstout · 2013-07-15T20:56:23Z

features/include/pcl/features/impl/integral_image_normal.hpp

@@ -731,30 +731,65 @@
  memset (depthChangeMap, 255, input_->points.size ());

  unsigned index = 0;
-  for (unsigned int ri = 0; ri < input_->height-1; ++ri)
+  for (unsigned int ri = 1; ri < input_->height-1; ++ri)


The first row and first column are now getting skipped. Most of them get touched by the NE and SW checks, but that's not sufficient (for example if there is a left-right or up-down discontinuity in the first row it won't be counted).

Actually this bug was also somewhat present before (e.g. left-right discontinuities were not checked in the last row). This seems to get handled later by BORDER_POLICY_IGNORE, although it looks like BORDER_POLICY_MIRROR isn't doing the right thing. Maybe just zero out the borders and columns? This effectively makes BORDER_POLICY_MIRROR a no-op, but honestly I'm not sure how useful that mode is (seems like it would produce really bad normals).

taketwo · 2013-07-17T08:24:18Z

Thanks for the comments.

I also think that BORDER_POLICY_MIRROR is not quite useful. Should we get rid of it then?

As for the BORDER_POLICY_IGNORE, I think we could do the following. Previously the width of the ignoration strip was chosen to be equal to the normal smoothing size. Since now we have additional minimum_normal_smoothing_size_ parameter, I think it makes sense to use it instead (divided by two, by the way). Nevertheless, it is still important to have "up-down" and "left-right" discontinuities of the outermost rows/columns registered in the depth discontinuities map, so I will add additional for loops to handle it.

wolfmanstout · 2013-07-17T16:27:00Z

Regarding the border policies: your proposals sound good. I'm not sure if we can actually remove BORDER_POLICY_MIRROR since it's part of the public interface, but we could at least make it a no-op.

taketwo · 2013-07-27T20:20:09Z

Hi, I am back working on this issue. I started to explore the weird artifacts of AVERAGE_3D_GRADIENT and it seems like they are due to wrong smoothing window size computation. I implemented it as you suggested in the ml:

The other bug is that the square dimensions are half of what they ought to be. The distance map value is used
as the lower bound of the window width, when in fact it should be the bound of the window radius. In other words,
a pixel with distance 2 to a discontinuity should be using a 5x5 window, not 3x3 as it currently does.

i.e. smoothing size used is D * 2 + 1, where D is the value in distance map. Shouldn't it rather be D * 2 - 1?

The image below shows the results for D * 2 - 1 on the left and D * 2 + 1 or the right.

It is strange though that for other methods some similar artifact does not show up...

taketwo · 2013-07-28T10:36:05Z

Another thing I mentioned is so-called depth dependent smoothing. When enabled, static_cast<float>(depth)/10.0f is added to the normal_smoothing_size_:

float smoothing = (std::min)(distanceMap[index] * 2 + 1, normal_smoothing_size_ + static_cast<float>(depth)/10.0f);

Assuming that depth is less than 10 m, this addend is always less than 1. If the user sets normal_smoothing_size_ to an integer value, this addend has no effect because later on the sum is cast to int. You suggested to use round instead, which means that for distances larger than 5 meters smoothing will be increased by 1. I wonder if this makes any sense at all.

wolfmanstout · 2013-08-11T16:36:08Z

Hi Sergey,

Sorry for the slow response! I've been super busy lately.

D * 2 + 1 is what you want. Think of the discontinuity as occurring at the edge between pixels, and the distances are from the edge of the current pixel to that discontinuity. Then a distance of 1 means we can get away with a 3x3 pixel square. It makes sense that using D*2-1 would fix it, but only because it's hiding a bug elsewhere.

Regarding the casting that I think may causing trouble, I'm referring to the static_cast in the line where the smoothing is passed to setRectSize. Try a round instead. The cast you added looks fine.

taketwo · 2013-08-20T10:40:34Z

Hi James,

Consider the following depth buffer:

05 00
00 14

Assuming 10 is the max depth change, the depth_change map will be:

255 000
000 000

This will yield the following distance map:

1 0
0 0

According to D * 2 + 1 formula we should use 3x3 window for the upper left pixel. But this window will cover two depth discontinuities!

taketwo · 2013-08-20T10:48:26Z

Regarding the casting that I think may causing trouble, I'm referring to the static_cast in the line where the
smoothing is passed to setRectSize. Try a round instead. The cast you added looks fine.

Yes, I got that. I added round there, but it did not seem to change anything.

In my comment I actually questioned the utility of this whole "depth dependent smoothing" thing. As I said, it boils down to increasing normal_smoothing_size_ by 1 for distances greater than 5 m.

wolfmanstout · 2013-08-20T22:15:52Z

The diagonal case is a good example; I hadn't considered that. This seems like a failure of the distance mapping itself, not the window sizing. Specifically, the base-case we are propagating is disobeying its contract. There are a few ways we could fix this:

Modify the base case to check for these discontinuities. This would be an additional 8 checks.
Make the base case more conservative by halving the allowed discontinuity in the existing checks. By transitivity this would guarantee that the unchecked cases are also within the threshold.

Both options should fix the base case and inductively the rest of the distance map. The tradeoff is that the first option is slower, but it would sometimes lead to a larger window size where the 2nd case is too conservative. I'm fine with either solution.

I don't think this is the cause of the strange artifacts. As the 2nd solution points out, all this bug really does is double the max depth change parameter. It won't let any really severe discontinuities in.

wolfmanstout · 2013-08-20T22:19:44Z

Also, is there an easy way to run the tests you posted earlier? Are those just existing unit tests? When I have some time I'd like to run them and take a closer look at what might be going wrong.

rbrusu · 2013-12-16T16:56:34Z

What is the consensus on this issue?

taketwo · 2013-12-16T17:00:36Z

This issue became irrelevant for my work in TOCS so I haven't had enough time/motivation to continue with it. Perhaps I will come to it later, but for now we may put this pull request on hold.

wolfmanstout · 2013-12-21T00:54:22Z

I'm in a similar situation. This isn't actually a problem for me, it's just
something I noticed when reading the code for research purposes. Based
Sergey's results, however, it's clear that there is real room for
improvement, ready to be picked up and finished by anyone who is impacted
by this.

On Mon, Dec 16, 2013 at 9:00 AM, Sergey Alexandrov <[email protected]

wrote:

This issue became irrelevant for my work in TOCS so I haven't had enough
time/motivation to continue with it. Perhaps I will come to it later, but
for now we may put this pull request on hold.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/191#issuecomment-30678019
.

mcopejans · 2014-11-24T11:54:01Z

This issue is very relevant to my current work. I agree that the proposed fixes result in the improvements discussed above, which I have tested for the COVARIANCE_MATRIX case. Would it be possible to merge this issue in the next version of PCL?

taketwo · 2014-11-24T12:00:43Z

This pull request is not finished. As you can see there were some discussions/suggestions/ideas after my initial attempt, but I never got back to work on them. We can not merge the pull request as is, but you are welcome address all the issues we discussed and submit a new one :)

vfdev-5 · 2017-08-03T15:41:53Z

I wonder whether this problem will be solved ?
Normal estimation fails when there are NaN points between non NaN points: example 1 and example 2.
Thanks

stale · 2020-02-21T20:43:46Z

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

stale · 2020-05-19T05:59:08Z

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

taketwo added 2 commits July 14, 2013 00:01

Add minimum normal smoothing size parameter.

c598108

This commit implements the change to IntergalImageNormalEstimation proposed in pcl-dev mailing list (http://www.pcl-developers.org/Bugs-in-IntegralImageNormalEstimation-distance-map-computation-td5708055.html).

wolfmanstout reviewed Jul 15, 2013
View reviewed changes

taketwo mentioned this pull request Oct 9, 2013

added references for normal estimation using integral images #309

Merged

SergioRAgostinho added changelog: ABI break Meta-information for changelog generation status: stale labels Nov 23, 2017

stale bot closed this Dec 14, 2017

jspricke reopened this Dec 18, 2017

stale bot removed the status: stale label Dec 18, 2017

taketwo added the status: stale label Dec 18, 2017

taketwo mentioned this pull request Aug 27, 2019

Normal Estimation Using Integral Images not always working #3307

Open

kunaltyagi removed the stale label Feb 27, 2020

stale bot removed the status: stale label Feb 27, 2020

taketwo added needs: more work Specify why not closed/merged yet status: stale and removed changelog: ABI break Meta-information for changelog generation labels Feb 27, 2020

stale bot removed the status: stale label Feb 27, 2020

stale bot added the status: stale label May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for IntegralImageNormalEstimation #191

Bugfix for IntegralImageNormalEstimation #191

taketwo commented Jul 13, 2013

wolfmanstout Jul 15, 2013

taketwo commented Jul 17, 2013

wolfmanstout commented Jul 17, 2013

taketwo commented Jul 27, 2013

taketwo commented Jul 28, 2013

wolfmanstout commented Aug 11, 2013

taketwo commented Aug 20, 2013

taketwo commented Aug 20, 2013

wolfmanstout commented Aug 20, 2013

wolfmanstout commented Aug 20, 2013

rbrusu commented Dec 16, 2013

taketwo commented Dec 16, 2013

wolfmanstout commented Dec 21, 2013

mcopejans commented Nov 24, 2014

taketwo commented Nov 24, 2014

vfdev-5 commented Aug 3, 2017 •

edited

Loading

stale bot commented Feb 21, 2020

stale bot commented May 19, 2020

Bugfix for IntegralImageNormalEstimation #191

Are you sure you want to change the base?

Bugfix for IntegralImageNormalEstimation #191

Conversation

taketwo commented Jul 13, 2013

wolfmanstout Jul 15, 2013

Choose a reason for hiding this comment

taketwo commented Jul 17, 2013

wolfmanstout commented Jul 17, 2013

taketwo commented Jul 27, 2013

taketwo commented Jul 28, 2013

wolfmanstout commented Aug 11, 2013

taketwo commented Aug 20, 2013

taketwo commented Aug 20, 2013

wolfmanstout commented Aug 20, 2013

wolfmanstout commented Aug 20, 2013

rbrusu commented Dec 16, 2013

taketwo commented Dec 16, 2013

wolfmanstout commented Dec 21, 2013

mcopejans commented Nov 24, 2014

taketwo commented Nov 24, 2014

vfdev-5 commented Aug 3, 2017 • edited Loading

stale bot commented Feb 21, 2020

stale bot commented May 19, 2020

vfdev-5 commented Aug 3, 2017 •

edited

Loading