qnanopainter icon indicating copy to clipboard operation
qnanopainter copied to clipboard

qnanopainter screen location float --> "comparing floating point with == or != is unsafe."

Open NielsMayer opened this issue 6 years ago • 2 comments

As corollary to #31, since qnanopainter uses float to represent screen locations, what is a correct and resolution independent way of comparing screen locations and points without incurring the "comparing floating point with == or != is unsafe." warning?

For example, https://stackoverflow.com/a/10334868 suggests the "epsilon" for an equality check should depend on what would be visually significant on a given display.

Perhaps QNanoPainter needs a primitive similar to CocoaTouch's CGPointEqualToPoint() as suggested here: https://stackoverflow.com/a/16418684 which ostensibly calculates the "epsilon" for point comparison based on current display properties?

NielsMayer avatar Sep 25 '18 23:09 NielsMayer

Yes, QNanoPainter uses floats in API because NanoVG underneath also uses floats.

Qt contains qFuzzyCompare http://doc.qt.io/qt-5/qtglobal.html#qFuzzyCompare-1 and qFuzzyIsNull http://doc.qt.io/qt-5/qtglobal.html#qFuzzyIsNull-1 for floating point comparison. Those should be used in own code when comparing float equality, but many use cases can be covered also with "<=" and ">=" instead of strict equality.

QUItCoding avatar Sep 26 '18 04:09 QUItCoding

On a related note, I'm trying out QtCreator 5.12.1 Analyzer->"Clang-Tidy and Clazy" functionality and it is giving the following suggestions for qnanopainter.cpp:

: warning: casting (double + 0.5) to integer leads to incorrect rounding; consider using lround (#include <cmath>) instead [bugprone-incorrect-roundings]

For the following:

// Align to full pixel
void QNanoPainter::_checkAlignPixels(float *a, float *b, float *c, float *d) {
    if (m_pixelAlign) {
        if (a) *a = (static_cast<int>(*a * m_devicePixelRatio + 0.5f)) / m_devicePixelRatio;
        if (b) *b = (static_cast<int>(*b * m_devicePixelRatio + 0.5f)) / m_devicePixelRatio;
        if (c) *c = (static_cast<int>(*c * m_devicePixelRatio + 0.5f)) / m_devicePixelRatio;
        if (d) *d = (static_cast<int>(*d * m_devicePixelRatio + 0.5f)) / m_devicePixelRatio;
    }
}

// Align text to full pixel
void QNanoPainter::_checkAlignPixelsText(float *a, float *b) {
    if (m_pixelAlignText) {
        if (a) *a = (static_cast<int>(*a * m_devicePixelRatio + 0.5f)) / m_devicePixelRatio;
        if (b) *b = (static_cast<int>(*b * m_devicePixelRatio + 0.5f)) / m_devicePixelRatio;
    }
}

NB: "Clang-Tidy and Clazy" analyzer is finding 259 "issues" in libqnanopainter. Note that it can fix many issues automatically, but I didn't want to be working off a forked libqnanopainter... I did let it fix my code and implemented a few of the suggestions, and my code still works.

NielsMayer avatar Feb 10 '19 19:02 NielsMayer