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 MAKE_ORGANIZED parameter #789

Merged
merged 1 commit into from Jul 2, 2018
Merged

Add MAKE_ORGANIZED parameter #789

merged 1 commit into from Jul 2, 2018

Conversation

@karnikram
Copy link
Contributor

karnikram commented Jun 23, 2018

PR Description

  • Added MAKE_ORGANIZED parameter to T3DPointsProjectionParams to be able to generate an organized pcl point cloud from a 2D range image
  • Appropriately set pcl cloud dimensions
  • More of an "ad-hoc" solution, just to get pcl::IntegralNormalEstimation to work for my gsoc project. Please let me know if I need to make more changes.

I acknowledge to have:

(Notify: @MRPT/owners )

Copy link
Member

jlblancoc left a comment

Good! But please, take a look at my review comments. If possible, please do a git rebase so the changes all appear as one single commit (the mrpt git tree is quite large already...)

@@ -1200,6 +1200,10 @@ class PointCloudAdapter<mrpt::maps::CPointsMap>
inline size_t size() const { return m_obj.size(); }
/** Set number of points (to uninitialized values) */
inline void resize(const size_t N) { m_obj.resize(N); }
/** Does nothing as of now */
inline void setDimensions(const size_t& height, const size_t& width)

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

I think this method does NOT need to be added here... it's enough to have the empty setDimensions() method in the PointCloudAdapter specialization for all MRPT classes, and only make it to have a real effect in the PCL specialization, right?

This comment has been minimized.

Copy link
@karnikram

karnikram Jun 24, 2018

Author Contributor

I had to add the method to other specializations too because project3DPointsFromDepthImageInto gets instantiated using the non-pcl types too. For instance, here.

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

because project3DPointsFromDepthImageInto gets instantiated using the non-pcl types too.

Sure... but if I recall it right, it internally instantiates a PointCloudAdapter (PCA) of the required target type as template argument. Then, my point is: shouldn't it be setDimensions() a method of the PCA instead of the base, target types? In this way, only one PCA should be modified to add a dummy (empty) setDimensions() method, the version for all MRPT classes. Only the specialization for PCL should have a setDimensions() method with real stuff inside.

Let me know if this makes now sense to you...

This comment has been minimized.

Copy link
@karnikram

karnikram Jun 24, 2018

Author Contributor

Yes, that makes sense. I did try to do that at first, but I couldn't actually find the definition for the class template PointCloudAdapter adapter anywhere... The class declaration is here, but only specialization specific definitions have been written, I think.

Or maybe I'm wrong..

This comment has been minimized.

Copy link
@karnikram

karnikram Jun 24, 2018

Author Contributor

Should I try to write the base template definition by myself?

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

Nope! It's done like that to avoid the need for an actual base virtual class. In this way, all methods are perfectly known at build time, and can even be inlined (typically).

Please, just add the new method to the specializations (I may have missed some, please do a search):

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 26, 2018

Member

Update: my fault. I didn't realize you added the methods to PointCloudAdapter<mrpt::maps::CPointsMap>, it seemed to me that the method was added to CPointsMap directly! So, it's good!

@@ -177,6 +177,10 @@ class PointCloudAdapter<mrpt::maps::CSimplePointsMap>
inline size_t size() const { return m_obj.size(); }
/** Set number of points (to uninitialized values) */
inline void resize(const size_t N) { m_obj.resize(N); }
/** Does nothing as of now */
inline void setDimensions(const size_t& height, const size_t& width)

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

same here...

T3DPointsProjectionParams()
: takeIntoAccountSensorPoseOnRobot(false),
robotPoseInTheWorld(nullptr),
PROJ3D_USE_LUT(true),
USE_SSE2(true),
MAKE_DENSE(true)
MAKE_DENSE(true),
MAKE_ORGANIZED(false)

This comment has been minimized.

Copy link
@jlblancoc

This comment has been minimized.

Copy link
@karnikram

karnikram Jun 24, 2018

Author Contributor

Forgot to add this,
MAKE_DENSE does not actually preserve the "organization" of the point cloud. It is only used to control whether or not the points can have nan / undefined values. If MAKE_DENSE is true, the cloud simply cannot contain undefined points and all the points contain finite values.

@@ -828,6 +832,11 @@ class PointCloudAdapter<mrpt::obs::CObservation3DRangeScan>
if (N) m_obj.hasPoints3D = true;
m_obj.resizePoints3DVectors(N);
}
/** Does nothing as of now */
inline void setDimensions(const size_t& height, const size_t& width)

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

same here...

@@ -334,6 +334,10 @@ class PointCloudAdapter<mrpt::opengl::CPointCloud>
inline size_t size() const { return m_obj.size(); }
/** Set number of points (to uninitialized values) */
inline void resize(const size_t N) { m_obj.resize(N); }
/** Does nothing as of now */
inline void setDimensions(const size_t& height, const size_t& width)

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

same here...

@@ -299,6 +299,10 @@ class PointCloudAdapter<mrpt::opengl::CPointCloudColoured>
inline size_t size() const { return m_obj.size(); }
/** Set number of points (to uninitialized values) */
inline void resize(const size_t N) { m_obj.resize(N); }
/** Does nothing as of now */
inline void setDimensions(const size_t& height, const size_t& width)

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

same here...

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 24, 2018

Member

ah! Please, add this change to the docs/doxygen-pages/changelog.h list of changes...

This comment has been minimized.

Copy link
@karnikram

karnikram Jun 24, 2018

Author Contributor

Made the changes you requested!
And did a git commit --amend, to make it one commit. Hope that's okay.

@karnikram karnikram force-pushed the karnikram:master branch 2 times, most recently from b5267af to e8df091 Jun 24, 2018
@@ -11,6 +11,9 @@

#include <mrpt/core/round.h> // round()

#include <pcl/point_cloud.h>
#include <pcl/point_types.h>

This comment has been minimized.

Copy link
@jlblancoc

jlblancoc Jun 26, 2018

Member

Ey! Red alarm here! :-)

We can't assume PCL is installed, since PCL is an optional dependency for MRPT.
I think you could remove those headers and all should build nice. Why did you add them?
For the templates for build, it's enough to have forward declarations, and in any case, the PCL headers included in the user code (the caller that includes this file).

So, please, remove those two includes.

This comment has been minimized.

Copy link
@karnikram

karnikram Jun 26, 2018

Author Contributor

Oops, sorry. Removed those two includes and checked the build. No issues.
Pushed.

Copy link
Member

jlblancoc left a comment

Once the PCL includes are removed, this can be merged.

@karnikram karnikram force-pushed the karnikram:master branch from e8df091 to 012c399 Jun 26, 2018
* Added MAKE_ORGANIZED parameter to T3DPointsProjectionParams to be able to generate an organized pcl point cloud from a 2D range image
* Appropriately set pcl cloud dimensions
@karnikram karnikram force-pushed the karnikram:master branch from 012c399 to 82110e8 Jun 26, 2018
@jlblancoc

This comment has been minimized.

Copy link
Member

jlblancoc commented Jun 26, 2018

Good. Let's wait for the travis result...

@jlblancoc

This comment has been minimized.

Copy link
Member

jlblancoc commented Jun 29, 2018

@jolting : Please, look at this...

Error response from daemon: Get https://registry-1.docker.io/v2/: unauthorized: incorrect username or password

I think we didn't change anything in the docker area (?).

On the AppVeyor build, it seems it timed out (sigh)...

@jlblancoc jlblancoc merged commit 1f8f3ac into MRPT:master Jul 2, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@jolting

This comment has been minimized.

Copy link
Member

jolting commented Jul 2, 2018

@jlblancoc Travis isn't passing in env variables to PR. That's a setting on travis. I'm not so worried about leaking docker hub credentials in this way because that account is only used for builds. It's just a risk if we enable builds for PR from outside of the main repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.