Compatibility for Spin Image features #1260

jgarstka · 2015-06-15T08:09:19Z

A new feature signature SISignature153 replaces Histogram<153>.
SISignature153 is registered to by usable with algorithms like
kd-tree search.

A new feature signature SISignature153 replaces Histogram<153>. SISignature153 is registered to by usable with algorithms like kd-tree search.

andersgb1 · 2015-06-15T08:14:56Z

Just out of curiosity, why is it necessary with a new point type here? It
has the exact same signature as the Histogram struct.

On Mon, Jun 15, 2015 at 10:09 AM, Jens Garstka [email protected]
wrote:

A new feature signature SISignature153 replaces Histogram<153>.
SISignature153 is registered to by usable with algorithms like

kd-tree search.

You can view, comment on, or merge this pull request online at:

#1260
Commit Summary

Compatibility for Spin Image features

File Changes

M common/include/pcl/common/point_tests.h
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-0 (1)

M common/include/pcl/impl/point_types.hpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-1 (19)

M common/include/pcl/point_representation.h
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-2 (17)

M common/include/pcl/point_types.h
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-3 (9)

M common/src/point_types.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-4 (8)

M
doc/tutorials/content/sources/implicit_shape_model/implicit_shape_model.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-5 (6)

M examples/features/example_spin_images.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-6 (6)

M features/include/pcl/features/spin_image.h
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-7 (2)

M features/src/spin_image.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-8 (4)

M gpu/features/include/pcl/gpu/features/features.hpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-9 (4)

M gpu/features/src/features.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-10 (2)

M gpu/features/test/test_spinimages.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-11
(58)

M test/features/test_ptr.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-12 (2)

M test/features/test_spin_estimation.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-13 (5)

M test/test_recognition_ism.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-14 (6)

M tools/spin_estimation.cpp
https://github.com/PointCloudLibrary/pcl/pull/1260/files#diff-15 (9)

Patch Links:

https://github.com/PointCloudLibrary/pcl/pull/1260.patch

https://github.com/PointCloudLibrary/pcl/pull/1260.diff

—
Reply to this email directly or view it on GitHub
#1260.

jgarstka · 2015-06-15T08:30:03Z

Histogram<153> is not a registered type when compiling PCL. Hence you'll get linker errors if you try to use Histogram<153> with algorithms like Kdtree.

andersgb1 · 2015-06-15T08:58:41Z

Ah, yes you're right! I overlooked that.

But shouldn't Histogram be removed then? I mean, otherwise we will have two
identical structs with different names.

On Mon, Jun 15, 2015 at 10:30 AM, Jens Garstka [email protected]
wrote:

Histogram<153> is not a registered type when compiling PCL. Hence you'll
get linker errors if you try to use Histogram<153> with algorithms like
Kdtree.

—
Reply to this email directly or view it on GitHub
#1260 (comment)
.

jgarstka · 2015-06-15T09:06:17Z

Histogram is a generic point structure with the ability to use it with different N. It is not exclusively reserved for SpinImage and is still required by other classes, e.g., CRHEstimation.

andersgb1 · 2015-06-15T11:25:27Z

Yes, I know - but it's quite bad having two classes encapsulating the same
type of data. It would imo be better to just register a Histogram<153>
instead of introducing a new struct.

On Mon, Jun 15, 2015 at 11:06 AM, Jens Garstka [email protected]
wrote:

Histogram is a generic point structure with the ability to use it with
different N. It is not exclusively reserved for SpinImage and is still
required by other classes, e.g., CRHEstimation.

—
Reply to this email directly or view it on GitHub
#1260 (comment)
.

jgarstka · 2015-06-15T11:32:12Z

Sure, one could do. But what would be the benefit. It is only consistent to implement the signature of a SpinImage feature in the same manner as it is done for all other features.

jgarstka · 2015-06-15T12:03:58Z

... and by the way: there are no two classes encapsulating the same type of data, since you'll not find instances of Histogram<153> in the code anymore.

andersgb1 · 2015-06-15T12:28:46Z

Yes, you're right - types such as e.g. FPFHSignature33 and VFHSignature308
support your argument.

I guess then it would be wise to completely remove Histogram, since it is
becoming superfluous if we are using dedicated containers for all features.
But maybe that should be another PR...

On Mon, Jun 15, 2015 at 2:04 PM, Jens Garstka [email protected]
wrote:

... and by the way: there are no two classes encapsulating the same type
of data, since you'll not find instances of Histogram<153> in the code
anymore.

—
Reply to this email directly or view it on GitHub
#1260 (comment)
.

jspricke · 2015-10-14T21:08:12Z

+1 here, @taketwo what do you think?

taketwo · 2015-10-16T18:29:10Z

The changes look good, but I must admit I never used any of the affected classes in PCL.
The proposal to remove Histogram sounds a bit radical though.

jspricke · 2015-10-19T06:58:53Z

I would propose to mark Histogram deprecated in a future version.

taketwo · 2015-10-19T08:57:13Z

If we plan to deprecate, maybe it makes sense to deprecate already now, so that users of 1.8.0 see the warning and (hopefully) adapt their code?

andersgb1 · 2015-10-19T09:22:17Z

I would vote for that!

On Mon, Oct 19, 2015 at 10:57 AM, Sergey Alexandrov <
[email protected]> wrote:

If we plan to deprecate, maybe it makes sense to deprecate already now, so
that users of 1.8.0 see the warning and (hopefully) adapt their code?

—
Reply to this email directly or view it on GitHub
#1260 (comment)
.

taketwo · 2015-11-16T16:58:52Z

So. Shall we deprecate? @jgarstka can you include deprecation in this pull request?

stale · 2017-12-14T14:34:29Z

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 · 2018-03-26T23:11:37Z

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-02-21T20:43:32Z

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.

Compatibility for Spin Image features

fd2c6ed

A new feature signature SISignature153 replaces Histogram<153>. SISignature153 is registered to by usable with algorithms like kd-tree search.

SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Jul 14, 2016

SergioRAgostinho added this to the pcl-1.9.0 milestone Aug 22, 2016

stale bot added the status: stale label Dec 14, 2017

SergioRAgostinho removed this from the pcl-1.9.0 milestone Jan 25, 2018

stale bot removed the status: stale label Jan 25, 2018

stale bot added the status: stale label Mar 26, 2018

kunaltyagi removed the stale label Feb 27, 2020

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

taketwo added the status: stale label Feb 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility for Spin Image features #1260

Compatibility for Spin Image features #1260

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

kd-tree search.

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

jgarstka commented Jun 15, 2015

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

jspricke commented Oct 14, 2015

taketwo commented Oct 16, 2015

jspricke commented Oct 19, 2015

taketwo commented Oct 19, 2015

andersgb1 commented Oct 19, 2015

taketwo commented Nov 16, 2015

stale bot commented Dec 14, 2017

stale bot commented Mar 26, 2018

stale bot commented Feb 21, 2020

Compatibility for Spin Image features #1260

Are you sure you want to change the base?

Compatibility for Spin Image features #1260

Conversation

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

kd-tree search.

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

jgarstka commented Jun 15, 2015

jgarstka commented Jun 15, 2015

andersgb1 commented Jun 15, 2015

jspricke commented Oct 14, 2015

taketwo commented Oct 16, 2015

jspricke commented Oct 19, 2015

taketwo commented Oct 19, 2015

andersgb1 commented Oct 19, 2015

taketwo commented Nov 16, 2015

stale bot commented Dec 14, 2017

stale bot commented Mar 26, 2018

stale bot commented Feb 21, 2020