RS_Painter::toGui crashes

when drawing a line starting on the word "draft"

yes, that's a bit interim update and work in progress so far.
You can just comment the code assert.

As a temporary solution, you add the following code at the end of the method to ensure your local version is operational:

//    {
        double uiX=0., uiY=0.;
        const_cast<RS_Painter*>(this)->toGui(worldCoordinates, uiX, uiY);
//        using namespace RS_Math;
//        assert(equal(uiX, uiPosition.x) && equal(uiY, uiPosition.y));
//    }
    return {uiX, uiY};
//    return uiPosition;

What are the steps to reproduce this issue?

I added the assertion based on the understanding the two versions are equivalent in computation.

the difference occurs when UCS includes rotation in addition to shift.  

Please take a look to my comment in PR - in comments, I've attached a sample file. It includes several UCS's.

You can simply load it and switch between UCS's in the widget.

Something like https://youtu.be/DniuQTr6UCU

I suppose the comment is there: https://github.com/LibreCAD/LibreCAD/pull/2032#issuecomment-2730457541

Plus take a look to several comments up to this.

Also, please note my comment regarding the bounding boxes calculation (in commit to master).

Hm... however, as far as I can see by the screenshot, there is no rotation... And this is strange, so it seems that the difference may occur even if there is shift only? Anyway, please try my sample file

The assertion is triggered due to tight ULP comparison, I have relaxed the assertion tolerance to 1E-12, which is much tighter than 1E-10 used throughout.

The assertion is not triggered with 1E-12 tolerance. I guess 1E-13 would work as well, but there's no point to be paranoid. By reviewing the code, it's clearly those two versions are equivalent.

While the equivalence is not always up to the ULP level, this is not accumulative here (meaning the output would feekback to the algorithm repeatededly).

The lesson is to use ULP comparison with care. In this particular case, there's probably no need to optimize at the ULP level. If needed, optimization up to ULP can be done by kaham summation, but there's no point to that here.

https://github.com/LibreCAD/LibreCAD/commit/e6f74e4e00a689a07fee738f036f2fa6805c304a

sand1024 wrote

Hm... however, as far as I can see by the screenshot, there is no rotation... And this is strange, so it seems that the difference may occur even if there is shift only? Anyway, please try my sample file

I have the current source code and can't reproduce the error anymore.

The key point I wanted to make was to follow C++ Core Guidelines.

To show the new version I added is indeed equivalent, I added an assertion to compare the result coordinates the ULP tolerance.

Using ULP in comparison is the mistake here:

The two versions are equivalent, but not at the binary level; For Example:
Original version: y = -y + Height - yOffset;
New version:      y = y + yOffset;  y = Height - y;
                  i.e.: y = - y - yOffset  + Height;


If we modify the original version to: y = - y - yOffset  + Height; (from - y  + Height - yOffset ;), those two versions are equivalent at the binary level.

@sand1024,

The main points of the changes I wanted to raise about the non-rules:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-not

Before following the "should rules", please first consider to avoid following non-rules.

Mostly, those non-rules are favored by some old projects, but we have to avoid. Please think about the possibility of allowing new developers with quickly understand the code, most likely only the relevant portion, and to be able to modify the code with confidence.

Also, our code should be under constant code review and optimization.

emanuel wrote

when drawing a line starting on the word "draft"

The main point regarding rules I'd like to express - is that they are not a "silver bullet" or set in stone, requiring blind adherence.

Instead, it is important to consider their applicability to each specific situation. Any design pattern (such as RAII, singleton, etc.) should be used only when it makes sense and provides practical benefits.

For almost any rule, counterarguments exist, which makes finding the right balance between code readability, ease of maintenance, and performance essential. Trade-offs are always necessary.

For example, here are some illustrations:

NR.1: Don’t insist that all declarations must be at the top of a function – this can lead to repetitive code when accessing a variable’s value (e.g., foo()->bar() instead of creating a variable var = foo()->bar() and using var later).

NR.2: Don’t insist on having only one return statement in a function – this can make step-through debugging more difficult.

NR.5: Don’t use two-phase initialization – while this is generally a good practice, it may not be possible when an object needs to be created first and initialized later.

NR.7: Don’t make all data members protected – this is acceptable, but only if inheritance from the class is not needed. It works well if the class is not part of a library and can be modified. Protected getter/setter methods for private fields can also be used, but this may not always be necessary.

Design patterns and guidelines are beneficial, but blindly following them without considering their applicability to the situation is problematic if:

The pattern reduces code readability
It complicates maintenance and modification
It introduces unnecessary overhead
It negatively impacts performance

In all cases, the balance between priorities must be carefully considered. In some situations, readability may be the most important factor; in others, performance or minimizing overhead may take precedence.

So it's just a question of balance of priorities.

@sand1024,

arguing is not the way to address this issue, and it's more about understanding, and that's why I suggested you to actually do profiling to avoid dictating based on incomplete understanding. If you feel an exception is needed, please present a specific example, as the guidelines make it clear:

You will find some of the rules contrary to your expectations or even contrary to your experience. If we haven’t suggested you change your coding style in any way, we have failed! Please try to verify or disprove rules!

Your arguments are actually covered by the Core Guidelines, if you read from its beginning. Those guidelines are developed by experienced users and should be followed with understanding.

Our goal is not only have a working codebase, also a codebase can be easily maintained by new developers, years later.