Add shape level inflate method

[DusKing1]

Fix #168 . Please kindly review this and just ask me to change code that you don't want it to be that way. Really appreciate if you can find your time to do the review job!

[fontanf]

Thank you. That looks good overall.

• The inflate function should take shape holes as parameters as well (const std::vector<Shape>& holes) and deflate them.

• Then you should remove the GenralizedTrapezoid::inflate method completely and replace the other calls by shape inflate and then trapezoidation.

• Could you add some comments in the inflate function so that I can understand and maintain it?

• Could you add a couple of unit tests in this file for example https://github.com/fontanf/packingsolver/blob/master/test/irregular/shape_test.cpp

[DusKing1]

Sure, lemme do that.

[DusKing1]

Urghhh, making this such a huge change was not my original intention.... Please let me know if you still need me to change the code.

[fontanf]

I would prefer if you don't include the refactoring, it makes the PR harder to read.

The more so, the changes broke multiple tests, including several not related to minimum distances:

 	  2 - IrregularTrapezoid.Intersection1 (Failed)
	  3 - IrregularTrapezoid.Intersection2 (Failed)
	  4 - IrregularTrapezoid.Intersection3 (Failed)
	 12 - IrregularShape.InflateWithHoles (Failed)
	 41 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/rectangles_non_guillotine.json" (Failed)
	 42 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/direction_y.json" (Failed)
	 43 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-06-25.json" (Failed)
	 44 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/non_rectangular_bin.json" (Failed)
	 45 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/polygon_with_hole.json" (Failed)
	 46 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-07-01.json" (Failed)
	 47 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/rotations.json" (Failed)
	 48 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/multiple_bins.json" (Failed)
	 49 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/trapezoidal_insertions.json" (Failed)
	 50 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/trapezoidal_insertions_2.json" (Failed)
	 51 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/trapezoidal_insertions_3.json" (Failed)
	 52 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/trapezoidal_insertions_4.json" (Failed)
	 53 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-05.json" (Failed)
	 54 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-08_sub_1.json" (Failed)
	 55 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-08_sub_2.json" (Failed)
	 56 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-08_sub_3.json" (Failed)
	 57 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-08.json" (Failed)
	 58 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-13.json" (Failed)
	 59 - Irregular/AlgorithmTest.Algorithm/"data/irregular/tests/mirror.json" (Failed)
	 60 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-25_1.json" (Failed)
	 61 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-25_2.json" (Failed)
	 62 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-25_3.json" (Failed)
	 63 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-08-25_4.json" (Failed)
	 64 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-09-11.json" (Failed)
	 65 - Irregular/AlgorithmTest.Algorithm/"data/irregular/users/2024-09-21.json" (Failed)

If you remove the refactoring and the failing tests are still there, I could look at it as well.

Could you also remove the holes attribute you added in the Shape structure?

[fontanf]

Additionally, inflating/deflating a polygon may lead to self-intersecting inflated/deflated polygons. There are functions in the library to post-process self-intersecting polygons here https://github.com/fontanf/packingsolver/blob/master/src/irregular/shape_self_intersections_removal.hpp

You can see how they are used in the polygon simplification algorithm https://github.com/fontanf/packingsolver/blob/master/src/irregular/shape_simplification.cpp

And that would be relevant to have unit tests for these cases.

[DusKing1]

So lemme reset the head to 87e77e4?

[fontanf]

Or you can do a commit to undo the changes. It will be squashed in the end anyway.

[DusKing1]

No problem I can do reset, that can keep clear commit history. Let's change code step by step. I'll back on code 5 hrs later.

[DusKing1]

Reset back to 87e77e4, what do you want me to change?

[fontanf]

Thank you. You can do what I mentioned in the first message #169 (comment) and in this other message #169 (comment)

Please run the tests locally

cd build/test/
ctest --parallel --output-on-failure

And check that the current tests and your new tests are working

Please let me know if you get stuck to fix some tests. I could take a look then.

[DusKing1]

I think I need to do it step by step to make commit history clear and catch what you want clearly since I'm not a proper C++ developer :(

1. to implement holes (const std::vector<Shape>& holes) to Shape inflate(), with using lib functions here to post-process self-intersecting polygons
2. to remove the GenralizedTrapezoid::inflate method completely and replace the other calls by, calling the new Shape inflate firstly and then to do the trapezoidation to the inflated shape.
3. to add a new unittest for this

Please correct me if any mis-understanding.

[fontanf]

Yes, that's it

[DusKing1]

I still have FAILED test:

The following tests FAILED:
          1 - PackingSolver_boxstacks_test_NOT_BUILT (Not Run)
         39 - IrregularShapeInflate.BasicInflate (Failed)
         40 - IrregularShapeInflate.InflateWithHoles (Failed)
         41 - IrregularShapeInflate.SelfIntersectingInflate (Failed)
         42 - IrregularShapeInflate.Deflate (Failed)
         43 - IrregularShapeInflate.TinyShape (Failed)
         44 - IrregularShapeInflate.LargeInflation (Failed)
         46 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\rectangles_non_guillotine.json" (Failed)
         47 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\direction_y.json" (Failed)
         48 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-06-25.json" (Failed)
         49 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\non_rectangular_bin.json" (Failed)
         50 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\polygon_with_hole.json" (Failed)
         51 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-07-01.json" (Failed)
         52 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\rotations.json" (Failed)
         53 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\multiple_bins.json" (Failed)
         54 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\trapezoidal_insertions.json" (Failed)
         55 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\trapezoidal_insertions_2.json" (Failed)
         56 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\trapezoidal_insertions_3.json" (Failed)
         57 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\trapezoidal_insertions_4.json" (Failed)
         58 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-05.json" (Failed)
         59 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08_sub_1.json" (Failed)
         60 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08_sub_2.json" (Failed)
         61 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08_sub_3.json" (Failed)
         62 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08.json" (Failed)
         63 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-13.json" (Failed)
         64 - Irregular/AlgorithmTest.Algorithm/"data\irregular\tests\mirror.json" (Failed)
         65 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_1.json" (Failed)
         66 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_2.json" (Failed)
         67 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_3.json" (Failed)
         68 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_4.json" (Failed)
         69 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-09-11.json" (Failed)
         70 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-09-21.json" (Failed)
Errors while running CTest

Can you take a look?

[fontanf]

Your new tests only call the code you added:

         39 - IrregularShapeInflate.BasicInflate (Failed)
         40 - IrregularShapeInflate.InflateWithHoles (Failed)
         41 - IrregularShapeInflate.SelfIntersectingInflate (Failed)
         42 - IrregularShapeInflate.Deflate (Failed)
         43 - IrregularShapeInflate.TinyShape (Failed)
         44 - IrregularShapeInflate.LargeInflation (Failed)

Please fix these first.

[DusKing1]

It seems that the compute_intersections can't handle CircularArc properly. Do you want me to modify it so to support the CircularArc element?

[fontanf]

Even if the input allows defining circular arcs, the solver doesn't handle them (yet) and there would be multiple other functions to update to do so.

But I actually think it's good to have the inflate function generating the rounded corners.

For now, we'll break them into multiple line segments after the inflate.

So yes, if you feel you can update this function to handle circular arcs, I think it's the best thing to do. I can handle this as well, but probably in at least a few days.

[DusKing1]

Got you. I can take a try to implement arc support to the solver, but i think that somehow is beyond the scope of this PR since, I originally just wanna enhance the inflate function. Do you like me to create a seperate PR for implementing arc support to the solver firstly then rebase the pr after that one got merged? Or I continue committing to this PR and you'll just squash them all at the end of the day?

[fontanf]

Implementing full support for circular arc is a big and not that obvious task.

If you can have an inflate function using circular arcs which is working and well tested, that would be very good for this PR.

Then I could take care of finishing its integration, or you could do it as well if you're more in a hurry

[DusKing1]

Lemme make an inflate function using circular arcs first, then think about how to get it well-tested (how to make proper unit test for it).

[DusKing1]

Bed time hehe. I'll just commit them here. It will be great if you can take a look into the new inflate function which is still WIP.

Another question, what about just implementing the Minkowski sum as the inflate function instead of struggling with such an approximation which I'm writing?

[fontanf]

Choose an approach that you can make work reliably.

I refactored and cleaned the shape code. It should be easier to write tests the same way as this one for example now:

packingsolver/test/irregular/shape_test.cpp

Lines 8 to 17 in 855d304

	TEST(IrregularShape, CleanShapeRedundant)
	{
	Shape shape = build_shape({{0, 0}, {0, 0}, {100, 0}, {100, 100}});
	Shape cleaned_shape = clean_shape(shape);
	std::cout << cleaned_shape.to_string(0) << std::endl;
	Shape expected_shape = build_shape({{0, 0}, {100, 0}, {100, 100}});
	EXPECT_EQ(expected_shape, cleaned_shape);
	}

I also updated the build_shape (previously build_polygon_shape) function to be able to use it to generate shapes with circular arcs:

packingsolver/include/packingsolver/irregular/shape.hpp

Lines 229 to 244 in 855d304

	/**
	* Function to help build a shape easily.
	*
	* Espacially useful to write tests.
	*
	* Examples:
	*
	* Build a polygon:
	*
	* Build a right triangle where the hypotenuse is an inflated circular arc
	* build_shape({{0, 0}, {1, 0}, {0, 0, 1}, {1, 1}})
	*
	* Build a right triangle where the hypotenuse is a drilled circular arc
	* build_shape({{0, 0}, {1, 0}, {0, 0, -1}, {1, 1}})
	*/
	Shape build_shape(const std::vector<BuildShapeElement>& points);

[bbeaulant]

I also updated the build_shape (previously build_polygon_shape) function to be able to use it to generate shapes with circular arcs:

Hello @fontanf,
Sorry about that, but this last commit degrades performance a lot.

The following input was computed in less than 2 seconds. Compilation based on this commit.

And now it gives me a less well optimized result in over 100 seconds :

[fontanf]

@bbeaulant thank you for your vigilance. I pushed a fix for this on the current dev branch.

[fontanf]

@DusKing1 could you rebase your branch and move the inflate function and its subfunctions to new shape_inflate.{hpp,cpp} files?

[DusKing1]

@fontanf yes for sure, lemme commit my latest work and rebase then do the move. I have less fail tests now. I saw your fix on the master branch, thanks for your quick response.

[DusKing1]

I think now the inflate function is ready for your review. Here are the remaining failed tests that I don't know how to fix:

The following tests FAILED:
         53 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-07-01.json" (Failed)
         61 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08_sub_1.json" (Failed)
         63 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08_sub_3.json" (Failed)
         64 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-08.json" (Failed)
         65 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-13.json" (Failed)
         67 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_1.json" (Failed)
         68 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_2.json" (Failed)
         69 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_3.json" (Failed)
         70 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-08-25_4.json" (Failed)
         71 - Irregular/AlgorithmTest.Algorithm/"data\irregular\users\2024-09-11.json" (Failed)

Please lemme know if you have further change requests.

[fontanf]

Thank you for the update.

In the tests, could you check the whole shape instead of the just the min and max? I added a near function to compare 2 shapes. I also changed some tests to parametrized tests. That would be relevant here as well. For example

packingsolver/test/irregular/shape_test.cpp

Lines 40 to 92 in 6ea89b0

	struct ApproximateCircularArcByLineSegmentsTestParams
	{
	ShapeElement circular_arc;
	ElementPos number_of_line_segments;
	bool outer;
	std::vector<ShapeElement> expected_line_segments;
	};
	class IrregularApproximateCircularArcByLineSegmentsTest: public testing::TestWithParam<ApproximateCircularArcByLineSegmentsTestParams> { };
	TEST_P(IrregularApproximateCircularArcByLineSegmentsTest, ApproximateCircularArcByLineSegments)
	{
	ApproximateCircularArcByLineSegmentsTestParams test_params = GetParam();
	std::cout << "circular_arc" << std::endl;
	std::cout << test_params.circular_arc.to_string() << std::endl;
	std::cout << "expected_line_segments" << std::endl;
	for (const ShapeElement& line_segment: test_params.expected_line_segments)
	std::cout << line_segment.to_string() << std::endl;
	std::vector<ShapeElement> line_segments = approximate_circular_arc_by_line_segments(
	test_params.circular_arc,
	test_params.number_of_line_segments,
	test_params.outer);
	std::cout << "line_segments" << std::endl;
	for (const ShapeElement& line_segment: line_segments)
	std::cout << line_segment.to_string() << std::endl;
	ASSERT_EQ(line_segments.size(), test_params.number_of_line_segments);
	for (ElementPos pos = 0; pos < test_params.number_of_line_segments; ++pos) {
	//std::cout << std::setprecision (15) << line_segments[pos].start.x << std::endl;
	EXPECT_TRUE(near(line_segments[pos], test_params.expected_line_segments[pos]));
	}
	}
	INSTANTIATE_TEST_SUITE_P(
	Irregular,
	IrregularApproximateCircularArcByLineSegmentsTest,
	testing::ValuesIn(std::vector<ApproximateCircularArcByLineSegmentsTestParams>{
	{
	build_shape({{1, 0}, {0, 0, 1}, {0, 1}}, true).elements.front(),
	1,
	false,
	build_shape({{1, 0}, {0, 1}}, true).elements
	}, {
	build_shape({{1, 0}, {0, 0, 1}, {0, 1}}, true).elements.front(),
	2,
	true,
	build_shape({{1, 0}, {1, 1}, {0, 1}}, true).elements
	}, {
	build_shape({{1, 0}, {0, 0, 1}, {0, 1}}, true).elements.front(),
	3,
	true,
	build_shape({{1, 0}, {1, 0.414213562373095}, {0.414213562373095, 1}, {0, 1}}, true).elements
	}}));

Could you translate Chinese comments into English?

To compare to 0, could you use equal(value, 0.0) instead of comparing to 1e-6?

                    // For small gaps, prefer line segment connectors
                    if (gap_distance < 0.1) {

I'm not a big fan of this, we cannot assume that 0.1 is small. All lengths could be around 0.01.

There are functions to compute angles in the library that you can use directly:

packingsolver/src/irregular/shape.cpp

Lines 120 to 137 in 6ea89b0

	Angle irregular::angle_radian(
	const Point& vector)
	{
	Angle a = std::atan2(vector.y, vector.x);
	if (a < 0)
	a += 2 * M_PI;
	return a;
	}
	Angle irregular::angle_radian(
	const Point& vector_1,
	const Point& vector_2)
	{
	Angle a = atan2(vector_2.y, vector_2.x) - atan2(vector_1.y, vector_1.x);
	if (a < 0)
	a += 2 * M_PI;
	return a;
	}

    // Remove duplicate points or degenerate elements
    if (inflated_shape.elements.size() > 1) {
        std::vector<ShapeElement> cleaned_elements;
        cleaned_elements.reserve(inflated_shape.elements.size());

        for (const ShapeElement& element : inflated_shape.elements) {
            if (euclidean_distance(element.start, element.end) > 1e-6) {
                cleaned_elements.push_back(element);
            }
        }

        inflated_shape.elements = cleaned_elements;
    }

Could the clean_shape function already available in the library replace this piece of code?

That great to have an is_point_inside_shape function. Is it strictly inside or not? could you explicit it in the name? And inside the function, could you use the equal, strictly_lesser and strictly_greater functions to do the comparisons?

Could you undo the changes in branching_scheme.cpp and trapezoid.hpp? Because of the circular arcs in the inflated shapes, the integration will need to be a bit more sophisticated. But that should provide better results in the end.

[fontanf]

Hi,

I decided to move the shape package in a separate repository since I need it for other applications.

https://github.com/fontanf/shape

The shape inflate function should now be added there. I close this pull request. Could you move your code to the new repository?

I apologize for the inconvenience. The development of the library is quite active right now and some things change quickly

[DusKing1]

Ok, I'll check that repo later, and maybe I'll need some guidance from you on how to setup that repo to continue development.
Just had some progress on the math these days, I think I'll made this inflate method more robust and reasonable.

[fontanf]

Ok, I'll check that repo later, and maybe I'll need some guidance from you on how to setup that repo to continue development.

The repository is very similar to the previous shape code of the packing library. But do not hesitate if you have question

Just had some progress on the math these days, I think I'll made this inflate method more robust and reasonable.

Great news