Added the ability to redirect logging to a callable. #6244

larshg · 2025-03-01T17:57:04Z

I'm in deep water here, but it seems to work :)

Please come with suggestions, before spent too much time in the wrong way.

Could close #5589

larshg · 2025-03-01T21:14:19Z

@mvieth do you have any idea to why there is linker errors with pcl::octree:OctreeKey::maxDepth ?

mvieth · 2025-03-01T23:02:15Z

@mvieth do you have any idea to why there is linker errors with pcl::octree:OctreeKey::maxDepth ?

Must have something to do with the usage of maxDepth here: https://github.com/PointCloudLibrary/pcl/blob/master/octree/include/pcl/octree/impl/octree_base.hpp#L98
maxDepth is a static const member, so maybe something related to ODR-use? I have to look at your changes in detail first before I can make a more specific guess.

larshg · 2025-03-01T23:09:18Z

Ahh, its in that print statement it fails... hmm, still odd though.

larshg · 2025-03-02T20:39:05Z

Maybe pcl::console::LogRecorder should be a member of the Logger, to avoid creating a stringstream for each stream command - would that make sense?

roncapat · 2025-03-03T11:43:47Z

Nice for me, even if I not like too much the following:

pcl::console::Logger::getInstance().setCallback(...)

because of the getInstance() call... is there a way to remove this indirection level?

Moreover, I think that the previous callback needs to be returned from the setCallaback() call to be able to restore it.

larshg · 2025-03-03T11:59:12Z

Nice for me, even if I not like too much the following:
pcl::console::Logger::getInstance().setCallback(...)
because of the getInstance() call... is there a way to remove this indirection level?

Moreover, I think that the previous callback needs to be returned from the setCallaback() call to be able to restore it.

Somehow the callback needs to be saved - and having just a static global variable to a std::function, didn't seem right either. At least it was the best solution I found, also browsing other logging frameworks.

roncapat · 2025-03-03T12:07:48Z

common/src/print.cpp

+  static Logger instance;
+  return instance;
+}
+
 ////////////////////////////////////////////////////////////////////////////////


template <typename Functor> void pcl::console::Logger::setCallback(Functor&& callback){ getInstance().setCallback(std::move(callback)); }

DISCLAIMER: draft, untested code.
@larshg wouldn't this work? Would the template be a problem there?

You want a free function like:

template <typename Functor> void setCallback(Functor&& callback) { Logger::getInstance().setCallback(std::move(callback)); }

so you can call:
pcl::console::setCallback(...);

Yes, thats possible.

roncapat · 2025-03-03T12:08:40Z

Moreover, why generic template "functor" instead of std::function?

larshg · 2025-03-03T12:20:22Z

Else I couldn't get it to work with lambdas - but maybe I did it wrong 😄

larshg · 2025-03-03T12:21:27Z

To restore normal logging again, you can just set the callback to a nullptr.

aurelienrb · 2025-03-05T14:49:10Z

I had a quick look and it looks promising to me, thanks for your work

mvieth

I assume we are targeting PCL 1.16.0, due to likely ABI changes?

Is there a way to easily reset to the default behaviour, after calling setCallback? Edit: I just read your other comment: set callback to a nullptr. Please document this in the code somewhere.

mvieth · 2025-03-22T16:11:26Z

common/include/pcl/console/print.h

+      }
+
+      LogRecorder&
+      operator<<(std::ostream& (std::ostream&))


This one looks strange, what is it for?

It was for handling std::endl if I remember correctly - that apparently need this operator.

mvieth · 2025-03-22T16:27:54Z

examples/segmentation/example_cpc_segmentation.cpp

@@ -210,7 +210,7 @@ CPCSegmentation Parameters: \n\
  pcl::PCLPointCloud2 input_pointcloud2;
  if (pcl::io::loadPCDFile (pcd_filename, input_pointcloud2))
  {
-    PCL_ERROR ("ERROR: Could not read input point cloud %s.\n", pcd_filename.c_str ());
+    PCL_ERROR ("ERROR: Could not read input point cloud %s.\n", pcd_filename);


Does .c_str() still work? But is not necessary any more?

Seems it still is necessary. See https://godbolt.org/z/zaca3aGK3 - MSVC output correctly, but GCC output random stuff.

mvieth · 2025-03-22T16:29:40Z

octree/include/pcl/octree/octree_key.h

@@ -138,9 +138,14 @@ class OctreeKey {
                                      (!!(this->z & depthMask)));
  }

-  /* \brief maximum depth that can be addressed */
-  static const unsigned char maxDepth =
-      static_cast<unsigned char>(sizeof(uindex_t) * 8);


Would it be possible to keep this and deprecate it? Same for max_sample_checks_.

I guess so - not sure if it will do linking errors downstream though.

mvieth · 2025-03-22T16:46:48Z

common/include/pcl/console/print.h

+  { \
+    pcl::console::LogRecorder rec; \
+    rec << ARGS; \
+    pcl::console::LogRecord entry{pcl::console::LEVEL, rec.to_string()}; \


Could there be a way to tell the stringstream of LogRecorder to directly operate on the message string of the LogRecord? Otherwise there are multiple allocations and copies here, I think.

…cation notice.

…t logging to console

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (14)

common/include/pcl/console/print.h: Language not supported
common/src/print.cpp: Language not supported
examples/segmentation/example_cpc_segmentation.cpp: Language not supported
examples/segmentation/example_lccp_segmentation.cpp: Language not supported
octree/include/pcl/octree/impl/octree2buf_base.hpp: Language not supported
octree/include/pcl/octree/impl/octree_base.hpp: Language not supported
octree/include/pcl/octree/impl/octree_pointcloud.hpp: Language not supported
octree/include/pcl/octree/octree_impl.h: Language not supported
octree/include/pcl/octree/octree_key.h: Language not supported
octree/src/octree_inst.cpp: Language not supported
sample_consensus/include/pcl/sample_consensus/sac_model.h: Language not supported
test/common/CMakeLists.txt: Language not supported
test/common/test_console.cpp: Language not supported
tools/train_linemod_template.cpp: Language not supported

larshg force-pushed the addconsoleredirect branch 10 times, most recently from a544354 to 9eb28fc Compare March 1, 2025 21:13

larshg force-pushed the addconsoleredirect branch 2 times, most recently from 6d1d0a4 to 44320b0 Compare March 2, 2025 19:44

larshg force-pushed the addconsoleredirect branch from 05b6bfa to a376418 Compare March 3, 2025 08:20

mvieth mentioned this pull request Mar 3, 2025

[console] How can I redirect the log output? #5589

Open

roncapat reviewed Mar 3, 2025

View reviewed changes

mvieth reviewed Mar 22, 2025

View reviewed changes

larshg added 4 commits March 25, 2025 14:59

Added the ability to redirect logging to a callable.

97f596e

Use formatted string instead of temporary stringstream.

ed01cff

Add includes.

83573ca

cleanup

0f0fa61

larshg added 9 commits March 25, 2025 14:59

Test for undefined symbol.

918ddbd

Fix copy paste error.

242261c

Fix ODR of OctreeKey::maxdepth.

b49c8f4

Remove unnecessary c_str().

5038d8b

Wrap static member variables in a getter method.

b0d715d

Add wrapper function for easier setting the callback.

be72de7

Address parsing in the string from the LogRecord to reduce copying.

1fb51d3

Revert removal of c_str()

172f52b

Revert direct removal of the two static variables, but added deprecre…

5384843

…cation notice.

larshg force-pushed the addconsoleredirect branch from 4248763 to 5384843 Compare March 25, 2025 15:33

Added more documentation for usage of the setCallback and how to rese…

746959d

…t logging to console

larshg force-pushed the addconsoleredirect branch from 448c188 to 746959d Compare March 25, 2025 15:53

Fix missing field in initializer

3169df7

larshg added this to the pcl-1.16.0 milestone Mar 26, 2025

larshg requested a review from Copilot March 26, 2025 18:18

Copilot AI reviewed Mar 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability to redirect logging to a callable. #6244

Added the ability to redirect logging to a callable. #6244

larshg commented Mar 1, 2025

larshg commented Mar 1, 2025

mvieth commented Mar 1, 2025

larshg commented Mar 1, 2025

larshg commented Mar 2, 2025

roncapat commented Mar 3, 2025

larshg commented Mar 3, 2025

roncapat Mar 3, 2025

larshg Mar 3, 2025

roncapat commented Mar 3, 2025

larshg commented Mar 3, 2025

larshg commented Mar 3, 2025

aurelienrb commented Mar 5, 2025

mvieth left a comment •

edited

Loading

mvieth Mar 22, 2025

larshg Mar 25, 2025

mvieth Mar 22, 2025

larshg Mar 25, 2025

mvieth Mar 22, 2025

larshg Mar 25, 2025

mvieth Mar 22, 2025

Copilot AI left a comment

Added the ability to redirect logging to a callable. #6244

Are you sure you want to change the base?

Added the ability to redirect logging to a callable. #6244

Conversation

larshg commented Mar 1, 2025

larshg commented Mar 1, 2025

mvieth commented Mar 1, 2025

larshg commented Mar 1, 2025

larshg commented Mar 2, 2025

roncapat commented Mar 3, 2025

larshg commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roncapat commented Mar 3, 2025

larshg commented Mar 3, 2025

larshg commented Mar 3, 2025

aurelienrb commented Mar 5, 2025

mvieth left a comment • 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

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

Copilot AI left a comment

Choose a reason for hiding this comment

mvieth left a comment •

edited

Loading