pcl icon indicating copy to clipboard operation
pcl copied to clipboard

[tools] pcl_visualizer segfaults when selecting double channel for coloring

Open themightyoarfish opened this issue 1 year ago • 3 comments

Describe the bug

When a pointcloud has an 8 F field, selecting this channel for display segfaults with this report:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x13fb99816)
  * frame #0: 0x0000000108aa4efc libvtkCommonCore-9.3.dylib`vtkAOSDataArrayTemplate<float>::SetArray(float*, long long, int, int) + 36
    frame #1: 0x0000000100492524 libpcl_visualization.1.14.dylib`pcl::visualization::PointCloudColorHandlerGenericField<pcl::PCLPointCloud2>::getColor() const + 696
    frame #2: 0x000000010047e2e8 libpcl_visualization.1.14.dylib`pcl::visualization::PCLVisualizerInteractorStyle::OnKeyDown() + 2428
    frame #3: 0x000000010286ce88 libvtkRenderingCore-9.3.1.dylib`vtkInteractorStyle::ProcessEvents(vtkObject*, unsigned long, void*, void*) + 4008
    frame #4: 0x0000000108a0613c libvtkCommonCore-9.3.dylib`vtkCallbackCommand::Execute(vtkObject*, unsigned long, void*) + 44
    frame #5: 0x0000000108b1ca98 libvtkCommonCore-9.3.dylib`vtkSubjectHelper::InvokeEvent(unsigned long, void*, vtkObject*) + 1232
    frame #6: 0x0000000101dce8f0 libvtkRenderingOpenGL2-9.3.dylib`-[vtkCocoaGLView invokeVTKKeyEvent:cocoaEvent:] + 608
    frame #7: 0x0000000198822fc8 AppKit`-[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:] + 316
    frame #8: 0x0000000198822cbc AppKit`-[NSWindow(NSEventRouting) sendEvent:] + 284
    frame #9: 0x000000019903aeb0 AppKit`-[NSApplication(NSEventRouting) sendEvent:] + 2360
    frame #10: 0x0000000198c4889c AppKit`-[NSApplication _handleEvent:] + 60
    frame #11: 0x00000001986eeb08 AppKit`-[NSApplication run] + 520
    frame #12: 0x000000010001cc48 pcl_viewer`main + 17620
    frame #13: 0x0000000194768274 dyld`start + 2840

Context

The point cloud was made from a custom point type:

struct PointXYZIT {
  PCL_ADD_POINT4D
  PCL_ADD_INTENSITY
  double time;
  PCL_MAKE_ALIGNED_OPERATOR_NEW
};

…

POINT_CLOUD_REGISTER_POINT_STRUCT(
    autocrane::PointXYZIT,
    (float, x, x)(float, y, y)(float, z, z)(float, intensity, intensity)(double,
                                                                         time,
                                                                         time))

Expected behavior

Viewer displays the double field correctly

Current Behavior

Viewer crashes

To Reproduce

  1. Build pcl d257a89130386c3356fba8ba0f04e8e2089704f3.
  2. run pcl_viewer on attached point cloud
  3. press key 6 to visualize time channel
  4. cry

Your Environment (please complete the following information):

  • OS: macos 15.1.1 ARM
  • Compiler: Apple Clang 16.0.0
  • PCL Version d257a89130386c3356fba8ba0f04e8e2089704f3

Possible Solution

Fix vtk display, or error when double fields are used

Additional context

2024-11-27T13_32_45_939Z.pcd.zip

themightyoarfish avatar Nov 27 '24 13:11 themightyoarfish

actually not sure if this is related to the double type, it happens too with an std::uint64_t type. maybe its just the 6th channel thats the issue?

themightyoarfish avatar Nov 27 '24 14:11 themightyoarfish

Inside PointCloudColorHandlerGenericField::getColor() it seems to assume that the field value fits inside a float.

float field_data
…
    // Color every point
    for (vtkIdType cp = 0; cp < nr_points; ++cp)
    {
      const std::uint8_t* pt_data = reinterpret_cast<const std::uint8_t*> (&(*cloud_)[cp]);
      memcpy (&field_data, pt_data + fields_[field_idx_].offset, pcl::getFieldSize (fields_[field_idx_].datatype));

      if (!std::isfinite (field_data))
        continue;

      colors[j] = field_data;
      j++;
    }

themightyoarfish avatar Nov 27 '24 14:11 themightyoarfish

Hi, yes seems like that currently only works with a float type. Even types that have the same size as a float (like uint32_t) would not work since they are not cast properly. I guess we should at least add a check whether the field is of type float or not. Making it actually work with other types would be more difficult and need a switch-case. The passthrough filter has a similar situation; it makes sure that the field is a float before computing.

mvieth avatar Nov 28 '24 11:11 mvieth

For posterity, i implemented it like this (slightly reduced version of generic field color handler), maybe this helps someone.

namespace pcl {
namespace visualization {
template <typename PointT> class PointCloudColorHandlerGenericField2
    : public PointCloudColorHandlerGenericField<PointT> {
  using PointCloud    = typename PointCloudColorHandler<PointT>::PointCloud;
  using PointCloudPtr = typename PointCloud::Ptr;
  using PointCloudConstPtr = typename PointCloud::ConstPtr;

public:
  using Ptr = shared_ptr<PointCloudColorHandlerGenericField2<PointT>>;
  using ConstPtr =
      shared_ptr<const PointCloudColorHandlerGenericField2<PointT>>;

  using PointCloudColorHandlerGenericField<
      PointT>::PointCloudColorHandlerGenericField;


  vtkSmartPointer<vtkDataArray> getColor() const override {
    if (!capable_ || !cloud_) {
      return nullptr;
    }

    auto scalars = vtkSmartPointer<vtkFloatArray>::New();
    scalars->SetNumberOfComponents(1);

    vtkIdType nr_points = cloud_->size();
    scalars->SetNumberOfTuples(nr_points);

    float* colors = new float[nr_points];

    int j = 0;
    // If XYZ present, check if the points are invalid
    int x_idx = -1;
    for (std::size_t d = 0; d < fields_.size(); ++d) {
      if (fields_[d].name == "x") {
        x_idx = static_cast<int>(d);
      }
    }

    if (x_idx != -1) {
      // Color every point
      for (vtkIdType cp = 0; cp < nr_points; ++cp) {
        // Copy the value at the specified field
        if (!std::isfinite((*cloud_)[cp].x) ||
            !std::isfinite((*cloud_)[cp].y) ||
            !std::isfinite((*cloud_)[cp].z)) {
          continue;
        }

        const std::uint8_t* pt_data =
            reinterpret_cast<const std::uint8_t*>(&(*cloud_)[cp]);
        const auto field_size = pcl::getFieldSize(fields_[field_idx_].datatype);
        std::uint8_t* field_data = new std::uint8_t[field_size];
        memcpy(field_data, pt_data + fields_[field_idx_].offset, field_size);

        float val;
        switch (fields_[field_idx_].datatype) {
          case pcl::PCLPointField::PointFieldTypes::FLOAT32:
            val = *reinterpret_cast<float*>(field_data);
            break;
          case pcl::PCLPointField::PointFieldTypes::UINT8:
            val = *reinterpret_cast<std::uint8_t*>(field_data);
            break;
          case pcl::PCLPointField::PointFieldTypes::UINT16:
            val = *reinterpret_cast<std::uint16_t*>(field_data);
            break;
          default:
            val = 0;
            break;
        }
        delete[] field_data;
        colors[j] = val;
        j++;
      }
    } else {
      throw std::runtime_error("No x coordinate in cloud");
    }
    scalars->SetArray(colors, j, 0, vtkFloatArray::VTK_DATA_ARRAY_DELETE);
    return scalars;
  }


protected:
  /** \brief Class getName method. */
  std::string getName() const override {
    return ("PointCloudColorHandlerGenericField2");
  }

private:
  using PointCloudColorHandler<PointT>::cloud_;
  using PointCloudColorHandler<PointT>::capable_;
  using PointCloudColorHandler<PointT>::field_idx_;
  using PointCloudColorHandler<PointT>::fields_;
};
}  // namespace visualization
}  // namespace pcl

themightyoarfish avatar Jul 03 '25 13:07 themightyoarfish