Appending points to PCD and Out-of-core Octree modification to use append #325

juagargi · 2013-10-15T13:11:45Z

The PCD writer now writes the width and number of points using 10 digits, so the header part of the PCD file is fixed.
There is a new call to append data to a PCD file.
The out-of-core octree has a specialization that is able to append data, instead of creating new point clouds and compressing them to disk.

jspricke · 2013-10-26T15:07:52Z

Hi @juagargi, thanks for your contribution. Could you clean up the patches a little, so we can review them easier? Have a look at git rebase -i for it. Also, please don't change the indention in unrelated files (as in outofcore/tools/outofcore_print.cpp).

jspricke · 2013-10-28T19:39:03Z

Hi @juagargi, could you give some justification why you made use_appender a template parameter instead of a parameter to the constructor, for example?

juagargi · 2013-10-29T09:09:35Z

Hi @jspricke , use_appender is a template argument because that decision is taken in compile time: one can choose to create and use the regular tree, or the one that appends data to the files. If it was a run-time argument, it'd look like you can modify its behavior during the life of the object.

jspricke · 2013-10-29T18:40:43Z

Is there any reason why you can't do it at runtime?

juagargi · 2013-10-30T10:27:47Z

Nono, let me explain my philosophy for this: that decision is taken at compile time because I believe once you've created a tree, you don't want to change it's writing mode.
If you find there are cases where we want to change the writing mode for a given tree at runtime, that parameter will have to be checked at runtime.

jspricke · 2013-11-08T10:51:14Z

@juagargi My point is, if we just add a runtime function and a default parameter, we can leave the current API in place. Also, we could make the number of zeros an argument. I see this more like a reserve() in std::vector, what do you think?

juagargi · 2013-11-08T16:06:09Z

@jspricke I like that idea. My concern in this case would be the distinction between using the append method or the previous one (loading the compressed PCD, pushing the data and saving it again compressed). Would we like to use such two different methods based only in one parameter? If the answer is yes, I'll move the template parameter to a constructor argument.

jspricke · 2013-11-09T16:19:15Z

I think a good API should hide away those two differences. A user shouldn't worry about the underlying implementation as long as performance is not a problem and if, it should be easy to adopt the code (like calling reserve).

rbrusu · 2013-12-17T23:37:48Z

This is a pretty cool contribution, thanks @juagargi ! Would it be possible to add some unit tests as well?

juagargi · 2018-02-22T17:15:00Z

I remembered I had this still open!
Do you guys still want some of these changes in? should I groom all this according to the comments?

SergioRAgostinho · 2018-02-26T09:54:01Z

Long long time no hear. Since it's been 4/5 years 😅, it might be a good idea to let the current maintainer team go through a review first before addressing any changes. The PR is already marked for a review.

It's definitely a good idea to rebase it against the current master branch though.

juagargi · 2018-02-26T13:53:12Z

It's rebased now. It still builds, but I haven't found where the UTs are, to run them. To be honest, I took just a quick look to the web pages for documentation on where they are, but didn't find them either.

SergioRAgostinho · 2018-02-27T09:44:43Z

PCD file tests are here:
https://github.com/PointCloudLibrary/pcl/blob/master/test/io/test_io.cpp

Out of core tests are here:
https://github.com/PointCloudLibrary/pcl/blob/master/test/outofcore/test_outofcore.cpp

To run the tests you first need to enable BUILD_global_tests option, let CMake find googletest, and then is a matter of running make tests.

To save compilation time you can simply request to build the tests you need make test_outofcore and then run them individually build/test/outofcore/test_outofcore.

… existing PCD file, and updates the header. Added a derived class from the OutofcoreOctreeDiskContainer that doesn't use compression, and uses the appender instead. Clean up and comments added. Fix mmap in Linux. Fix Linux build.

juagargi · 2018-03-06T09:30:28Z

The out of core tests seem to pass as well. I wonder if I should write one to cover this appender thing.

frozar · 2018-03-08T13:29:23Z

io/include/pcl/io/impl/pcd_io.hpp

+  data_size = cloud.points.size () * fsize;
+
+  data_idx += (tmp_cloud.width - cloud.width) * fsize;
+  if(data_idx != file_size)


Missing space before parenthese.

frozar · 2018-03-08T13:30:34Z

io/include/pcl/io/impl/pcd_io.hpp

+  data_idx += (tmp_cloud.width - cloud.width) * fsize;
+  if(data_idx != file_size)
+  {
+    // warn , deleteme: do more than just warn : is the file going to end up corrupted?


It seems that it is a missing TODO.

frozar · 2018-03-08T13:31:01Z

io/include/pcl/io/impl/pcd_io.hpp

+  data_idx += (tmp_cloud.width - cloud.width) * fsize;
+  if(data_idx != file_size)
+  {
+    // warn , deleteme: do more than just warn : is the file going to end up corrupted?


The same missing TODO here.

frozar · 2018-03-08T13:31:55Z

outofcore/include/pcl/outofcore/octree_disk_container.h

@@ -299,6 +299,50 @@ namespace pcl
        static boost::uuids::random_generator uuid_gen_;

    };
+
+/** \class OutofcoreOctreeDiskContainer without compression


Misaligned doc string.

Also fix indentation of doc string.

SergioRAgostinho · 2018-03-09T09:36:31Z

The out of core tests seem to pass as well. I wonder if I should write one to cover this appender thing.

Yes definitely!

stale · 2020-02-21T20:43:40Z

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.

DanielSepeda · 2021-01-25T22:36:22Z

Have these changes been implemented yet?

TaylorDale · 2021-03-11T03:46:48Z

I've attempted to implement this over the top of PCL by extending the writer class:
https://gist.github.com/TaylorDale/49ccff40d1591db2985e36cd14a003d1

However unfortunately after the first write the file becomes corrupted, specifically
"The expected data size and the current data size are different" as the data_idx differs from the file size.

Was a nice try but PCL's probably changed a bit since 8 years ago when this was pull was made.

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

SergioRAgostinho added status: stale needs: author reply Specify why not closed/merged yet labels Nov 22, 2017

SergioRAgostinho removed this from the pcl-1.9.0 milestone Nov 22, 2017

stale bot closed this Dec 14, 2017

jspricke reopened this Dec 18, 2017

stale bot removed the status: stale label Dec 18, 2017

taketwo added the status: stale label Dec 18, 2017

stale bot removed the status: stale label Feb 22, 2018

taketwo added needs: code review Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Feb 22, 2018

juagargi force-pushed the master branch from 2db8872 to f23e432 Compare February 26, 2018 13:18

frozar reviewed Mar 8, 2018

View reviewed changes

Don't append when incorrect header.

be94808

Also fix indentation of doc string.

juagargi force-pushed the master branch from f23e432 to be94808 Compare March 8, 2018 18:50

stale bot added the status: stale label Feb 21, 2020

taketwo added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Feb 29, 2020

stale bot removed status: stale labels Feb 29, 2020

taketwo added the status: stale label Feb 29, 2020

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

Uh oh!

Appending points to PCD and Out-of-core Octree modification to use append #325

Are you sure you want to change the base?

Appending points to PCD and Out-of-core Octree modification to use append #325

Uh oh!

Conversation

juagargi commented Oct 15, 2013

Uh oh!

jspricke commented Oct 26, 2013

Uh oh!

jspricke commented Oct 28, 2013

Uh oh!

juagargi commented Oct 29, 2013

Uh oh!

jspricke commented Oct 29, 2013

Uh oh!

juagargi commented Oct 30, 2013

Uh oh!

jspricke commented Nov 8, 2013

Uh oh!

juagargi commented Nov 8, 2013

Uh oh!

jspricke commented Nov 9, 2013

Uh oh!

rbrusu commented Dec 17, 2013

Uh oh!

juagargi commented Feb 22, 2018

Uh oh!

SergioRAgostinho commented Feb 26, 2018

Uh oh!

juagargi commented Feb 26, 2018

Uh oh!

SergioRAgostinho commented Feb 27, 2018

Uh oh!

juagargi commented Mar 6, 2018

Uh oh!

frozar Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

frozar Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

frozar Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

frozar Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho commented Mar 9, 2018

Uh oh!

stale bot commented Feb 21, 2020

Uh oh!

DanielSepeda commented Jan 25, 2021

Uh oh!

TaylorDale commented Mar 11, 2021

Uh oh!

Uh oh!