The implementation of ring_outlier_filter seems to have incorrect part

[taisa1]

Checklist

• I've read the contribution guidelines.
• I've searched other issues and no duplicate issues were found.
• I'm convinced that this is not my fault but a bug.

Description

There appears to be some issues of the implementation of ring_outlier_filter, so I listed them below.
The target of this issue is this code.

1. The last point in each ring seems to be not processed

After exiting the for loop (starts from line 126), walk_last_idx is indices.size() - 2. So even if it loops over [walk_first_idx, walk_last_idx], the last element will be ignored.

2. The same point can be pushed back to outlier_pcl multiple times

In the for loop (starts from line 185), input_ptr is always same, because the index is always walk_first_idx. I think the index should be i.

3. Incorrect type specification when copying point data

According to ring-outlier-filter.md, input is in PointXYZIRCAEDT format, so channel should be uint16_t. However, when copying, it is read as uint8_t. Also, intensity should be uint8_t, but it is read as float in line 226.

4. Same process

It's a matter of style, but I think the same processing can be unified.

Below is the psuedo code that shows the solution of problem 1 and 4.

before:

l=0, r=-1
for i=0 to N-2 {
    r=i
    if is_same_walk(a[i], a[i+1]) then
        continue
    copy_points(a[l,r])
    l=i+1
}
if l>r then 
    continue
copy_points(a[l,r])

after:

l=0, r=-1
for i=0 to N-1{
    r=i
    if i < N-1 then
        if is_same_walk(a[i], a[i+1]) then
            continue
    copy_points(a[l,r])
    l=i+1
}

It will change the output topic and it may be difficult for me to test, so I posted this as a issue.
Please let me know if the behavior is intended or if I've misunderstood something.

Expected behavior

None

Actual behavior

None

Steps to reproduce

None

Versions

No response

Possible causes

No response

Additional context

No response

[badai-nguyen]

@mojomex As I mentioned internally, I really appreciate if you could follow up this issue as you did a major change in algorithm recently.

[mojomex]

Hi @taisa1, thank you for the issue.
As for 1), you are absolutely correct. Actually, there is a performance improvement PR (that has been rolled back until dependent modules can deal with reordered points) that restructures this code (and should be correct):

• perf(ring_outlier_filter): a cache friendly impl (continuation of VRichardJP's work) #4185
• revert(perf_ring_outlier_filter): temporal revert for further investigation #6148 (reverted here)

Good catch for 2) and 3)!
I will push a PR with a fix some time this week.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity.