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

+  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

  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


-  /* \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

Pull Request Overview

This PR adds the ability to redirect PCL logging to a callable function, which addresses the need for programmatic access to logging output rather than just console printing. This change allows users to capture and handle log messages in custom ways.

Introduces a callback-based logging system with LogRecord structure
Refactors existing print functions to use templates and std::string instead of C-style variadic arguments
Replaces deprecated static member variables with static getter functions

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
common/include/pcl/console/print.h	Adds Logger class with callback support and converts print functions to templates
common/src/print.cpp	Implements Logger singleton and callback-based printing logic
test/common/test_console.cpp	New test file for validating logging functionality and callback behavior
test/common/CMakeLists.txt	Adds test registration for console testing
sample_consensus/include/pcl/sample_consensus/sac_model.h	Replaces deprecated static member with getter function
octree/include/pcl/octree/octree_key.h	Replaces deprecated static member with getter function
octree/include/pcl/octree/impl/octree_pointcloud.hpp	Updates to use new getter function
octree/include/pcl/octree/impl/octree_base.hpp	Updates to use new getter function and adds missing includes
octree/include/pcl/octree/impl/octree2buf_base.hpp	Updates to use new getter function
octree/src/octree_inst.cpp	Adds missing include for octree_key.h
octree/include/pcl/octree/octree_impl.h	Removes redundant include
tools/train_linemod_template.cpp	Updates print calls to use std::string instead of .c_str()
examples/segmentation/example_lccp_segmentation.cpp	Simplifies logging by removing stringstream usage
examples/segmentation/example_cpc_segmentation.cpp	Simplifies logging by removing stringstream usage

Copilot · 2025-08-09T16:43:11Z

common/include/pcl/console/print.h

+     */
+    template <typename... Args>
+    std::string
+    to_string(const std::string fmt_str, ...)


The variadic template function to_string declares variadic arguments with ... but doesn't use them in the parameter list. This should be to_string(const std::string& fmt_str, Args&&... args) to properly accept the template arguments.

Copilot · 2025-08-09T16:43:11Z

common/include/pcl/console/print.h

+      va_list ap;
+      while (true) {
+        formatted.reset(new char[n]); /* Wrap the plain char array into the unique_ptr */
+        std::strcpy(&formatted[0], fmt_str.c_str());


The call to std::strcpy is copying the format string over the allocated buffer, but then vsnprintf is called with the same format string. This strcpy call is unnecessary and potentially dangerous as it doesn't respect buffer bounds. Remove this line.

Suggested change

std::strcpy(&formatted[0], fmt_str.c_str());

// Removed unnecessary and potentially dangerous strcpy

Copilot · 2025-08-09T16:43:11Z

sample_consensus/include/pcl/sample_consensus/sac_model.h

        // Get a second point which is different than the first
        samples.resize (getSampleSize ());
-        for (unsigned int iter = 0; iter < max_sample_checks_; ++iter)
+        for (unsigned int iter = 0; iter < getMaxSampleSize(); ++iter)


The loop condition has changed from max_sample_checks_ to getMaxSampleSize(). However, getMaxSampleSize() returns the maximum sample checks (1000), while the function name suggests it returns a sample size. This creates confusion and potential logical errors. The function should be named getMaxSampleChecks() to match its purpose.

Copilot · 2025-08-09T16:43:11Z

test/common/test_console.cpp

+  pcl::console::setVerbosityLevel(pcl::console::L_VERBOSE);
+
+  PCL_ALWAYS_STREAM("Always_stream\n");
+  PCL_ERROR_STREAM("Error_stream\n")


Missing semicolon at the end of the statement.

Suggested change

PCL_ERROR_STREAM("Error_stream\n")

PCL_ERROR_STREAM("Error_stream\n");

Copilot · 2025-08-09T16:43:12Z

common/include/pcl/console/print.h

+
+    class LogRecorder {
+    public:
+      LogRecorder(std::string& string) : strstream(string) { }


The constructor is trying to initialize strstream with a std::string&, but strstream is declared as std::stringstream. This will cause a compilation error. The constructor should be LogRecorder(std::string& string) and the string should be stored as a reference to be updated later, or the design should be changed.

Suggested change

LogRecorder(std::string& string) : strstream(string) { }

LogRecorder(std::string& string) : target_string(string) { }

~LogRecorder() { target_string = strstream.str(); }

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

This comment was marked as outdated.

Sign in to view

larshg requested a review from Copilot August 9, 2025 16:41

Copilot AI reviewed Aug 9, 2025

View reviewed changes

	std::strcpy(&formatted[0], fmt_str.c_str());
	// Removed unnecessary and potentially dangerous strcpy

	PCL_ERROR_STREAM("Error_stream\n")
	PCL_ERROR_STREAM("Error_stream\n");

	LogRecorder(std::string& string) : strstream(string) { }
	LogRecorder(std::string& string) : target_string(string) { }
	~LogRecorder() { target_string = strstream.str(); }

Uh oh!

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

Uh oh!

Conversation

larshg commented Mar 1, 2025

Uh oh!

larshg commented Mar 1, 2025

Uh oh!

mvieth commented Mar 1, 2025

Uh oh!

larshg commented Mar 1, 2025

Uh oh!

larshg commented Mar 2, 2025

Uh oh!

roncapat commented Mar 3, 2025

Uh oh!

larshg commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roncapat commented Mar 3, 2025

Uh oh!

larshg commented Mar 3, 2025

Uh oh!

larshg commented Mar 3, 2025

Uh oh!

aurelienrb commented Mar 5, 2025

Uh oh!

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

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!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mvieth left a comment •

edited

Loading