Add VREP conversion methods for MRPT class objects by shubham-kumar1410 · Pull Request #13 · MRPT/mrpt_vrep_bridge

shubham-kumar1410 · 2018-05-27T14:26:52Z

Added methods (Issue #3)

Add conversion method and gtest for CObservation2DRangeScan class
Add conversion method and gtest for CPose3D class
Add conversion method and gtest for CObservationOdometry class

bergercookie

Left some comments in various parts of the code.
As general comments, also note the following:

Add some instructions in this PR of how are we supposed to test the functionality of this code.
Please, please, don't ignore compiler warnings! This time you could have caught the bug if you had dealt with it

As general comments, with regards to the style of this PR, obviously its not a major thing as we'll be adding automatic formatting using clang-format, but still I think the following is useful to know:

Don't add trailing spaces at the end of lines. You might not even be aware of that as they may not be visible in your editor, but when using tools like git-diff they are irritating to see. Also, modifiying them also marks the line as modified and thus it appears as part of the commit.
Watch your indentation.

Depending on the editor that you use I'm pretty sure there are options for both showing the trailing white spaces (or even automatically trimming them) and handling the indentation correctly. See MRPT CONTRIBUTING.md for the indentation rules.

bergercookie · 2018-05-27T14:34:11Z

-  set(test_sources "test/runTests.cpp")
-  add_executable(${PROJECT_TEST_NAME}
-    ${test_sources})
+  set(test_sources


The V-REP related files should be compiled in a library of their own. Also do that in the main CMakeLists file so that all the MRPT algorithms can make use of it.

Also watch the indentation here, seems to be one space off.

bergercookie · 2018-05-27T14:34:49Z

-  set (SOURCES 
+  set (SOURCES
+  	include/mrpt_bridge/vrep_conversion.h 
    ${VREP_PATH}/remoteApi/extApi.h


Watch the indentation here again.
Isn't there a setting in your editor for adding the correct number of spaces/tabs automatically?

bergercookie · 2018-05-27T14:35:55Z

+    remoteApi/vrep_conversion.cpp)

-  include_directories (${VREP_PATH}/remoteApi ${VREP_PATH}/include)
+  include_directories (${VREP_PATH}/remoteApi ${VREP_PATH}/include include)


The V-REP related sources/libraries.. should be handled in the main CMakeLists.txt file

bergercookie · 2018-05-27T14:36:38Z

+}
+}
+namespace vrep_bridge
+{ bool convert(const float _range[], const simxFloat& _maxScanDistance,


Document these methods with doxygen.

Add method, parameters [in, out] and return type descriptions to them etc.

bergercookie · 2018-05-27T14:37:35Z

@@ -0,0 +1,36 @@
+#ifndef VREP_CONVERSION_H


MRPT <-> V-REP conversions should be added to the conversions subdirectory instead of the mrpt_graphslam_2d/include/mrpt_bridge. Remember that they may be by other MRPT libraries other than mrpt_graphslam_2d. Same goes for the vrep_conversions.cpp. Why is the latter under mrpt_graphslam_2d/remoteApi/?

bergercookie · 2018-05-27T16:02:43Z

+    CObservation2DRangeScan obj;
+    EXPECT_TRUE(convert(range,maxScanDistance,scanningAngle,sensor_pose,obj));
+    EXPECT_TRUE(obj.rightToLeft);
+	EXPECT_EQ(obj.aperture, scanningAngle);


As pointed out by @maxchaos comparing float/double values with EXPECT_EQ is strongly discouraged. googletest also points this out in their Advanced Guide

bergercookie · 2018-05-27T16:04:37Z

+
+TEST(ConvertTest,CPose3DTest){
+	simxFloat position[3] ={1.00,1.00,1.00};
+	simxFloat quaternion[4] = {0.5,0.5,0.5,0.5};


This is not enough data to be sure about this functionality. Have at least ... 3 different cases. Also you aren't actually testing the expected results in the CPose3D but only its pitch value!

bergercookie · 2018-05-27T16:05:20Z

+	simxFloat position[3] ={1.00,1.00,1.00};
+	simxFloat quaternion[4] = {0.5,0.5,0.5,0.5};
+	CPose3D pose;
+	EXPECT_TRUE(convert(position,quaternion,pose));


This isn't a helpful asserrtion; your convert methods always return true anyway.

bergercookie · 2018-05-27T16:06:27Z

+	EXPECT_EQ((double)pitch,pose.pitch());
+}
+
+TEST(ConvertTest,CObservationOdometryTest){


Minor style thing, but be consistent of where to place the bracket ... is it going to be at the end of the line or the beginning of the next?

bergercookie · 2018-05-27T16:07:09Z

+	EXPECT_EQ(obj.sensorPose,sensor_pose);
+}
+
+TEST(ConvertTest,CPose3DTest){


Add simple docstrings of what these tests do

bergercookie · 2018-05-27T16:48:58Z

+namespace mrpt
+{
+namespace obs
+{


Given the C++17 PR you can now use nested namespaces to cleanup such parts of code

cmake: Check minimum compiler version - Bump to C++17 standard

jlblancoc · 2018-05-28T23:51:49Z

+	CPose3D pose;
+	EXPECT_TRUE(convert(position,quaternion,pose));
+	float pitch = asin(-2*((quaternion[0]*quaternion[2])-(quaternion[1]*quaternion[3])));
+	EXPECT_EQ((double)pitch,pose.pitch());


EXPECT_EQ() should not be used with floating numbers...
Please, take a look at: https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#floating-point-comparison

I have taken care of that in my new commit

bergercookie · 2018-05-29T21:03:04Z

+using namespace vrep_bridge;
+
+/**
+*This unit test tests the CObservation2DRangeScan conversion method. It tests the following parameters of the object created with the functions parameters.


bergercookie · 2018-05-29T21:08:40Z

+namespace vrep_bridge
+{   /**
+    *This method converts VREP laser scans into a CObservation2DRangeScan class object. It has the following arguments:
+    *- <b>_range[]</b> -> This array contains all the laser scan measurements(distances).


No need for bold there..

Use @param [in] or @param[out] here instead

https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdparam

bergercookie · 2018-05-29T21:10:03Z

+        return true;
+    }
+    /**
+    *This method converts pose,velocity and angular velocity value of an object into a CObservationOdometry class object. It has the following arguments:


Very very minor but.. no need to repeat the This method .... Start with the verb instead..

Convert sensor position...

bergercookie · 2018-05-29T21:13:11Z

@@ -0,0 +1,26 @@
+#ifndef VREP_CONVERSION_H


Why do we have an mrpt_bridge subdirectory?
Why not put the vrep_conversions directly under include?

bergercookie · 2018-05-29T21:13:35Z

+#ifndef VREP_CONVERSION_H
+#define VREP_CONVERSION_H
+
+#include <stdint.h>


Use C++ style includes

bergercookie · 2018-05-29T21:19:16Z

+*
+*/
+TEST(ConvertTest, CObservation2DRangeScanTest)
+{


You should be checking the actual data of the laser scan.

Grab an actual simulated laser scan from V-REP..

Write its contents to an e.g., textfile (or just dump them in the cpp file for now. Then in this test function read them back, convert them to a CObservation2DRangeScan and check that the data of the latter object is still there as it should be...

This may require some tinkering with capturing the data and reading /writing that text file but it is the most definitive way moving forward to make sure that the convert method produces the expected results...

Refer to this as an example: https://github.com/MRPT/mrpt/blob/master/samples/slam_icp_simple_example/test.cpp

Let me know if you need a more detailed explanation on this

bergercookie · 2018-06-04T17:57:06Z

-#include <mrpt/obs/CObservationOdometry.h>
-#include <mrpt/poses/CPose3D.h>
-#include "../../conversions/include/mrpt_bridge/vrep_conversion.h"
+#include "mrpt/obs/CObservation2DRangeScan.h"


Any reason you switch from <...> to "..."?

I was following google style guidelines. Let me know if I need to revert back.

bergercookie · 2018-06-04T17:58:05Z

+		0.446379f ,0.446323f ,0.446342f ,0.443543f ,0.444175f ,0.444877f ,0.442981f ,0.444239f ,0.445565f ,0.444401f ,
+		0.446282f ,0.446131f ,0.448157f ,0.44866f ,0.451289f ,0.452373f ,0.455437f ,0.457836f ,0.460719f ,0.465214f ,
+		0.471546f ,0.485626f ,	
+	};


Have you tested that these values are written correctly or are you still working on this?

Yes these values were received from vrep and they have been tested

No, that's not what I mean. I want to have the values, one by one checked whether they are written correctly in the CObservation2DRangeScan object.

Loop through that array and check each value, one by one.

bergercookie · 2018-06-06T15:07:08Z

-    *- <b>_position[]</b> -> This array contains x,y,z coordinates of the sensor.
-    *- <b>_quaternion</b> -> Contains the quaternion vector of the sensor.
-    *
+    *Converts sensor position,quaternion into a CPose3D class object. It has the following arguments:


Minor but don't stick the letters next docstring asterisks

bergercookie · 2018-06-06T15:08:14Z

+		0.446379f ,0.446323f ,0.446342f ,0.443543f ,0.444175f ,0.444877f ,0.442981f ,0.444239f ,0.445565f ,0.444401f ,
+		0.446282f ,0.446131f ,0.448157f ,0.44866f ,0.451289f ,0.452373f ,0.455437f ,0.457836f ,0.460719f ,0.465214f ,
+		0.471546f ,0.485626f ,	
+	};


No, that's not what I mean. I want to have the values, one by one checked whether they are written correctly in the CObservation2DRangeScan object.

Loop through that array and check each value, one by one.

bergercookie · 2018-06-06T15:18:59Z

+
+namespace vrep_bridge
+{   /**
+    *Converts VREP laser scans into a CObservation2DRangeScan class object. It has the following arguments:


Just noticed this.

The doxygen comments should be put in the declaration of the methods i.e., in the .h file

bergercookie · 2018-06-06T15:24:59Z

Hey @shubham-kumar1410 you still haven't addressed some comments of mine..
Check the outdated (as marked by github) comments as some of them are still relevant.

shubham-kumar1410 · 2018-06-06T17:28:16Z

Hi @bergercookie , I have pushed a commit with what I believe are all the issues addressed by you. Kindly let me know if I missed something.

bergercookie · 2018-06-08T11:33:52Z

Hi @bergercookie , I have pushed a commit with what I believe are all the issues addressed by you. Kindly let me know if I missed something.

You have added the vrep shared library in the main CMakeLists.txt file. Now link against it in the conversions and graphslam_2d CMakeLists.txt files as well instead of readding the SOURCES files
Don't do this: #include <stdio.h>. Instead do this #include <cstdio>.

And that is for every one of the includes not just stdio.

include_directories (${VREP_PATH}/remoteApi ${VREP_PATH}/include include)

You just have to do this once in the main CMakeLists.txt file

shubham-kumar1410 · 2018-06-08T12:34:37Z

I have linked the vrep shared lib with the graphslam_2d CMakeLists.txt. And also have changed the header styles. Kindly have a look

shubham-kumar1410 · 2018-06-08T13:03:37Z

Let me know if there are any other changes. @bergercookie

shubham-kumar1410 added 4 commits May 25, 2018 17:36

Add converstion from VREP laserscans to CObservation2DRangeScan

6721dc6

Add gtest for CObservation2DRangeScan conversion method

8a9a758

Add conversion method for CPose3D class

9b8c5de

Add conversion method for CObservationOdometry class

2c4a8a5

bergercookie requested changes May 27, 2018

View reviewed changes

bergercookie mentioned this pull request May 27, 2018

Port Existing MRPT algorithms for V-REP simulator (shubham-kumar1410) MRPT/GSoC2018-discussions#3

Open

cmake: Check minimum compiler version - Bump to C++17 standard

4df37d1

bergercookie reviewed May 27, 2018

View reviewed changes

shubham-kumar1410 added 2 commits May 27, 2018 23:32

Merge pull request #1 from MRPT/minor-cmake-additions

5ad3a13

cmake: Check minimum compiler version - Bump to C++17 standard

Add documentation for conversion methods and bug fix

868434d

jlblancoc reviewed May 28, 2018

View reviewed changes

Remove variable prefix

d461cad

bergercookie requested changes May 29, 2018

View reviewed changes

Test the convert functions to data fetched from VREP

6eef707

bergercookie requested changes Jun 4, 2018

View reviewed changes

bergercookie reviewed Jun 6, 2018

View reviewed changes

Add additionaltests for conversion method

56b7818

Add vrep shared library code

d25b982

Change header style

9aa289b

Conversation

shubham-kumar1410 commented May 27, 2018

Added methods (Issue #3)

Uh oh!

bergercookie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham-kumar1410 Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergercookie commented Jun 6, 2018

Uh oh!

shubham-kumar1410 commented Jun 6, 2018

Uh oh!

bergercookie commented Jun 8, 2018

Uh oh!

shubham-kumar1410 commented Jun 8, 2018

Uh oh!

shubham-kumar1410 commented Jun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

shubham-kumar1410 Jun 4, 2018 •

edited

Loading