BuFran
commented
1 year ago Fast problem analysis
The datatype Length holds maximum 9 223 000km (if base is 1nm), and squared area overflows the data type above 2.147m so the limit of user input can be specified safely at +/- 2000mm
Deep analysis
Looking at code, at point.h line 213, there is first int64 multiply and add, then implicit conversion to binary64, then computation qSqrt and converting binary64 result back to int64 so floating point calculus is already used and the result will be precise at 53bits of mantissa (12 bits lost so when we will compute at ranges less than 9 223 000km/4096 resulting precision of the result will be unchanged anyway)
If the precision is your concern, you can use qHypot where the square of value is doing inside library itself, not storing the result anywhere and on modern compiler the calculation is done using binary128 datatype (until the result doesn't leave FPU, or is stored into memory). From the source, it is clear that qHypot calls std::hypot so that is the case.
From the reference, the precision of std::hypot is strict +-1LSB by definition so the result will be precise to 4um on 9223000km range. At distances 1000km wide, the double is not rounded at input and the result is precise to 1nm.
Reasonable fix
So for me, I recommend using std::hypot(mX, mY) (aka qHypot) instead of qSqrt(mX * mX + mY * mY) and the usable range will be +/- 1000km without loss of precision (quite enough for printed circuit board)
I can make PR with this change and make some tests around, seems simple fix here.
Architecture fix
All input should be always checked for errorneous values/invalid states at the edge of architecture. (Visual deny of operation on user input or Exception dialog when loading corrupted file). The clipping is possible to use too, but there is problem with little feedback to the user. User want set some value and program accepted different value that user wanted, so user lost original component position and has component on wrong place now. I prefer don't do that way. It's surprise for me that this nice app doesn't have it implemented yet.
That doesn't mean it should be tested every operation inside data layer, but the test should reside in data layer and be called only at edges of architecture.
ubruhin
VERSION / OS / ENVIRONMENT
SUMMARY
The application crashes when entering very large X/Y values in a board (probably also in other editors). See original bug report here: https://librepcb.discourse.group/t/crashing/570
STEPS TO REPRODUCE
Set a device X coordinate to e.g. 6000 mm.
EXPECTED RESULTS
The device is moved to X=6000mm.
ACTUAL RESULTS
LibrePCB crashes.
ADDITIONAL INFORMATION
Seems to be happening because an integer overflow happens during calculations of distance (involving the calculation of
x^2 + y^2), at least an assert fails - probably more goes wrong since the release build crashes as well.Not sure how to fix that properly. Maybe any user input should be clipped to a reasonable range (e.g. +/- 1000mm, if this is safe) to have enough reserve for further calculations. But maybe the limitation should be in the data layer instead of in the GUI :thinking:
Just failing during the calculations is not really nice as this could happen some time later in different actions (e.g. during DRC).
An alternative might be to perform the calculations with floating point operations if the numbers are large, but that feels quite ugly and leads to non-deterministic results...