[pdal] filters.icp transformation

Fri Mar 6 08:51:55 PST 2020

See my latest comment in the PR. I think the way to go is to remove the
centering step altogether. It doesn't address the ease of use, but that's
really a separate issue anyway.

On Fri, Mar 6, 2020 at 11:50 AM James Klassen <klassen.js at gmail.com> wrote:

> 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/9f699454/attachment.html>