refactor-improve-math-functions

[0x5844]

Description

This pull request addresses a critical issue in the math.h utility functions related to incorrect rounding behavior in the ceil_divide_signed function. Specifically, it fixes a bug where the function would incorrectly round up even when the division was exact, such as returning 31 instead of 30 for ceil_divide_signed(90, 3) -> (https://github.com/Ultimaker/CuraEngine/issues/2005)

Additionally, the PR includes the following improvements:

• Error Handling: Added exception handling to ensure division by zero is appropriately managed across all relevant functions.
• Refactoring: Refactored functions like round_divide_signed and floor_divide_signed to clarify logic, improve readability, and maintain consistency.
• Documentation: Enhanced inline documentation for better clarity and maintainability.

Type of change

• [x] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• [ ] Unit Testing: Updated and ran unit tests for all modified functions.
• [x] Code Review.

Test Configuration:

• Operating System: Ubuntu 22.04 LTS

Checklist:

• [x] My code follows the style guidelines of this project as described in UltiMaker Meta
• [x] I have read the Contribution guide
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] I have uploaded any files required to test this change

[0x5844]

Thanks for this! You know the current team-size, so we really appreciate these PR's 😄

--

I've made a few changes, most notably, I've removed the exception throwing after a discussion with Erwan:

Throwing exceptions means you rely on the code around that to be set up for that, which it currently is not. If we only want to know where it crashed, then we can just rely on SIGFPE to be signalled on division-by-zero (which is almost the only case where FPE happens anyway).

If we want to use exceptions in the code in general, we should have that discussion, but introducing this into a PR that fixes other stuff is not a good idea (especially in the same commit). -- Even then I'm on the fence on these particular functions, which should be basic math-ops.

That sounds great! I was on the fence about throwing exceptions but I went forward with it. I updated the tests under https://github.com/Ultimaker/CuraEngine/pull/2135/commits/953714c469fba494f6e21df85cb77f734560b4d6 and removed the exception expectation from them.

[rburema]

That sounds great! I was on the fence about throwing exceptions but I went forward with it.

That's what we have reviews for!

Dumping some info here since I'm off until next Tuesday:

• It seems some of the actions are failing because of a '/' in the branch-name.
• I've moved this to QA (who're in charge of the actual merging these days), but they might not be able to test if they've not got any builds.

[0x5844]

Dumping some info here since I'm off until next Tuesday:

Let me know if I can help with anything!