Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add ellipsoid shape to pcl_visualizer #4531

Merged
merged 7 commits into from
Apr 8, 2021

Conversation

Yosshi999
Copy link
Contributor

Implemented addEllipsoid

Visualization sample: visualize the voxel grid covariances for Bunny
Screenshot from 2020-12-02 16-52-50

@mvieth mvieth added changelog: new feature Meta-information for changelog generation module: visualization needs: code review Specify why not closed/merged yet labels Dec 2, 2020
Copy link
Contributor

@JStech JStech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!
I am not totally sure that the order in which translation and rotation are applied is the order one would expect. Perhaps using Eigen::Isometry3f instead of translation and rotation would leave less room for interpretation and make it less ambiguous?

visualization/include/pcl/visualization/common/shapes.h Outdated Show resolved Hide resolved
@Yosshi999
Copy link
Contributor Author

Yosshi999 commented Dec 3, 2020

According to the documentation,

Most of the methods for manipulating this transformation, e.g. Translate, Rotate, and Concatenate, can operate in either PreMultiply (the default) or PostMultiply mode. In PreMultiply mode, the translation, concatenation, etc. will occur before any transformations which are represented by the current matrix. In PostMultiply mode, the additional transformation will occur after any transformations represented by the current matrix.

the vtk's default mode is PreMultiply, so I think its order is common (I mean, vtk will perform rotation and then translation.)
But it is good idea to use Isometry3f for its arguments.

(Edit)
Or it may be less ambiguous if "orientation" is used instead of "rotation" for the name of that argument. What do you think about this?

@Yosshi999 Yosshi999 requested a review from mvieth March 31, 2021 07:58
@mvieth
Copy link
Member

mvieth commented Mar 31, 2021

Thanks for pinging me.
I am still not so sure about translation+rotation vs Eigen::Isometry. If a user calls createEllipsoid (or addEllipsoid, which calls createEllipsoid), it might not be clear whether the translation is applied first or the rotation. I think usually the rotation would be applied first, then the translation, but here it is the other way around, right?
So if there are only versions of createEllipsoid and addEllipsoid that receive an Isometry transformation as a parameter, that would IMO be totally unambiguous. Internally, you could get the 4x4 transformation matrix from the Eigen Isometry object with matrix(), or maybe with data(), and use the SetMatrix function of vtkTransform (possibly with some conversion steps in between). What do you think?

@Yosshi999
Copy link
Contributor Author

@mvieth
At first I tried to imitate the arguments of addCube, but I agree with your idea. I'll remove translation+rotation version.

Internally, you could get the 4x4 transformation matrix from the Eigen Isometry object with matrix(), or maybe with data(), and use the SetMatrix function of vtkTransform

I will check it, thank you!

@Yosshi999
Copy link
Contributor Author

Undefined symbols for architecture x86_64:
  "vtkParametricEllipsoid::New()", referenced from:
      pcl::visualization::createEllipsoid(Eigen::Transform<double, 3, 1, 0> const&, double, double, double) in shapes.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/libpcl_visualization.1.11.1.99.dylib] Error 1
make[1]: *** [visualization/CMakeFiles/pcl_visualization.dir/all] Error 2

I have no idea to resolve this...

@larshg
Copy link
Contributor

larshg commented Apr 3, 2021

For VTK9 you have to link more specific libraries, as all is not included by default.

You'll have to add CommonComputationalGeometry to the list of required libraries in CMakeList of visualization.

@Yosshi999 Yosshi999 force-pushed the visualizer_add_ellipsoid branch from 9a4efe9 to 22acc46 Compare April 3, 2021 08:21
@larshg
Copy link
Contributor

larshg commented Apr 3, 2021

You need to add it here as well.

target_link_libraries("${LIB_NAME}"
VTK::ChartsCore
VTK::CommonColor

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM (Though I'm wondering if VTK also has modules which might be non-default on some platforms and we can accidentally break them by adding a dependency in PCL)

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
@kunaltyagi Regarding the possible VTK dependency problem: not really much we can do about that, right? Just waiting and seeing if someone reports a problem

@kunaltyagi
Copy link
Member

That's correct. Just pointing out something that stood out to me (apart from the PR, nice work @Yosshi999 )

@kunaltyagi kunaltyagi removed the needs: code review Specify why not closed/merged yet label Apr 8, 2021
@mvieth mvieth merged commit e1664a4 into PointCloudLibrary:master Apr 8, 2021
mvieth pushed a commit to mvieth/pcl that referenced this pull request Dec 27, 2021
* add ellipsoid shape to pcl_visualizer

* more accurate description

* addEllipsoid with Isometry3f

* use const

* support only Isometry3d argument

* add a component for ellipsoid

* add a link library for ellipsoid
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: new feature Meta-information for changelog generation module: visualization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants