Decouple Point/Points from Scale/Offset (#119)

* Store points as doubles internally * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix equality typo * update tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Have scale and offset apply only at pack/unpack time * Fix cpp tests * Fix cpp examples * fix py tests * fix py examples * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * PR Review * Changelog * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
RockRobotic · Sep 8, 2022 · 27fc408 · 27fc408
1 parent 7c26c91
commit 27fc408
Show file tree

Hide file tree

Showing 20 changed files with 372 additions and 538 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+- **\[Python/C++\]** Point and Points objects no longer depend on an internal scale or offset. XYZ coordinates are stored as doubles internally, and converted into integers using scale and offset only when packing or unpacking.
+
 ## [2.3.1] - 2022-04-14
 
 ### Fixed

diff --git a/cpp/include/copc-lib/las/point.hpp b/cpp/include/copc-lib/las/point.hpp
@@ -13,33 +13,23 @@ namespace copc::las
 class Point
 {
   public:
-    Point(const int8_t &point_format_id, const Vector3 &scale = Vector3::DefaultScale(),
-          const Vector3 &offset = Vector3::DefaultOffset(), const uint16_t &eb_byte_size = 0);
+    Point(const int8_t &point_format_id, const uint16_t &eb_byte_size = 0);
     Point(const LasHeader &header);
     Point(const Point &other);
 
     // Getters and Setters
 
 #pragma region XYZ
     // XYZ Scaled
-    double X() const { return ApplyScale(x_, scale_.x, offset_.x); }
-    void X(const double &x) { x_ = RemoveScale<int32_t>(x, scale_.x, offset_.x); }
+    double X() const { return x_scaled_; }
+    void X(const double &x) { x_scaled_ = x; }
 
-    double Y() const { return ApplyScale(y_, scale_.y, offset_.y); }
-    void Y(const double &y) { y_ = RemoveScale<int32_t>(y, scale_.y, offset_.y); }
+    double Y() const { return y_scaled_; }
+    void Y(const double &y) { y_scaled_ = y; }
 
-    double Z() const { return ApplyScale(z_, scale_.z, offset_.z); }
-    void Z(const double &z) { z_ = RemoveScale<int32_t>(z, scale_.z, offset_.z); }
+    double Z() const { return z_scaled_; }
+    void Z(const double &z) { z_scaled_ = z; }
 
-    // XYZ Unscaled
-    int32_t UnscaledX() const { return x_; }
-    void UnscaledX(const int32_t &x) { x_ = x; }
-
-    int32_t UnscaledY() const { return y_; }
-    void UnscaledY(const int32_t &y) { y_ = y; }
-
-    int32_t UnscaledZ() const { return z_; }
-    void UnscaledZ(const int32_t &z) { z_ = z; }
 #pragma endregion XYZ
 
     uint16_t Intensity() const { return intensity_; }
@@ -219,9 +209,6 @@ class Point
     int8_t PointFormatId() const { return point_format_id_; }
     uint16_t EbByteSize() const;
 
-    Vector3 Scale() const { return scale_; }
-    Vector3 Offset() const { return offset_; }
-
     bool operator==(const Point &other) const;
     bool operator!=(const Point &other) const { return !(*this == other); };
 
@@ -231,13 +218,13 @@ class Point
 
     static std::shared_ptr<Point> Unpack(std::istream &in_stream, const int8_t &point_format_id, const Vector3 &scale,
                                          const Vector3 &offset, const uint16_t &eb_byte_size);
-    void Pack(std::ostream &out_stream) const;
+    void Pack(std::ostream &out_stream, const Vector3 &scale, const Vector3 &offset) const;
     void ToPointFormat(const int8_t &point_format_id);
 
   protected:
-    int32_t x_{};
-    int32_t y_{};
-    int32_t z_{};
+    double x_scaled_{};
+    double y_scaled_{};
+    double z_scaled_{};
     uint16_t intensity_{};
     uint8_t returns_{};
     uint8_t flags_{};
@@ -258,8 +245,6 @@ class Point
   private:
     uint32_t point_record_length_;
     int8_t point_format_id_;
-    Vector3 scale_;
-    Vector3 offset_;
 };
 
 } // namespace copc::las

diff --git a/cpp/include/copc-lib/las/points.hpp b/cpp/include/copc-lib/las/points.hpp
@@ -18,8 +18,7 @@ namespace copc::las
 class Points
 {
   public:
-    Points(const int8_t &point_format_id, const Vector3 &scale, const Vector3 &offset,
-           const uint16_t &eb_byte_size = 0);
+    Points(const int8_t &point_format_id, const uint16_t &eb_byte_size = 0);
     Points(const LasHeader &header);
     // Will create Points object given a points vector
     Points(const std::vector<std::shared_ptr<Point>> &points);
@@ -47,15 +46,14 @@ class Points
     void AddPoints(std::vector<std::shared_ptr<Point>> points);
 
     // Point functions
-    std::shared_ptr<Point> CreatePoint()
-    {
-        return std::make_shared<Point>(point_format_id_, scale_, offset_, EbByteSize());
-    }
+    std::shared_ptr<Point> CreatePoint() { return std::make_shared<Point>(point_format_id_, EbByteSize()); }
     void ToPointFormat(const int8_t &point_format_id);
 
     // Pack/unpack
-    std::vector<char> Pack() const;
-    void Pack(std::ostream &out_stream) const;
+    std::vector<char> Pack(const LasHeader &header) const;
+    std::vector<char> Pack(const Vector3 &scale, const Vector3 &offset) const;
+    void Pack(std::ostream &out_stream, const Vector3 &scale, const Vector3 &offset) const;
+    void Pack(std::ostream &out_stream, const LasHeader &header) const;
     static Points Unpack(const std::vector<char> &point_data, const int8_t &point_format_id,
                          const uint16_t &eb_byte_size, const Vector3 &scale, const Vector3 &offset);
     static Points Unpack(const std::vector<char> &point_data, const LasHeader &header);
@@ -114,57 +112,6 @@ class Points
             points_[i]->Z(in[i]);
     }
 
-    std::vector<int32_t> UnscaledX() const
-    {
-        std::vector<int32_t> out;
-        out.resize(Size());
-        std::transform(points_.begin(), points_.end(), out.begin(),
-                       [](const std::shared_ptr<Point> &p) { return p->UnscaledX(); });
-        return out;
-    }
-    void UnscaledX(const std::vector<int32_t> &in)
-    {
-        if (in.size() != Size())
-            throw std::runtime_error("UnscaledX setter array must be same size as Points array!");
-
-        for (unsigned i = 0; i < points_.size(); ++i)
-            points_[i]->UnscaledX(in[i]);
-    }
-
-    std::vector<int32_t> UnscaledY() const
-    {
-        std::vector<int32_t> out;
-        out.resize(Size());
-        std::transform(points_.begin(), points_.end(), out.begin(),
-                       [](const std::shared_ptr<Point> &p) { return p->UnscaledY(); });
-        return out;
-    }
-    void UnscaledY(const std::vector<int32_t> &in)
-    {
-        if (in.size() != Size())
-            throw std::runtime_error("UnscaledY setter array must be same size as Points array!");
-
-        for (unsigned i = 0; i < points_.size(); ++i)
-            points_[i]->UnscaledY(in[i]);
-    }
-
-    std::vector<int32_t> UnscaledZ() const
-    {
-        std::vector<int32_t> out;
-        out.resize(Size());
-        std::transform(points_.begin(), points_.end(), out.begin(),
-                       [](const std::shared_ptr<Point> &p) { return p->UnscaledZ(); });
-        return out;
-    }
-    void UnscaledZ(const std::vector<int32_t> &in)
-    {
-        if (in.size() != Size())
-            throw std::runtime_error("UnscaledZ setter array must be same size as Points array!");
-
-        for (unsigned i = 0; i < points_.size(); ++i)
-            points_[i]->UnscaledZ(in[i]);
-    }
-
     std::vector<uint8_t> Classification() const
     {
         std::vector<uint8_t> out;
@@ -279,8 +226,6 @@ class Points
     std::vector<std::shared_ptr<Point>> points_;
     int8_t point_format_id_;
     uint32_t point_record_length_;
-    Vector3 scale_;
-    Vector3 offset_;
 };
 } // namespace copc::las
 #endif // COPCLIB_LAS_POINTS_H_
diff --git a/cpp/include/copc-lib/utils.hpp b/cpp/include/copc-lib/utils.hpp
@@ -0,0 +1,12 @@
+#ifndef COPCLIB_UTILS_H_
+#define COPCLIB_UTILS_H_
+
+#include <cmath>
+
+namespace copc
+{
+
+bool AreClose(double a, double b, double epsilon) { return fabs(a - b) < epsilon; }
+
+} // namespace copc
+#endif // COPCLIB_UTILS_H_
diff --git a/cpp/src/io/copc_writer_public.cpp b/cpp/src/io/copc_writer_public.cpp
@@ -107,7 +107,7 @@ Node Writer::AddNode(const VoxelKey &key, const las::Points &points, const Voxel
         points.PointRecordLength() != config_->LasHeader()->PointRecordLength())
         throw std::runtime_error("Writer::AddNode: New points must be of same format and size.");
 
-    std::vector<char> uncompressed_data = points.Pack();
+    std::vector<char> uncompressed_data = points.Pack(*config_->LasHeader());
     return AddNode(key, uncompressed_data, page_key);
 }
 

diff --git a/cpp/src/io/laz_writer.cpp b/cpp/src/io/laz_writer.cpp
@@ -22,7 +22,7 @@ void LazWriter::WritePoints(const las::Points &points)
         points.PointRecordLength() != config_->LasHeader().PointRecordLength())
         throw std::runtime_error("LazWriter::WritePoints: New points must be of same format and size.");
 
-    std::vector<char> uncompressed_data = points.Pack();
+    std::vector<char> uncompressed_data = points.Pack(config_->LasHeader());
     WriteChunk(uncompressed_data);
 }
 

diff --git a/cpp/src/las/point.cpp b/cpp/src/las/point.cpp
@@ -1,9 +1,9 @@
 #include "copc-lib/las/point.hpp"
+#include "copc-lib/utils.hpp"
 
 namespace copc::las
 {
-Point::Point(const int8_t &point_format_id, const Vector3 &scale, const Vector3 &offset, const uint16_t &eb_byte_size)
-    : scale_(scale), offset_(offset), point_format_id_(point_format_id)
+Point::Point(const int8_t &point_format_id, const uint16_t &eb_byte_size) : point_format_id_(point_format_id)
 {
     if (point_format_id < 6 || point_format_id > 8)
         throw std::runtime_error("Point: Point format must be 6-8");
@@ -16,16 +16,13 @@ Point::Point(const int8_t &point_format_id, const Vector3 &scale, const Vector3
     extra_bytes_.resize(eb_byte_size, 0);
 }
 
-Point::Point(const LasHeader &header)
-    : Point(header.PointFormatId(), header.Scale(), header.Offset(), header.EbByteSize())
-{
-}
+Point::Point(const LasHeader &header) : Point(header.PointFormatId(), header.EbByteSize()) {}
 
-Point::Point(const Point &other) : Point(other.point_format_id_, other.Scale(), other.Offset(), other.EbByteSize())
+Point::Point(const Point &other) : Point(other.point_format_id_, other.EbByteSize())
 {
-    x_ = other.x_;
-    y_ = other.y_;
-    z_ = other.z_;
+    x_scaled_ = other.x_scaled_;
+    y_scaled_ = other.y_scaled_;
+    z_scaled_ = other.z_scaled_;
     intensity_ = other.intensity_;
     returns_ = other.returns_;
     flags_ = other.flags_;
@@ -56,8 +53,8 @@ bool Point::operator==(const Point &other) const
 {
     if (point_format_id_ != other.point_format_id_ || point_record_length_ != other.point_record_length_)
         return false;
-    if (x_ != other.UnscaledX() || y_ != other.UnscaledY() || z_ != other.UnscaledZ() ||
-        intensity_ != other.Intensity())
+    if (!AreClose(X(), other.X(), 0.000001) || !AreClose(Y(), other.Y(), 0.000001) ||
+        !AreClose(Z(), other.Z(), 0.000001) || intensity_ != other.Intensity())
         return false;
     if (returns_ != other.ReturnsBitField())
         return false;
@@ -83,11 +80,11 @@ bool Point::Within(const Box &box) const { return box.Contains(Vector3(X(), Y(),
 std::shared_ptr<Point> Point::Unpack(std::istream &in_stream, const int8_t &point_format_id, const Vector3 &scale,
                                      const Vector3 &offset, const uint16_t &eb_byte_size)
 {
-    std::shared_ptr<Point> p = std::make_shared<Point>(point_format_id, scale, offset, eb_byte_size);
+    std::shared_ptr<Point> p = std::make_shared<Point>(point_format_id, eb_byte_size);
 
-    p->x_ = unpack<int32_t>(in_stream);
-    p->y_ = unpack<int32_t>(in_stream);
-    p->z_ = unpack<int32_t>(in_stream);
+    p->x_scaled_ = ApplyScale(unpack<int32_t>(in_stream), scale.x, offset.x);
+    p->y_scaled_ = ApplyScale(unpack<int32_t>(in_stream), scale.y, offset.y);
+    p->z_scaled_ = ApplyScale(unpack<int32_t>(in_stream), scale.z, offset.z);
     p->intensity_ = unpack<uint16_t>(in_stream);
     p->returns_ = unpack<uint8_t>(in_stream);
     p->flags_ = unpack<uint8_t>(in_stream);
@@ -114,12 +111,12 @@ std::shared_ptr<Point> Point::Unpack(std::istream &in_stream, const int8_t &poin
     return p;
 }
 
-void Point::Pack(std::ostream &out_stream) const
+void Point::Pack(std::ostream &out_stream, const Vector3 &scale, const Vector3 &offset) const
 {
     // Point
-    pack(x_, out_stream);
-    pack(y_, out_stream);
-    pack(z_, out_stream);
+    pack(RemoveScale<int32_t>(x_scaled_, scale.x, offset.x), out_stream);
+    pack(RemoveScale<int32_t>(y_scaled_, scale.y, offset.y), out_stream);
+    pack(RemoveScale<int32_t>(z_scaled_, scale.z, offset.z), out_stream);
     pack(intensity_, out_stream);
     pack(returns_, out_stream);
     pack(flags_, out_stream);
@@ -157,7 +154,7 @@ std::string Point::ToString() const
 {
     std::stringstream ss;
     ss << "Point: " << std::endl;
-    ss << "\tX: " << x_ << ", Y: " << y_ << ", Z: " << z_ << std::endl;
+    ss << "\tX: " << x_scaled_ << ", Y: " << y_scaled_ << ", Z: " << z_scaled_ << std::endl;
     ss << "\tIntensity: " << intensity_ << std::endl;
     ss << "\tReturn Number: " << static_cast<short>(ReturnNumber())
        << ", Number of Returns: " << static_cast<short>(NumberOfReturns()) << std::endl;

diff --git a/cpp/src/las/points.cpp b/cpp/src/las/points.cpp
@@ -7,19 +7,15 @@
 namespace copc::las
 {
 
-Points::Points(const int8_t &point_format_id, const Vector3 &scale, const Vector3 &offset, const uint16_t &eb_byte_size)
-    : point_format_id_(point_format_id), scale_(scale), offset_(offset)
+Points::Points(const int8_t &point_format_id, const uint16_t &eb_byte_size) : point_format_id_(point_format_id)
 {
     if (point_format_id < 0 || point_format_id > 10)
         throw std::runtime_error("Point format must be 0-10.");
 
     point_record_length_ = PointByteSize(point_format_id, eb_byte_size);
 }
 
-Points::Points(const LasHeader &header)
-    : Points(header.PointFormatId(), header.Scale(), header.Offset(), header.EbByteSize())
-{
-}
+Points::Points(const LasHeader &header) : Points(header.PointFormatId(), header.EbByteSize()) {}
 
 Points::Points(const std::vector<std::shared_ptr<Point>> &points)
 {
@@ -28,8 +24,6 @@ Points::Points(const std::vector<std::shared_ptr<Point>> &points)
 
     point_record_length_ = points[0]->PointRecordLength();
     point_format_id_ = points[0]->PointFormatId();
-    scale_ = points[0]->Scale();
-    offset_ = points[0]->Offset();
 
     AddPoints(points);
 }
@@ -91,7 +85,7 @@ Points Points::Unpack(const std::vector<char> &point_data, const int8_t &point_f
     auto ss = std::istringstream(std::string(point_data.begin(), point_data.end()));
 
     // Go through each Point to unpack the data from the stream
-    Points points(point_format_id, scale, offset, eb_byte_size);
+    Points points(point_format_id, eb_byte_size);
     points.Reserve(point_count);
 
     // Unpack points
@@ -103,16 +97,23 @@ Points Points::Unpack(const std::vector<char> &point_data, const int8_t &point_f
     return points;
 }
 
-void Points::Pack(std::ostream &out_stream) const
+void Points::Pack(std::ostream &out_stream, const LasHeader &header) const
+{
+    Pack(out_stream, header.Scale(), header.Offset());
+}
+
+void Points::Pack(std::ostream &out_stream, const Vector3 &scale, const Vector3 &offset) const
 {
     for (const auto &point : points_)
-        point->Pack(out_stream);
+        point->Pack(out_stream, scale, offset);
 }
 
-std::vector<char> Points::Pack() const
+std::vector<char> Points::Pack(const LasHeader &header) const { return Pack(header.Scale(), header.Offset()); }
+
+std::vector<char> Points::Pack(const Vector3 &scale, const Vector3 &offset) const
 {
     std::stringstream out;
-    Pack(out);
+    Pack(out, scale, offset);
     auto ostr = out.str();
     return std::vector<char>(ostr.begin(), ostr.end());
 }

diff --git a/example/example-laz-writer.cpp b/example/example-laz-writer.cpp
@@ -35,9 +35,9 @@ las::Points RandomPoints(const las::LasHeader &header, int number_points)
         // The use of las::Point constructor is strongly discouraged, instead use las::Points::CreatePoint
         auto point = points.CreatePoint();
         // point has getters/setters for all attributes
-        point->UnscaledX(rand_x(gen));
-        point->UnscaledY(rand_y(gen));
-        point->UnscaledZ(rand_z(gen));
+        point->X(header.ApplyScaleX(rand_x(gen)));
+        point->Y(header.ApplyScaleY(rand_y(gen)));
+        point->Z(header.ApplyScaleZ(rand_z(gen)));
 
         points.AddPoint(point);
     }

diff --git a/example/example-writer.cpp b/example/example-writer.cpp
@@ -220,9 +220,9 @@ las::Points RandomPoints(const VoxelKey &key, const las::LasHeader &header, int
         // The use of las::Point constructor is strongly discouraged, instead use las::Points::CreatePoint
         auto point = points.CreatePoint();
         // point has getters/setters for all attributes
-        point->UnscaledX(rand_x(gen));
-        point->UnscaledY(rand_y(gen));
-        point->UnscaledZ(rand_z(gen));
+        point->X(header.ApplyScaleX(rand_x(gen)));
+        point->Y(header.ApplyScaleY(rand_y(gen)));
+        point->Z(header.ApplyScaleZ(rand_z(gen)));
         // For visualization purposes
         point->PointSourceId(key.d + key.x + key.y + key.z);