[pdal] filters.icp transformation

Fri Mar 6 08:49:55 PST 2020

Some ideas:

Add a centroid (or center?) option to filters.transformation so this can be
done in one filters.transform stage.

It would be slightly simpler if filters.transformation (or a maybe a
different filter) could read the filters.icp metadata file directly instead
of having to separately shuttle the values across to a second pipeline.
This could get complicated if the filters.icp was part of a more complex
pipeline (possibly with multiple filters.icp operations).

If the goal is just to run ICP on a sub-sampled dataset (for performance
reasons) and the apply the resulting transformation to the whole dataset, a
parameter could be added to filters.icp so that it does the sub-sampling
internally for the ICP stage and then uses the full dataset when it applies
the transformation.  This only helps a very specific use case though and
leaves questions as to how the sample should be created and assumes that
the transformation only needs to be applied once (vs to many tiles, etc.)
This might make examples simpler, but in general, on real data, I'm not
sure this would help.  (This is made worse because filters.icp doesn't
support streaming, and so even if the ICP part is only done on a subset of
the date, it will still be limited in total dataset size, while using a
separate filters.transformation supports streaming and wouldn't be limited).

Overall: With the PR, it is good enough for me.

On Fri, Mar 6, 2020 at 9:39 AM Bradley Chambers <brad.chambers at gmail.com>
wrote:

> Jim, do you have any thoughts on how you'd rather communicate the
> transformation result of ICP (whatever we settle on)? What would be more
> user friendly?
>
> On Fri, Mar 6, 2020 at 10:24 AM Jim Klassen <klassen.js at gmail.com> wrote:
>
>> I suspect we could compose it three operations into a single transform
>> matrix, but probably at a loss of precision.  (I suspect preserving
>> precision this is why filters.icp bothers with calculating and shifting to
>> the centroid in the first place).
>>
>> Alternatively, it might be simpler for the user if the centroid value and
>> matrix could be passed directly into filters.transform (instead of needing
>> three filters.transforms in the pipeline).
>>
>> In my opinion, the current workflow of saving the metadata, manually
>> parsing it, and then constructing a new pipeline with values from the
>> metadata isn't very user friendly in general.  If this was something I was
>> doing regularly, I would certainly write a script do those steps for me
>> (probably as part of a larger workflow).  And if a script is generating the
>> second pipeline, it doesn't really matter to me if the second pipeline is
>> one stage or three stages.
>>
>> I have a first pass at a PR here:
>> https://github.com/PDAL/PDAL/pull/2962
>>
>> On Fri, Mar 6, 2020, 08:47 Bradley Chambers <brad.chambers at gmail.com>
>> wrote:
>>
>>> I’m wondering if it would make more sense to simply compose the three
>>> steps into a single output transformation. Making the user do the three
>>> steps themselves seems overly complicated. Is there a scenario in which we
>>> only want the transformation of the centered clouds (current state of the
>>> code)? Your use case is probably more common.
>>>
>>> On Thu, Mar 5, 2020 at 18:53 Jim Klassen <klassen.js at gmail.com> wrote:
>>>
>>>> Does a strategy of adding the centroid to the filters.icp metadata and
>>>> updating filters.icp documentation with the pipeline to apply it sound good?
>>>> On 3/5/20 5:41 PM, Andrew Bell wrote:
>>>>
>>>> This seems the same as this already reported bug:
>>>>
>>>> https://github.com/PDAL/PDAL/issues/2939
>>>>
>>>> If you want to submit a PR, I'll look it over.
>>>>
>>>> Thanks!
>>>>
>>>>
>>>> On Thu, Mar 5, 2020 at 6:36 PM Jim Klassen <klassen.js at gmail.com>
>>>> wrote:
>>>>
>>>>> I am trying to do something similar (apply ICP on a subset of a large
>>>>> dataset) and am running into confusion over the transformation matrix
>>>>> in
>>>>> the metadata.
>>>>>
>>>>> The point cloud output of the ICP filter on the subset looks good.
>>>>> However when I try to use the resulting "transform" metadata as a
>>>>> input
>>>>> into filters.transformation.matrix over the full dataset, the
>>>>> resulting
>>>>> transformation moves the points wildly out of position.
>>>>>
>>>>> Looking at the code, in IterativeClosestPoint.cpp, it looks like the
>>>>> transformation matrix (T) is applied after shifting the point cloud to
>>>>> the centroid of the reference point cloud (C), and the shifted back to
>>>>> regular coordinates.
>>>>>
>>>>> So something like Pout = ((P - C) * T) + C
>>>>>
>>>>> Where P is a 4 element input point vector (x,y,z,1), Pout is the
>>>>> output
>>>>> point vector, C is the centroid vector (x,y,z,0), T is the 4x4
>>>>> transformation matrix found by ICP.
>>>>>
>>>>> T is reported in the metadata for the stage, however, C is not
>>>>> reported
>>>>> in the metadata.
>>>>>
>>>>> But the transformation matrix that filters.transformation expects
>>>>> something more like
>>>>>
>>>>> Pout = P * T'
>>>>>
>>>>> If C is not (0,0,0) then T and T' will be different.
>>>>>
>>>>> I modified IterativeClosestPoint.cpp to also export the centroid to
>>>>> the
>>>>> metadata and then created a pipeline with three transformations, first
>>>>> shifting by the negative of the centroid, then applying T, then
>>>>> shifting
>>>>> back by the centroid and that produced the expected results.
>>>>>
>>>>>
>>>>> On 2/18/20 7:24 PM, Bradley Chambers wrote:
>>>>> > The options are very heavily borrowed from the PCL library, where
>>>>> this
>>>>> > was a squared value. Also, keep in mind that the default values may
>>>>> be
>>>>> > better suited for terrestrial/robotics applications (as opposed to
>>>>> > aerial), and will very likely need some tweaking.
>>>>> >
>>>>> > The reported transformation should also be reported in row major
>>>>> > order, like the transformation filter. If you find it is not, I’d
>>>>> > consider that a bug, so feel free to file a ticket.
>>>>> >
>>>>> > PRs are always welcome, especially for documentation!
>>>>> >
>>>>> > Brad
>>>>> >
>>>>>
>>>>> _______________________________________________
>>>>> pdal mailing list
>>>>> pdal at lists.osgeo.org
>>>>> https://lists.osgeo.org/mailman/listinfo/pdal
>>>>
>>>>
>>>>
>>>> --
>>>> Andrew Bell
>>>> andrew.bell.ia at gmail.com
>>>>
>>>> _______________________________________________
>>>> pdal mailing list
>>>> pdal at lists.osgeo.org
>>>> https://lists.osgeo.org/mailman/listinfo/pdal
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/pdal/attachments/20200306/37e76287/attachment-0001.html>