<div dir="ltr">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.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 6, 2020 at 11:50 AM James Klassen <<a href="mailto:klassen.js@gmail.com">klassen.js@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Some ideas:</div><div><br></div><div>Add a centroid (or center?) option to filters.transformation so this can be done in one filters.transform stage.<br></div><div><br></div><div>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).</div><div><div><br></div><div>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).<br></div><div><br></div><div>Overall: With the PR, it is good enough for me.<br></div><div><div><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 6, 2020 at 9:39 AM Bradley Chambers <<a href="mailto:brad.chambers@gmail.com" target="_blank">brad.chambers@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>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?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 6, 2020 at 10:24 AM Jim Klassen <<a href="mailto:klassen.js@gmail.com" target="_blank">klassen.js@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div dir="auto">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).<br>
<div dir="auto"><br>
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).<br>
<br>
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.<br>
<br>
I have a first pass at a PR here:<br>
<a href="https://github.com/PDAL/PDAL/pull/2962" target="_blank">https://github.com/PDAL/PDAL/pull/2962</a><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Mar 6, 2020, 08:47
Bradley Chambers <<a href="mailto:brad.chambers@gmail.com" target="_blank">brad.chambers@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div>
<div>
<div dir="auto">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. </div>
</div>
</div>
</div>
<div>
<div><br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Mar 5, 2020 at
18:53 Jim Klassen <<a href="mailto:klassen.js@gmail.com" rel="noreferrer" target="_blank">klassen.js@gmail.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>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?<br>
</p>
</div>
<div>
<div>On 3/5/20 5:41 PM, Andrew Bell wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">This seems the same as this already
reported bug:
<div><br>
</div>
<div><a href="https://github.com/PDAL/PDAL/issues/2939" rel="noreferrer" target="_blank">https://github.com/PDAL/PDAL/issues/2939</a><br>
</div>
<div><br>
</div>
<div>If you want to submit a PR, I'll look it
over.</div>
<div><br>
</div>
<div>Thanks!</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Mar 5,
2020 at 6:36 PM Jim Klassen <<a href="mailto:klassen.js@gmail.com" rel="noreferrer" target="_blank">klassen.js@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I am trying
to do something similar (apply ICP on a subset
of a large <br>
dataset) and am running into confusion over the
transformation matrix in <br>
the metadata.<br>
<br>
The point cloud output of the ICP filter on the
subset looks good. <br>
However when I try to use the resulting
"transform" metadata as a input <br>
into filters.transformation.matrix over the full
dataset, the resulting <br>
transformation moves the points wildly out of
position.<br>
<br>
Looking at the code, in
IterativeClosestPoint.cpp, it looks like the <br>
transformation matrix (T) is applied after
shifting the point cloud to <br>
the centroid of the reference point cloud (C),
and the shifted back to <br>
regular coordinates.<br>
<br>
So something like Pout = ((P - C) * T) + C<br>
<br>
Where P is a 4 element input point vector
(x,y,z,1), Pout is the output <br>
point vector, C is the centroid vector
(x,y,z,0), T is the 4x4 <br>
transformation matrix found by ICP.<br>
<br>
T is reported in the metadata for the stage,
however, C is not reported <br>
in the metadata.<br>
<br>
But the transformation matrix that
filters.transformation expects <br>
something more like<br>
<br>
Pout = P * T'<br>
<br>
If C is not (0,0,0) then T and T' will be
different.<br>
<br>
I modified IterativeClosestPoint.cpp to also
export the centroid to the <br>
metadata and then created a pipeline with three
transformations, first <br>
shifting by the negative of the centroid, then
applying T, then shifting <br>
back by the centroid and that produced the
expected results.<br>
<br>
<br>
On 2/18/20 7:24 PM, Bradley Chambers wrote:<br>
> The options are very heavily borrowed from
the PCL library, where this <br>
> was a squared value. Also, keep in mind
that the default values may be <br>
> better suited for terrestrial/robotics
applications (as opposed to <br>
> aerial), and will very likely need some
tweaking.<br>
><br>
> The reported transformation should also be
reported in row major <br>
> order, like the transformation filter. If
you find it is not, I’d <br>
> consider that a bug, so feel free to file a
ticket.<br>
><br>
> PRs are always welcome, especially for
documentation!<br>
><br>
> Brad<br>
><br>
<br>
_______________________________________________<br>
pdal mailing list<br>
<a href="mailto:pdal@lists.osgeo.org" rel="noreferrer" target="_blank">pdal@lists.osgeo.org</a><br>
<a href="https://lists.osgeo.org/mailman/listinfo/pdal" rel="noreferrer noreferrer" target="_blank">https://lists.osgeo.org/mailman/listinfo/pdal</a></blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr">Andrew Bell<br>
<a href="mailto:andrew.bell.ia@gmail.com" rel="noreferrer" target="_blank">andrew.bell.ia@gmail.com</a></div>
</blockquote>
</div>
_______________________________________________<br>
pdal mailing list<br>
<a href="mailto:pdal@lists.osgeo.org" rel="noreferrer" target="_blank">pdal@lists.osgeo.org</a><br>
<a href="https://lists.osgeo.org/mailman/listinfo/pdal" rel="noreferrer noreferrer" target="_blank">https://lists.osgeo.org/mailman/listinfo/pdal</a></blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote></div>
</blockquote></div>
</blockquote></div>