AngusJohnson / Clipper2

Polygon Clipping and Offsetting - C++, C# and Delphi
Boost Software License 1.0
1.45k stars 266 forks source link

First step for habitrary types Clipper2 #860

Closed soufianekhiat closed 3 months ago

soufianekhiat commented 3 months ago

First step to have Clipper2 compiling on arbitrary type. The point of the PR is to prepare the future work to have Clipper2 working with various precision. Open to comment. Suffix of "I" (for integer) instead of "64" (for int64_t) and "S" (for Scalar) instead of "D" (for double).

#ifndef Scalar
  // Use double as default
  typedef double Scalar;
#endif
#ifndef Integer
  // Use 64 bits as default
  typedef int64_t Integer;
#endif
  typedef std::make_unsigned_t<Integer> UInteger;

Performance Impact: {double, int64_t}:

Edge Count: 1000 = 67.0 millisecs
Edge Count: 2000 = 249 millisecs
Edge Count: 3000 = 739 millisecs
Edge Count: 4000 = 1.46 secs
Edge Count: 5000 = 3.29 secs
Edge Count: 6000 = 7.75 secs
Edge Count: 7000 = 14.6 secs
28.2 secs

{float, int32_t}:

Complex Polygons Benchmark:
Edge Count: 1000 = 69.0 millisecs
Edge Count: 2000 = 257 millisecs
Edge Count: 3000 = 747 millisecs
Edge Count: 4000 = 1.42 secs
Edge Count: 5000 = 3.18 secs
Edge Count: 6000 = 7.25 secs
Edge Count: 7000 = 13.6 secs
26.6 secs

{float, int16_t}:

Complex Polygons Benchmark:
Edge Count: 1000 = 63.6 millisecs
Edge Count: 2000 = 247 millisecs
Edge Count: 3000 = 801 millisecs
Edge Count: 4000 = 1.47 secs
Edge Count: 5000 = 3.32 secs
Edge Count: 6000 = 7.13 secs
Edge Count: 7000 = 13.0 secs
26.0 secs

The idea the prepare future work to have Clipper2 independant of data type storage. Scalar could be = { double, float, float16_t } Integer could be = { int64_t, int32_t, int16_t }

Currently all the test pass for { double, int64_t } pair. For { float, int32_t } Currently we can accept that Clipper2 only support { double, int64_t } version. Default test (and Z-version):

[  FAILED  ] 4 tests, listed below:
[  FAILED  ] Clipper2Tests.TestCarryCalculation
[  FAILED  ] Clipper2Tests.TestIsCollinear2
[  FAILED  ] Clipper2Tests.TestOffsets3
[  FAILED  ] Clipper2Tests.TestOffsets8

For { float, int16_t }

[  FAILED  ] 10 tests, listed below:
[  FAILED  ] Clipper2Tests.TestCarryCalculation
[  FAILED  ] Clipper2Tests.TestIsCollinear2
[  FAILED  ] Clipper2Tests.TestOffsets3
[  FAILED  ] Clipper2Tests.TestOffsets4
[  FAILED  ] Clipper2Tests.TestOffsets5
[  FAILED  ] Clipper2Tests.TestOffsets8
[  FAILED  ] Clipper2Tests.TestOffsets10
[  FAILED  ] Clipper2Tests.TestMultiplePolygons
[  FAILED  ] Clipper2Tests.TestPolytreeHoles7
[  FAILED  ] Clipper2Tests.TestRectClip3

Results for Benchmark with fixed seed srand( 97 ):

{ double, int64_t }: double_int64_t

{ double, int32_t }: double_int32_t

{ double, int16_t }: double_int16_t

{ float, int64_t }: float_int64_t

{ float, int32_t }: float_int32_t

{ float, int16_t }: float_int16_t

AngusJohnson commented 3 months ago

Thanks for the suggestion. While your use of arbitrary types certainly has merit, this would require too many changes for me to consider. Maybe in Clipper3 😁.

soufianekhiat commented 3 months ago

The point of this PR is not to support all type permutation. Just "preparing the work". Currently for each update of Clipper I do the same thing again multiple time: hiding "double" behind a typedef, same for int64_t. But the rename is maybe an extra too much. Would you consider the PR is the only had a typedef without renaming the struct/class?