CURA 10724 smooth operator #1891
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a smooth action, which will smooth small segments (maximum resolution) of a polygon, whom deviate more than a certain angle (wall transition angle) compared to a fluid motion line between the segment before and after. Smoothing is done by moving the first point of the small middle segment to the first point of the previous point with a smooth distance (need to add this to the front end). The second point of the middle segment is moved to the second point of the last segment. If either the previous or next segment is shorter then the smooth distance, then that point in the middle segment is removed Contributes to CURA-10724 Contributes to Ultimaker/Cura#14811
First make sure that the smoothing works, by not skipping any math steps. Contributes to Ultimaker/Cura#14811 Contributes to CURA-10724
Shift with vectors instead Contributes to Ultimaker/Cura#14811 Contributes to CURA-10724
Bring a bit more order to this [CURA-10724]
This would allow for getting a x,y,z coordinate for a IntPoint, or 3D point,
described as a struct or vector/array/tuple etc. Will be expanded to accommodate
Arachne types.
```
struct Point{ int X; int Y; }
Point p_struct{ .X = 1, .Y = 2. };
std::array<int, 2> p_array { 1, 2 };
auto x_struct = std::get<"X">(p_struct);
auto x_array = std::get<"X">(p_array);
```
[CURA-10724]
This allows for reusing the logic for Arachne types. [CURA-10724]
Update smooth action to handle changing path size, cycle views, and filtering out removed points. The previous implementation had issues with sliding views due to the path size change. Also, fix range constraints and improve readability. This refactor improves the algorithm's accuracy and performance by better handling the changing path size and filtering out points marked for removal. Additionally, the updated range constraints and readability make it easier to understand and maintain the code. Contributes to CURA-10724
The `smooth` action codebase has been heavily refactored to improve readability, avoid redundancy and make the codebase more efficient. Importantly, third-party library RangeV3 has been more efficiently used, unnecessary headers were removed, and unnecessary debug logs were deleted. Also, reusable shifting operation for p1 (towards p0) and p2 (towards p3) were extracted to a new function, `shiftPointTowards`. Contributes to CURA-10724
The smoothing function has been optimized and a bug with the shift_points function has been fixed. The operator in the smoothing function has been made "constexpr" to potentially improve performance. The way "windows" are generated has been restructured to make it clearer and eliminate redundancy. In shift_points, the subtraction used to alter point's coordinates was replaced with addition to rectify a mistake. The distance calculation in the smoothing function has been adjusted from an incorrect dot product into a correct hypotenuse calculation. These changes should help in improving the efficiency and correctness of the smoothing function. Contributes to CURA-10724
The existing tests for the smooth function were simplified and adjusted. It applies the smoothing function to those points and then verifies that the original polygon remains the same after smoothing. This change will make the test more reliable as it verifies that smoothing algorithm doesn't alter essential data. Contributes to CURA-10724
This commit adds more granular checks for geometrical types (2D & 3D points) in the geometry header. It introduces concepts to inspect whether types are represented as tuple, range, or named fields. Moreover, it checks if an object is of type segment or a range of segments. This refactoring enhances versatility for geometrical types, ensuring greater type safety and readability in the code. Contribute to CURA-10724
…haracter This commit addresses the need of accessing point coordinates more flexibly. It introduces functions to access the coordinates of 2D and 3D points either by coordinate index (e.g. 0 for X, 1 for Y in a 2D point) or by character (e.g. 'x' or 'X' for X-coordinate). It makes handling points more intuitive and comes in handy when the specific type of point (2D or 3D) is not of interest. Additionally, accessing coordinates of a junction by index or character is also implemented, further enhancing the ease of use. Contributes to CURA-10724
In this commit, the syntax for the comments in the geometry.h file has been updated. The "\\" syntax was replaced by "@" as the latter is more standard for Doxygen usage. This change allows for better parsing and generation of documentation by Doxygen, and aligns with common practices in the software industry. Contributes to CURA-10724
…ability The smoothing function in `smooth.h` was refactored to improve code readability and maintainability. The 'smooth_distance' parameter was removed as it was not being utilized. The core logic of the function remains unchanged, and several variable names were updated to better reflect their context and function. Introduced a clearer, more accurate method to calculate the cosine of the angle deviation which is also generally optimized for smaller values of the `fluid_angle`. This is a more computational efficient approach since for 'Curved' interpolations, the `fluid_angle` is usually small which solves the primary problem. Contributes to CURA-10724
Moved the 'smoother' logic from the 'slicer' and 'WallToolPaths' execution to occur earlier in the 'prepared_outline' execution. This change intentional to reduce redundancy and improve the efficiency of our slicing process by ensuring we're smoothing the polygons just once. This could also help reduce the amount of crashes in the Voronoi generation/usage An unnecessary smoother function call with redundant parameters was also removed from src/slicer.cpp." Contributes to CURA-10724
Updated the path smoothing algorithm by refining the condition checks and optimizing certain calculations for better efficiency. - Introduced a condition to handle open paths. - Corrections in comments for better understanding. - Loop stop condition refined to prevent unwanted iterations. - Compute function introduced to calculate magnitudes, making the code cleaner. - Reorganized functions for better readability. - Deviation calculation and filtering methods optimized for better performance. This update is aimed at enhancing the performance of path smoothing. Contributes to CURA-10724
# Conflicts: # src/WallToolPaths.cpp
Updated the parameters in the SmoothTest of CURA utility tests. The third parameter of the smoothing function has been removed due to recent changes in the CURA engine algorithm. Consequently, the expected results of test were also updated to match the output from the updated smoothing function. Contributes to CURA-10724
Contributes to CURA-10724
I hate Apple Clang! Contribute to CURA-10724
Due to varying support for the C++20 feature std::integral, preprocessor checks were added to the Geometry utility. This ensures that the Geometry utility functions will compile and run as expected across different compilers, especially older ones or those that don't fully support C++20 yet. Additionally, if std::integral is not supported, standard member accessing is used instead. This modification enhances the code's robustness and compatibility. Contributes to CURA-10724
Such that future developers are aware of struggles we had to support Apple Clang. _The pain is real... never give up... word!_ Contributes to CURA-10724
This commit addresses potential compatibility issues with different versions of C++ by adding conditionals to the `junction` concept in the `arachne.h` utility file. As some versions may not support the `std::integral` check, we fall back to directly assessing `val.w` if the C++ version is older or a particular library version with compatibility issues is detected. This change ensures successful compilation across varying environments. Contributes to CURA-10724
This commit removes the preprocessor conditions that were originally put in to cope with an issue in Clang13 C++20. A fallback version of the integral concept has been added in the std namespace if the C++ version is below 201703L or if the library version is lesser than the Clang13 version. This allows for a cleaner and more compatible codebase across various C++ versions. In addition, "include/utils/types/generic.h" is now being included in "include/utils/types/geometry.h", "include/utils/types/arachne.h" and "include/utils/actions/smooth.h" everywhere std::integral might be used. Contributes to CURA-10724
1 task
The angle checking in the smoothing utility now check against actual angles instead of the cosine angle. This is slightly more computational heavy but needed since otherwise a lot of points where missed. The function `isWithinDeviations` has also been renamed to `isWithinAllowedDeviations` for clarity. Contributes to CURA-10724
Often the first segments weren't filtered, by concatenating the tail to the front we also take these last elements into account. Also, adjusted the conditions in if-statements for better accuracy and introduced variables for allowed deviations and smoothing distance for better code consistency. Contributes to CURA-10724
This change replaces the 'tail' view with a 'single' view in the smoothing process of the model. The change ensures that the smoothing process takes into consideration the entire model, instead of ignoring the first point (head). The 'single' view denotes the single element at the back of the range 'tmp'. This small adjustment ensures the smoother start of the 3D model's path and addresses a specific corner case where the first point was being neglected. Contributes to CURA-10724
The filter condition has been altered to be less aggressive, since the intend is to smooth out the small sudden deviations. With this in mind the removal of points is also discarded since this can introduce self- intersecting shapes, which will result in hard crashes, but more importantly introduce to much of a deviation from the intended shape. Contribute to CURA-10724
Added a debug log line for 'prepared_outline' in 'WallToolPaths.cpp' to capture and trace its parameters. This is to help troubleshoot issues related to polygon simplification and self-intersection fixes in complex polygons as it wasn't clear if the existing adjustments solved all occurrences. Contributes to CURA-10724
This commit reorders the includes in SkeletonTrapezoidation.cpp for better code organization and adds a logging line to aid troubleshooting. The logger will now print values of polys, section_type, and layer_idx when constructing from polygons, giving better visibility into their state in case of issues. Contributes to CURA-10724
Added condition to skip smoothing process for sections that are of section type 'support'. This is done to improve performance and eliminate unnecessary computation as these support walls do not require smoothing. Contributes to CURA-10724
Apple Clang 13 doesn't have full support yet. Contributes to CURA-10724
This reverts commit 95234f7.
casperlamboo
reviewed
Jul 3, 2023
nallath
requested changes
Jul 3, 2023
|
|
||
|
|
||
| TEST(SmoothTest, TestSmooth) | ||
| { |
There was a problem hiding this comment.
Needs a bit of documentation; Why is this a good test? What is it trying to do? (nitpicking)
There was a problem hiding this comment.
Test was used during debugging, for quick iterative cycles. I believe it also fails currently. In will create a couple actual useful test cases, tomorrow
Applied review comments. This commit addresses several changes in the code to improve readability and add more assertions where necessary. Redundant comments have been removed, such as the namespace declaration in geometry.h for readability. Grammar mistakes in comments were corrected for the same reason in smooth.h. In get.h and generic.h, a static assertion was added to restrict the size of the string value to one character, ensuring concise point coordinate inputs. In generic.h, a comment was added to indicate why we are adding a manual implementation of integral and floating point, due to lack of support in older versions of Apple Clang. These changes were made to improve the quality and maintainability of the code. Contributes to CURA-10724
Write some actual tests CURA-10724
Smoothing can probably introduce some self-intersections. Which can result in crashes CURA-10724
casperlamboo
reviewed
Jul 3, 2023
| template<typename T> | ||
| concept point2d_tuple = requires(T t) | ||
| { | ||
| requires std::is_same_v<T, std::tuple<typename std::tuple_element<0, T>::type, typename std::tuple_element<0, T>::type>>; |
There was a problem hiding this comment.
Suggested change
| requires std::is_same_v<T, std::tuple<typename std::tuple_element<0, T>::type, typename std::tuple_element<0, T>::type>>; | |
| requires std::is_same_v<T, std::tuple<typename std::tuple_element<0, T>::type, typename std::tuple_element<1, T>::type>>; |
There was a problem hiding this comment.
This is intentional, this forces to have both elements the same type, because it should now check if a tuple<type_0, type_1> is the same as a tuple<type_0, type_0>, hence type_0 should be the same as type_1
| template<typename T> | ||
| concept point3d_tuple = requires(T t) | ||
| { | ||
| requires std::is_same_v<T, std::tuple<typename std::tuple_element<0, T>::type, typename std::tuple_element<0, T>::type, typename std::tuple_element<0, T>::type>>; |
There was a problem hiding this comment.
Suggested change
| requires std::is_same_v<T, std::tuple<typename std::tuple_element<0, T>::type, typename std::tuple_element<0, T>::type, typename std::tuple_element<0, T>::type>>; | |
| requires std::is_same_v<T, std::tuple<typename std::tuple_element<0, T>::type, typename std::tuple_element<1, T>::type, typename std::tuple_element<2, T>::type>>; |
This reverts commit d7fd718.
casperlamboo
approved these changes
Jul 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The simplification of polygons sometimes leaves small segments with large differences in angles compared to the segments before and after them. This occurs because the simplification algorithm is over-constrained, and the solution space doesn't diverge to an optimal point, but to a plateau. This means that smaller segments won't be filtered out since doing so would violate other criteria, such as removing too much area, etc.
Printers with Marlin 1.x seem to handle the resulting toolpaths better than smooth motion planners such as Klipper. These printers see a sudden change in direction and will try to adhere to the requested toolpath as much as possible. Because that toolpath is now suddenly requesting a change in direction (for a very short length), the head will slow down to make the corner, but the extrusion process can't adjust at the same speed, resulting in over-extrusion at that spot.
Fix
Since we can reasonably assume that such a sudden change in toolpath direction over a short distance, compared to the direction of the toolpath before and after, does not belong to the actual intended shape of the sliced model, we can adjust the polygons in favour of the intent and smooth motion printers.
We loop over each closed polygon, which is used to generate the Aranche toolpaths, and examine 3 segments (4 points) each time:
AB->BC->CD. Sudden small segmentsBC(< maximum resolution) that deviate more than a certain angle compared to a fluid motion angle (wall transition angle) between the preceding segmentABor subsequentCDare smoothed out. This is done by either shifting pointBorCtowardsAorDby half the maximum resolution's distanceor removing them altogether if there is no room to shift them.It also introduces some
conceptsandstd::getfunctions which can be used to obtain coordinates from different Point and Junction types by either specifying the index or letter: X, Y or x, y, zTODO:
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: