Add support pcl::PointXYZRGBA to pcl::VLPGrabber #1738

UnaNancyOwen · 2016-10-20T12:15:39Z

Add support pcl::PointXYZRGBA to pcl::VLPGrabber.
This change will be able to attach color to point cloud using pcl::visualization::PointCloudColorHandlerRGBField.
It has already been supported in pcl::HDLGrabber.

UnaNancyOwen · 2016-10-24T18:25:35Z

@SergioRAgostinho
I think that this Pull Request should be included in PCL 1.8.1.
It is minor update for pcl::VLPGrabber that is included in PCL from PCL 1.8.0.

SergioRAgostinho · 2016-10-25T15:24:41Z

@UnaNancyOwen but it makes changes to the public API and it breaks the ABI for the VLPGrabber class :/

UnaNancyOwen · 2016-10-25T15:30:48Z

@SergioRAgostinho Okay, I understand.

SergioRAgostinho

I can see the usefulness of what you implemented, but I don't like the fact that the class is populating pointclouds of all types it supports, without taking into account which callbacks are actually registered. Your functionality extends this behaviour even more.

So before integrating this new feature, I would prefer to look into how to correct what I just described, and then add your functionality on top, with it just being active if I register a callback which request a pointcloud with color info.

SergioRAgostinho · 2017-07-02T01:49:00Z

io/include/pcl/io/vlp_grabber.h

    private:
+      pcl::RGB laser_rgb_mapping_[VLP_MAX_NUM_LASERS];


I have the impression that PCL's convention is to have members bellow methods.

This guy needs a brief comment on top explaining what this is.

SergioRAgostinho · 2017-07-02T01:56:34Z

io/include/pcl/io/vlp_grabber.h

+
+    protected:
+      static const int VLP_MAX_NUM_LASERS = 16;
+      static const int VLP_DUAL_MODE = 0x39;


Although you just moved these guys from the .cpp file, it would be nice to add a comment, to generate proper documentation about them on doxygen.

Why not both of uint8_t?

SergioRAgostinho · 2017-07-02T02:08:39Z

io/src/vlp_grabber.cpp

 {
-  double vlp16_vertical_corrections[] = { -15, 1, -13, 3, -11, 5, -9, 7, -7, 9, -5, 11, -3, 13, -1, 15
+  for (int i = 0; i < 8; i++)


Following up on the comments above. I assume the limit is half the VLP_MAX_NUM_LASERS

for (uint8_t i = 0; i < VLP_MAX_NUM_LASERS / 2u ; ++i)

SergioRAgostinho · 2017-07-02T02:10:19Z

io/src/vlp_grabber.cpp

+  {
+     laser_rgb_mapping_[i * 2].b = static_cast<uint8_t> (i * 6 + 64);
+     laser_rgb_mapping_[i * 2 + 1].b = static_cast<uint8_t> ((i + 16) * 6 + 64);
+  }


So... looking at this

union { union { struct { uint8_t b; uint8_t g; uint8_t r; uint8_t a; }; float rgb; }; uint32_t rgba; };

You're initialising the first element of b, which I have the impression that actually corresponds to the alpha component and not the blue one), because I think rgba is packed like |a|r|g|b| . I remember we had problems with floats NaN and the alfa component because of this.

0x40 0x00 0x00 0x00
0xA0 0x00 0x00 0x00

0x46 0x00 0x00 0x00
0xA6 0x00 0x00 0x00

0x4B 0x00 0x00 0x00
0xAB 0x00 0x00 0x00

0x52 0x00 0x00 0x00
0xB2 0x00 0x00 0x00

0x58 0x00 0x00 0x00
0xB8 0x00 0x00 0x00

0x5E 0x00 0x00 0x00
0xBE 0x00 0x00 0x00

...

Are you just not modifying the alpha component? Can you show me a visual results of this change you made?

It seems to be properly colored.

SergioRAgostinho · 2017-07-02T02:14:48Z

io/src/vlp_grabber.cpp

          current_sweep_xyzi_->header.seq = sweep_counter;

          sweep_counter++;

          HDLGrabber::fireCurrentSweep ();
        }
        current_sweep_xyz_.reset (new pcl::PointCloud<pcl::PointXYZ> ());
+        current_sweep_xyzrgb_.reset (new pcl::PointCloud<pcl::PointXYZRGBA> ());


Although you're just using this from the base class it's kind of sad it's called *_xyzrgb and then it takes a pointcloud XYZRGBA :(

In order to change name of this variable, It need to change base class.
I think It would be better to separate it into other pull requests.
What do you think?

BTW, Sorry for late reply.
I'm planning to start fix this pull request in a few days. Thanks,

SergioRAgostinho · 2017-07-02T02:28:30Z

io/include/pcl/io/vlp_grabber.h

+       */
+      void
+      setLaserColorRGB (const pcl::RGB& color,
+                        unsigned int laserNumber);


Can be uint8_t.

SergioRAgostinho · 2017-07-02T03:32:33Z

io/src/vlp_grabber.cpp

+/////////////////////////////////////////////////////////////////////////////
+void
+pcl::VLPGrabber::setLaserColorRGB (const pcl::RGB& color,
+                                   unsigned int laserNumber)


SergioRAgostinho · 2017-07-02T03:34:32Z

io/src/vlp_grabber.cpp

+      return;
+
+  laser_rgb_mapping_[laserNumber] = color;
+}


It's kinda limiting to only be able to set one color at the time on a 16 element array. Why not add something which allows copying an entire block?

Does that mean adding a function such as following?

// set one color to all lasers void pcl::VLPGrabber::setLaserColorRGB (const pcl::RGB& color) { for (uint8_t i = 0; i < VLP_MAX_NUM_LASERS; i++) { setLaserColorRGB (color, i); } }

or

// set colors array to lasers // note: in this case, this grabber need to provide api for the users to obtain the number of lasers. void pcl::VLPGrabber::setLaserColorRGB (const pcl::RGB (&colors)[VLP_MAX_NUM_LASERS]) { laser_rgb_mapping_ = colors; }

I think it should be added to the base class as well as this class.
These change would be better to separate it into other pull requests.

Does that mean adding a function such as following?

I meant the second one but if you want to be generic between HDL and VPL grabbers we should provide a signature which specifies iterators for the read beginning and end positions.

I think it should be added to the base class as well as this class.
These change would be better to separate it into other pull requests.

No need for the separate PR. Just add another commit providing the setters in HDL.

I meant the second one but if you want to be generic between HDL and VPL grabbers we should provide a signature which specifies iterators for the read beginning and end positions.

Can you show implementation examples?

No need for the separate PR. Just add another commit providing the setters in HDL.

I get it.

Something like the following would be ok for both base and derived classes but won't allow you to make a boundary check

// provide iteraror range template<typename IterT> void pcl::HDLGrabber::setLaserColorRGB (const IterT& begin, const IterT& end) { std::copy (begin, end, laser_rgb_mapping_); }

This needs to be implemented in the header because of the template ~and HDL will require it's own version due to the different name of the constant~~.

Another idea to allow boundary checking would be to rename these max num lasers constants to a single max_num_lasers, which is set on object construction, but this already involves a lot more changes...

UnaNancyOwen · 2017-09-12T18:25:07Z

@SergioRAgostinho I fixed part of pointed out in first review. Could you please confirm this changes?

I think following points would be better to separate it into other pull requests, because it need to change base class or it should be fixed to the base class as well as this class.

#1738 (comment)
#1738 (comment)

Thanks,

SergioRAgostinho · 2017-11-22T01:16:56Z

Can you rebase this again? I'll have another at it soon.

UnaNancyOwen · 2017-11-22T04:56:12Z

I squashed commits. Is there anything else to do?

SergioRAgostinho · 2017-11-22T11:03:39Z

I squashed commits. Is there anything else to do?

The idea was not to squash everything yet. It was just to rebase to the current tip of master. If you check the status of this PR it says I can't merge it because there's conflicting files. The rebase will fix that.

https://stackoverflow.com/questions/7929369/how-to-rebase-local-branch-with-remote-master/18442755#18442755

Add support pcl::PointXYZRGBA to pcl::VLPGrabber. This change will be able to attach color to point cloud using pcl::visualization::PointCloudColorHandlerRGBField.

Add set colors to lasers.

SergioRAgostinho · 2017-11-22T16:32:32Z

io/include/pcl/io/hdl_grabber.h

@@ -71,7 +71,7 @@ namespace pcl
       *         Represents 1 corrected packet from the HDL Velodyne.  Each laser has a different RGB
       */
      typedef void
-      (sig_cb_velodyne_hdl_scan_point_cloud_xyzrgb) (const boost::shared_ptr<const pcl::PointCloud<pcl::PointXYZRGBA> >&,
+      (sig_cb_velodyne_hdl_scan_point_cloud_xyzrgba) (const boost::shared_ptr<const pcl::PointCloud<pcl::PointXYZRGBA> >&,


These are public typedefs and need to be deprecated.

Does that mean these typedef definitions should change to protected (or private) access specifier?

The change is not in scope. The idea is to not break the public API while warning the users to update their code to the new signature type. You'll need to change it to something like this

typedef void (sig_cb_velodyne_hdl_scan_point_cloud_xyzrgba) (const boost::shared_ptr<const pcl::PointCloud<pcl::PointXYZRGBA> >&, float, float); typedef PCL_DEPRECATED ("Use 'sig_cb_velodyne_hdl_scan_point_cloud_xyzrgba' instead") sig_cb_velodyne_hdl_scan_point_cloud_xyzrgba sig_cb_velodyne_hdl_scan_point_cloud_xyzrgb;

So apparently there's no portable solution because the deprecation attribute for typedefs is placed in different places depending on the compiler, so you'll need to safeguard against msvc and (gcc/clang). More info here https://stackoverflow.com/questions/4995868/deprecate-typedef

Add PCL_DEPRECATED

SergioRAgostinho · 2017-11-22T16:32:49Z

io/include/pcl/io/hdl_grabber.h

@@ -98,7 +98,7 @@ namespace pcl
       *         This signal is sent when the Velodyne passes angle "0".  Each laser has a different RGB
       */
      typedef void
-      (sig_cb_velodyne_hdl_sweep_point_cloud_xyzrgb) (const boost::shared_ptr<const pcl::PointCloud<pcl::PointXYZRGBA> >&);
+      (sig_cb_velodyne_hdl_sweep_point_cloud_xyzrgba) (const boost::shared_ptr<const pcl::PointCloud<pcl::PointXYZRGBA> >&);


Same comment as above.

Add PCL_DEPRECATED

SergioRAgostinho · 2017-11-22T16:34:41Z

io/include/pcl/io/hdl_grabber.h

@@ -152,12 +152,17 @@ namespace pcl
      filterPackets (const boost::asio::ip::address& ipAddress,
                     const unsigned short port = 443);

-      /** \brief Allows one to customize the colors used for each of the lasers.
+      /** \brief Allows one to customize the colors used for each number of the lasers.


Switch to Allows one to customize the colors used by each laser..

Add the documentation for the input parameter.

SergioRAgostinho · 2017-11-22T16:35:45Z

io/include/pcl/io/vlp_grabber.h

@@ -80,13 +80,34 @@ namespace pcl
      virtual std::string
      getName () const;

+      /** \brief Allows one to customize the colors used for each of the lasers. */


Method description similar to the one before. Add doc regarding input parameters.

SergioRAgostinho · 2017-11-22T16:41:32Z

io/include/pcl/io/vlp_grabber.h

+      /** \brief Allows one to customize the colors used for each of the lasers. */
+      void
+      setLaserColorRGB (const pcl::RGB& color,
+                        uint8_t laserNumber);


You normally wouldn't need to redefine this method, but this inheritance scheme is completely broken.

This declaration is correct, because the definition is different from base class. (laser number check)

void pcl::HDLGrabber::setLaserColorRGB (const pcl::RGB& color, uint8_t laserNumber) { if (laserNumber >= HDL_MAX_NUM_LASERS) return; laser_rgb_mapping_[laserNumber] = color; }

void pcl::VLPGrabber::setLaserColorRGB (const pcl::RGB& color, uint8_t laserNumber) { if (laserNumber >= VLP_MAX_NUM_LASERS) return; laser_rgb_mapping_[laserNumber] = color; }

You're right 👍

SergioRAgostinho · 2017-11-22T16:43:25Z

io/include/pcl/io/vlp_grabber.h

+      /** \brief Allows one to customize the colors used for each number of the lasers. */
+      void
+      setLaserColorRGB (const pcl::RGB& color,
+                        unsigned int laserNumber);


Doc parameters

Why is this method defined in private scope?

Sorry, It is mistake.

SergioRAgostinho · 2017-11-22T16:43:35Z

io/include/pcl/io/vlp_grabber.h

+
+      /** \brief Allows one to customize the colors used for each of the lasers. */
+      void
+      setLaserColorRGB (const pcl::RGB (&colors)[VLP_MAX_NUM_LASERS]);


Same comment here

Fix Add set colors to lasers.

SergioRAgostinho · 2017-11-22T18:12:47Z

io/include/pcl/io/hdl_grabber.h

@@ -189,6 +201,7 @@ namespace pcl
      static const int HDL_LASER_PER_FIRING = 32;
      static const int HDL_MAX_NUM_LASERS = 64;
      static const int HDL_FIRING_PER_PKT = 12;
+      pcl::RGB laser_rgb_mapping_[HDL_MAX_NUM_LASERS];


Even though you're moving this to protected scope, which is correct, remember that VLPGrabber has its own laser_rgb_mapping_ member, which overshadows this one.

SergioRAgostinho · 2017-11-22T18:17:23Z

io/include/pcl/io/hdl_grabber.h

+      setLaserColorRGB( const IterT& begin, const IterT& end )
+      {
+          std::copy( begin, end, laser_rgb_mapping_ );
+      }


Spacing in the parenthesis in the functions/methods

Fix Space

Deprecates old typedefs, because changed to xyzrgba from xyzrgb.

Update examples to support VTK >= 7.1

Suppress strict alias warning

Add search path suffixes for PCL port of Vcpkg.

Add support pcl::PointXYZRGBA to pcl::VLPGrabber. This change will be able to attach color to point cloud using pcl::visualization::PointCloudColorHandlerRGBField.

UnaNancyOwen · 2017-11-24T12:51:26Z

Sorry, I tried to organize commits, but I couldn't do it successfully.
I will send a new pull request because I'm not familiar with git operation.

SergioRAgostinho · 2017-11-24T13:26:51Z

In case you want to read more about it, I always find this post very useful and intuitive.
https://www.atlassian.com/git/tutorials/merging-vs-rebasing

UnaNancyOwen · 2017-11-24T13:35:50Z

@SergioRAgostinho Thank you telling me the documents about it! 👍

SergioRAgostinho added this to the pcl-1.9.0 milestone Oct 24, 2016

SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Oct 24, 2016

SergioRAgostinho requested changes Jul 2, 2017

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jul 2, 2017

SergioRAgostinho added needs: code review Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Nov 7, 2017

UnaNancyOwen force-pushed the fix_grabber branch from 8c6145e to 1515a0f Compare November 22, 2017 04:53

SergioRAgostinho added changelog: API break Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Nov 22, 2017

Add support pcl::PointXYZRGBA to pcl::VLPGrabber

0dc4b89

Add support pcl::PointXYZRGBA to pcl::VLPGrabber. This change will be able to attach color to point cloud using pcl::visualization::PointCloudColorHandlerRGBField.

UnaNancyOwen force-pushed the fix_grabber branch from 1515a0f to 0dc4b89 Compare November 22, 2017 13:09

UnaNancyOwen added 2 commits November 22, 2017 22:21

Merge branch 'master' into fix_grabber

3890526

Add set colors to lasers

01a0580

Add set colors to lasers.

SergioRAgostinho requested changes Nov 22, 2017

View reviewed changes

SergioRAgostinho mentioned this pull request Nov 22, 2017

Revise inheritance between HDL and VPL grabbers. #2098

Open

Fix Add set colors to lasers

9a1fd94

Fix Add set colors to lasers.

SergioRAgostinho reviewed Nov 22, 2017

View reviewed changes

UnaNancyOwen added 2 commits November 23, 2017 05:25

Fix Space

247b9a7

Fix Space

Deprecated old typedefs

b2c6171

Deprecates old typedefs, because changed to xyzrgba from xyzrgb.

yongduek and others added 23 commits November 24, 2017 21:34

VTK function change since version 7.1 (PointCloudLibrary#2063)

3fe2dd5

Update examples to support VTK >= 7.1

Removed unused member from ply_parser

fb39b13

Suppress if unused parameter warnings

664bd29

Suppress sign compare warnings

57a26df

Remove unused variable from octree_viewer

de00778

Removal of PCA deprecated constructor

2dc19fc

Remove deprecated functions and variables from SAC module

59fcebd

Suppress (maybe) uninitialized warning

83b8dfe

Suppress unused parameter warning

04e26cb

Suppress strict alias warning (PointCloudLibrary#2072)

0dd1d06

Suppress strict alias warning

Remove registration module deprecated methods

3739b90

Remove deprecated ros headers

b1be693

Remove deprecated methods in surface module

51cadfe

Remove deprecated methods in filters module

a836751

Remove deprecated methods in io module

956c468

Remove deprecated methods in visualization module

b0c5e01

fix header names

280259f

More warning suppresion in pcl apps

3fe588d

Add search path suffixes for Vcpkg

9ab6359

Add search path suffixes for PCL port of Vcpkg.

Suppress the final set of warnings in pcl apps

1a399eb

[c++] replace auto_ptr with scoped_ptr (PointCloudLibrary#2037)

4a700f3

Add support pcl::PointXYZRGBA to pcl::VLPGrabber

870f6ed

Add support pcl::PointXYZRGBA to pcl::VLPGrabber. This change will be able to attach color to point cloud using pcl::visualization::PointCloudColorHandlerRGBField.

Merge remote-tracking branch 'origin/fix_grabber' into fix_grabber

2e0b9fb

UnaNancyOwen closed this Nov 24, 2017

SergioRAgostinho removed the status: ready to merge label Nov 24, 2017

UnaNancyOwen deleted the fix_grabber branch November 24, 2017 12:59

UnaNancyOwen mentioned this pull request Nov 24, 2017

Add support pcl::PointXYZRGBA to pcl::VLPGrabber. Deprecate rgb signatures. #2102

Merged

Add support pcl::PointXYZRGBA to pcl::VLPGrabber #1738

Add support pcl::PointXYZRGBA to pcl::VLPGrabber #1738

Conversation

UnaNancyOwen commented Oct 20, 2016

UnaNancyOwen commented Oct 24, 2016

SergioRAgostinho commented Oct 25, 2016

UnaNancyOwen commented Oct 25, 2016

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UnaNancyOwen Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

UnaNancyOwen commented Sep 12, 2017 • edited Loading

SergioRAgostinho commented Nov 22, 2017

UnaNancyOwen commented Nov 22, 2017

SergioRAgostinho commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UnaNancyOwen commented Nov 24, 2017

SergioRAgostinho commented Nov 24, 2017

UnaNancyOwen commented Nov 24, 2017

UnaNancyOwen Sep 1, 2017 •

edited

Loading

SergioRAgostinho Nov 22, 2017 •

edited

Loading

UnaNancyOwen commented Sep 12, 2017 •

edited

Loading