E57Simple: Fix min/max fields in Data3D (#153)

There was a mixture of using float and double min/max. When using float, they should be set to E57_FLOAT_MIN and E57_FLOAT_MAX. When using double, they should be set to E57_DOUBLE_MIN and E57_DOUBLE_MAX. Do this adjustment to Data3D automatically in the Data3DPointsData_t() constructor and add tests for it.
asmaloney · Oct 29, 2022 · fc00ce3 · fc00ce3
1 parent 90a1cc6
commit fc00ce3
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 23 deletions.
diff --git a/include/E57SimpleData.h b/include/E57SimpleData.h
@@ -308,27 +308,27 @@ namespace e57
       bool sphericalElevationField = false;    //!< Indicates that the PointRecord sphericalElevation field is active
       bool sphericalInvalidStateField = false; //!< Indicates that the PointRecord sphericalInvalidState field is active
 
-      //! Indicates that the PointRecord cartesian and range fields should be configured with this minimum value
-      //! E57_FLOAT_MIN. If using a ScaledIntegerNode then this needs to be a minimum range value.
-      double pointRangeMinimum = E57_FLOAT_MIN;
+      //! Indicates that the PointRecord cartesian and range fields should be configured with this minimum value e.g.
+      //! E57_FLOAT_MIN or E57_DOUBLE_MIN. If using a ScaledIntegerNode then this needs to be a minimum range value.
+      double pointRangeMinimum = E57_DOUBLE_MIN;
 
-      //! Indicates that the PointRecord cartesian and range fields should be  configured with this maximum value
-      //! E57_FLOAT_MAX. If using a ScaledIntegerNode then this needs to be a maximum range value.
-      double pointRangeMaximum = E57_FLOAT_MAX;
+      //! Indicates that the PointRecord cartesian and range fields should be configured with this maximum value e.g.
+      //! E57_FLOAT_MAX or E57_DOUBLE_MAX. If using a ScaledIntegerNode then this needs to be a maximum range value.
+      double pointRangeMaximum = E57_DOUBLE_MAX;
 
       //! Indicates that the PointRecord cartesian and range fields should be configured
       //! as a ScaledIntegerNode with this scale setting. If E57_NOT_SCALED_USE_FLOAT, then use FloatNode.
       double pointRangeScaledInteger = E57_NOT_SCALED_USE_FLOAT;
 
-      //! Indicates that the PointRecord angle fields should be configured with  this minimum value E57_FLOAT_MIN. If
-      //! using a ScaledIntegerNode then this needs to be a minimum angle value.
-      double angleMinimum = E57_FLOAT_MIN;
+      //! Indicates that the PointRecord angle fields should be configured with this minimum value E57_FLOAT_MIN or
+      //! E57_DOUBLE_MIN. If using a ScaledIntegerNode then this needs to be a minimum angle value.
+      double angleMinimum = E57_DOUBLE_MIN;
 
-      //! Indicates that the PointRecord angle fields should be configured with this maximum value E57_FLOAT_MAX. If
-      //! using a ScaledIntegerNode then this needs to be a maximum angle value.
-      double angleMaximum = E57_FLOAT_MAX;
+      //! Indicates that the PointRecord angle fields should be configured with this maximum value e.g. E57_FLOAT_MAX or
+      //! E57_DOUBLE_MAX. If using a ScaledIntegerNode then this needs to be a maximum angle value.
+      double angleMaximum = E57_DOUBLE_MAX;
 
-      //! Indicates that the PointRecord angle fields should be configured as a  ScaledIntegerNode with this scale
+      //! Indicates that the PointRecord angle fields should be configured as a ScaledIntegerNode with this scale
       //! setting. If E57_NOT_SCALED_USE_FLOAT, then use FloatNode.
       double angleScaledInteger = E57_NOT_SCALED_USE_FLOAT;
 
@@ -352,13 +352,15 @@ namespace e57
       bool timeStampField = false;          //!< Indicates that the PointRecord timeStamp field is active
       bool isTimeStampInvalidField = false; //!< Indicates that the PointRecord isTimeStampInvalid field is active
 
-      //! Indicates that the PointRecord timeStamp fields should be configured with this maximum value.
-      double timeMaximum = E57_DOUBLE_MAX;
-
-      //! Indicates that the PointRecord timeStamp fields should be configured  with this minimum value E57_DOUBLE_MIN.
-      //! If using a ScaledIntegerNode then this needs to be a minimum time value.
+      //! Indicates that the PointRecord timeStamp fields should be configured with this minimum value e.g.
+      //! E57_UINT32_MIN, E57_DOUBLE_MIN or E57_DOUBLE_MIN. If using a ScaledIntegerNode then this needs to be a minimum
+      //! time value.
       double timeMinimum = E57_DOUBLE_MIN;
 
+      //! Indicates that the PointRecord timeStamp fields should be configured with this maximum value. e.g.
+      //! E57_UINT32_MAX, E57_DOUBLE_MAX or E57_DOUBLE_MAX.
+      double timeMaximum = E57_DOUBLE_MAX;
+
       //! Indicates that the PointRecord timeStamp fields should be configured as a ScaledIntegerNode with this scale
       //! setting. If E57_NOT_SCALED_USE_FLOAT then use FloatNode, if E57_NOT_SCALED_USE_INTEGER use IntegerNode.
       double timeScaledInteger = E57_NOT_SCALED_USE_FLOAT;
@@ -446,12 +448,13 @@ namespace e57
    {
       static_assert( std::is_floating_point<COORDTYPE>::value, "Floating point type required." );
 
-      //! @brief Default constructor does not manage any memory
+      //! @brief Default constructor does not manage any memory or adjust min/max for floats.
       Data3DPointsData_t() = default;
 
       //! @brief Constructor which allocates buffers for all valid fields in the given Data3D header
+      //! This constructor will also adjust the min/max fields in the data3D pointFields if we are using floats.
       //! @param [in] data3D Completed header which indicates the fields we are using
-      explicit Data3DPointsData_t( const e57::Data3D &data3D );
+      explicit Data3DPointsData_t( e57::Data3D &data3D );
 
       //! @brief Destructor will delete any memory allocated using the Data3DPointsData_t( const e57::Data3D & )
       //! constructor

diff --git a/src/E57SimpleData.cpp b/src/E57SimpleData.cpp
@@ -29,7 +29,7 @@ namespace e57
    }
 
    template <typename COORDTYPE>
-   Data3DPointsData_t<COORDTYPE>::Data3DPointsData_t( const Data3D &data3D ) : _selfAllocated( true )
+   Data3DPointsData_t<COORDTYPE>::Data3DPointsData_t( Data3D &data3D ) : _selfAllocated( true )
    {
       const auto cSize = data3D.pointsSize;
 
@@ -38,6 +38,17 @@ namespace e57
          throw E57_EXCEPTION2( E57_ERROR_VALUE_OUT_OF_BOUNDS, "pointsSize=" + toString( cSize ) + " minimum=1" );
       }
 
+      // We need to adjust min/max for floats.
+      if ( std::is_same<COORDTYPE, float>::value )
+      {
+         data3D.pointFields.pointRangeMinimum = E57_FLOAT_MIN;
+         data3D.pointFields.pointRangeMaximum = E57_FLOAT_MAX;
+         data3D.pointFields.angleMinimum = E57_FLOAT_MIN;
+         data3D.pointFields.angleMaximum = E57_FLOAT_MAX;
+         data3D.pointFields.timeMinimum = E57_FLOAT_MIN;
+         data3D.pointFields.timeMaximum = E57_FLOAT_MAX;
+      }
+
       if ( data3D.pointFields.cartesianXField )
       {
          cartesianX = new COORDTYPE[cSize];
@@ -196,8 +207,8 @@ namespace e57
       *this = Data3DPointsData_t<COORDTYPE>();
    }
 
-   template Data3DPointsData_t<float>::Data3DPointsData_t( const Data3D &data3D );
-   template Data3DPointsData_t<double>::Data3DPointsData_t( const Data3D &data3D );
+   template Data3DPointsData_t<float>::Data3DPointsData_t( Data3D &data3D );
+   template Data3DPointsData_t<double>::Data3DPointsData_t( Data3D &data3D );
 
    template Data3DPointsData_t<float>::~Data3DPointsData_t();
    template Data3DPointsData_t<double>::~Data3DPointsData_t();

diff --git a/test/src/CMakeLists.txt b/test/src/CMakeLists.txt
@@ -5,6 +5,7 @@ target_sources( ${PROJECT_NAME}
     PRIVATE
         ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/TestData.cpp
+        ${CMAKE_CURRENT_SOURCE_DIR}/test_SimpleData.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/test_SimpleReader.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/test_SimpleWriter.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/test_StringFunctions.cpp

diff --git a/test/src/test_SimpleData.cpp b/test/src/test_SimpleData.cpp
@@ -0,0 +1,63 @@
+// libE57Format testing Copyright © 2022 Andy Maloney <asmaloney@gmail.com>
+// SPDX-License-Identifier: MIT
+
+#include "gtest/gtest.h"
+
+#include "E57SimpleData.h"
+
+#include "Helpers.h"
+
+TEST( SimpleDataH, InvalidPointSize )
+{
+   e57::Data3D dataHeader;
+
+   E57_ASSERT_THROW( e57::Data3DPointsData pointsData( dataHeader ) );
+}
+
+TEST( SimpleDataH, HeaderMinMaxFloat )
+{
+   e57::Data3D dataHeader;
+
+   dataHeader.pointsSize = 1;
+
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMaximum, e57::E57_DOUBLE_MAX );
+   EXPECT_EQ( dataHeader.pointFields.angleMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.angleMaximum, e57::E57_DOUBLE_MAX );
+   EXPECT_EQ( dataHeader.pointFields.timeMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.timeMaximum, e57::E57_DOUBLE_MAX );
+
+   // This call should adjust our min/max for a variety of fields since we are using floats.
+   e57::Data3DPointsData pointsData( dataHeader );
+
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMinimum, e57::E57_FLOAT_MIN );
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMaximum, e57::E57_FLOAT_MAX );
+   EXPECT_EQ( dataHeader.pointFields.angleMinimum, e57::E57_FLOAT_MIN );
+   EXPECT_EQ( dataHeader.pointFields.angleMaximum, e57::E57_FLOAT_MAX );
+   EXPECT_EQ( dataHeader.pointFields.timeMinimum, e57::E57_FLOAT_MIN );
+   EXPECT_EQ( dataHeader.pointFields.timeMaximum, e57::E57_FLOAT_MAX );
+}
+
+TEST( SimpleDataH, HeaderMinMaxDouble )
+{
+   e57::Data3D dataHeader;
+
+   dataHeader.pointsSize = 1;
+
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMaximum, e57::E57_DOUBLE_MAX );
+   EXPECT_EQ( dataHeader.pointFields.angleMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.angleMaximum, e57::E57_DOUBLE_MAX );
+   EXPECT_EQ( dataHeader.pointFields.timeMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.timeMaximum, e57::E57_DOUBLE_MAX );
+
+   // This call should NOT adjust our min/max for a variety of fields since we are using doubles.
+   e57::Data3DPointsData_d pointsData( dataHeader );
+
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.pointRangeMaximum, e57::E57_DOUBLE_MAX );
+   EXPECT_EQ( dataHeader.pointFields.angleMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.angleMaximum, e57::E57_DOUBLE_MAX );
+   EXPECT_EQ( dataHeader.pointFields.timeMinimum, e57::E57_DOUBLE_MIN );
+   EXPECT_EQ( dataHeader.pointFields.timeMaximum, e57::E57_DOUBLE_MAX );
+}