From e6324e86ac7e5be003791829d41c586242a5d90f Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sat, 17 Nov 2018 09:25:26 -0500 Subject: [PATCH] Fix cross-platform one-pixel differences in line drawing... ... Make all line drawing go through our AColor which wraps the wxDC line-drawing functions. Despite what wxWidgets documentation says, the functions still don't consistently include the first point and omit the last point of a line. I observed inclusion of both on Mac, while on Windows, omission sometimes of the first point instead of the last. Discrepancies could be observed in 2.3.0 between Windows and Mac, using the Window magnifier or command+alt+ +, zooming in closely on the ends of the shadow lines below and right of tracks, or at the bottom-right corners of the bevels drawn around vertical rulers. So where there is an observable one-pixel difference of drawing between platforms, there is the question, which was the intent when the drawing code was written? Should the coordinates be corrected by one or not? I reviewed each case and used my judgment. Most of the calls are in drawing rulers. Major tick lines were drawn five pixels long, and minor, three, on Mac. I keep this behavior, but you might argue for making them four and two. On the other hand the drawing of ruler grid lines, which you can see in the equalization and frequency analysis windows, did need a one-pixel correction to avoid straying out of bounds. --- src/AColor.cpp | 77 ++++++++++++++++++++------------------- src/AColor.h | 7 ++++ src/AdornedRulerPanel.cpp | 4 +- src/effects/AutoDuck.cpp | 2 +- src/widgets/KeyView.cpp | 4 +- src/widgets/Meter.cpp | 6 +-- src/widgets/Ruler.cpp | 47 ++++++++++++------------ 7 files changed, 79 insertions(+), 68 deletions(-) diff --git a/src/AColor.cpp b/src/AColor.cpp index 4e04b217c..01c27cc90 100644 --- a/src/AColor.cpp +++ b/src/AColor.cpp @@ -117,51 +117,51 @@ void AColor::Arrow(wxDC & dc, wxCoord x, wxCoord y, int width, bool down) } // -// Draw a line while accounting for differences in wxWidgets versions +// Draw a line, inclusive of endpoints, +// compensating for differences in wxWidgets versions across platforms // void AColor::Line(wxDC & dc, wxCoord x1, wxCoord y1, wxCoord x2, wxCoord y2) { - // As of 2.8.9 (possibly earlier), wxDC::DrawLine() on the Mac draws the - // last point since it is now based on the NEW wxGraphicsContext system. - // Make the other platforms do the same thing since the other platforms - // "may" follow they get wxGraphicsContext going. + const wxPoint points[] { { x1, y1 }, { x2, y2 } }; + Lines( dc, 2, points ); +} + +// Draw lines, INCLUSIVE of all endpoints +void AColor::Lines(wxDC &dc, size_t nPoints, const wxPoint points[]) +{ + if ( nPoints <= 1 ) { + if (nPoints == 1) + dc.DrawPoint( points[0] ); + return; + } + + for (size_t ii = 0; ii < nPoints - 1; ++ii) { + const auto &p1 = points[ii]; + const auto &p2 = points[ii + 1]; + + // As of 2.8.9 (possibly earlier), wxDC::DrawLine() on the Mac draws the + // last point since it is now based on the NEW wxGraphicsContext system. + // Make the other platforms do the same thing since the other platforms + // "may" follow they get wxGraphicsContext going. + + // PRL: as of 3.1.1, I still observe that on Mac, the last point is + // included, contrary to what documentation says. Also that on Windows, + // sometimes it is the first point that is excluded. + #if defined(__WXMAC__) || defined(__WXGTK3__) - dc.DrawLine(x1, y1, x2, y2); + dc.DrawLine(p1, p2); #else - bool point = false; - - if (x1 == x2) { - if (y1 < y2) { - y2++; + dc.DrawPoint(p1); + if ( p1 != p2 ) { + dc.DrawLine(p1, p2); } - else if (y2 < y1) { - y1++; - } - else { - point = true; - } - } - else if (y1 == y2) { - if (x1 < x2) { - x2++; - } - else if (x2 < x1) { - x1++; - } - else { - point = true; - } - } - else { - dc.DrawPoint(x2, y2); +#endif } - if (point) { - dc.DrawPoint(x2, y2); - } - else { - dc.DrawLine(x1, y1, x2, y2); - } +#if defined(__WXMAC__) || defined(__WXGTK3__) + ; +#else + dc.DrawPoint( points[ nPoints - 1 ] ); #endif } @@ -264,6 +264,9 @@ void AColor::BevelTrackInfo(wxDC & dc, bool up, const wxRect & r, bool highlight #ifndef EXPERIMENTAL_THEMING Bevel( dc, up, r ); #else + // Note that the actually drawn rectangle extends one pixel right of and + // below the given + wxColour col; col = Blend( theTheme.Colour( clrTrackInfo ), up ? wxColour( 255,255,255):wxColour(0,0,0)); diff --git a/src/AColor.h b/src/AColor.h index 2b47ea935..b1f8f4bc2 100644 --- a/src/AColor.h +++ b/src/AColor.h @@ -63,7 +63,14 @@ class AColor { static void ReInit(); static void Arrow(wxDC & dc, wxCoord x, wxCoord y, int width, bool down = true); + + // Draw a line, INCLUSIVE of both endpoints + // (unlike what wxDC::DrawLine() documentation specifies) static void Line(wxDC & dc, wxCoord x1, wxCoord y1, wxCoord x2, wxCoord y2); + + // Draw lines, INCLUSIVE of all endpoints + static void Lines(wxDC &dc, size_t nPoints, const wxPoint points[]); + static void DrawFocus(wxDC & dc, wxRect & r); static void Bevel(wxDC & dc, bool up, const wxRect & r); static void Bevel2 diff --git a/src/AdornedRulerPanel.cpp b/src/AdornedRulerPanel.cpp index faffa3301..136fe911f 100644 --- a/src/AdornedRulerPanel.cpp +++ b/src/AdornedRulerPanel.cpp @@ -1913,9 +1913,9 @@ void AdornedRulerPanel::DoDrawEdge(wxDC *dc) // Black stroke at bottom dc->SetPen( *wxBLACK_PEN ); - dc->DrawLine( mOuter.x, + AColor::Line( *dc, mOuter.x, mOuter.y + mOuter.height - 1, - mOuter.x + mOuter.width, + mOuter.x + mOuter.width - 1 , mOuter.y + mOuter.height - 1 ); } diff --git a/src/effects/AutoDuck.cpp b/src/effects/AutoDuck.cpp index d3eb80062..c7313d3f6 100644 --- a/src/effects/AutoDuck.cpp +++ b/src/effects/AutoDuck.cpp @@ -715,7 +715,7 @@ void EffectAutoDuckPanel::OnPaint(wxPaintEvent & WXUNUSED(evt)) points[5].x = clientWidth - 10; points[5].y = DUCK_AMOUNT_START; - dc.DrawLines(6, points); + AColor::Lines(dc, 6, points); dc.SetPen(wxPen(*wxBLACK, 1, wxPENSTYLE_DOT)); diff --git a/src/widgets/KeyView.cpp b/src/widgets/KeyView.cpp index edc820f7a..4b252ef36 100644 --- a/src/widgets/KeyView.cpp +++ b/src/widgets/KeyView.cpp @@ -1102,9 +1102,9 @@ KeyView::OnDrawBackground(wxDC & dc, const wxRect & rect, size_t line) const { // Non-selected lines get a thin bottom border dc.SetPen(wxColour(240, 240, 240)); - dc.DrawLine(r.GetLeft(), r.GetBottom(), r.GetRight(), r.GetBottom()); + AColor::Line(dc, r.GetLeft(), r.GetBottom(), r.GetRight(), r.GetBottom()); if (mViewType == ViewByTree ) - dc.DrawLine(r2.GetLeft(), r2.GetBottom(), r2.GetRight(), r2.GetBottom()); + AColor::Line(dc, r2.GetLeft(), r2.GetBottom(), r2.GetRight(), r2.GetBottom()); } } diff --git a/src/widgets/Meter.cpp b/src/widgets/Meter.cpp index aed451a39..94979d5c1 100644 --- a/src/widgets/Meter.cpp +++ b/src/widgets/Meter.cpp @@ -523,13 +523,13 @@ void MeterPanel::OnPaint(wxPaintEvent & WXUNUSED(event)) { // 2 pixel spacing between the LEDs if( (i%7)<2 ){ - dc.DrawLine( i+r.x, r.y, i+r.x, r.y+r.height ); + AColor::Line( dc, i+r.x, r.y, i+r.x, r.y+r.height ); } else { // The LEDs have triangular ends. // This code shapes the ends. int j = abs( (i%7)-4); - dc.DrawLine( i+r.x, r.y, i+r.x, r.y+j +1); - dc.DrawLine( i+r.x, r.y+r.height-j, i+r.x, r.y+r.height ); + AColor::Line( dc, i+r.x, r.y, i+r.x, r.y+j +1); + AColor::Line( dc, i+r.x, r.y+r.height-j, i+r.x, r.y+r.height ); } } } diff --git a/src/widgets/Ruler.cpp b/src/widgets/Ruler.cpp index 2114970fe..d2b8d7947 100644 --- a/src/widgets/Ruler.cpp +++ b/src/widgets/Ruler.cpp @@ -58,6 +58,7 @@ array of Ruler::Label. #include "Ruler.h" +#include "../AColor.h" #include "../AllThemeResources.h" #include "../NumberScale.h" #include "../Theme.h" @@ -1307,19 +1308,19 @@ void Ruler::Draw(wxDC& dc, const TimeTrack* timetrack) { if (mOrientation == wxHORIZONTAL) { if (mFlip) - mDC->DrawLine(mLeft, mTop, mRight+1, mTop); + AColor::Line(*mDC, mLeft, mTop, mRight, mTop); else - mDC->DrawLine(mLeft, mBottom, mRight+1, mBottom); + AColor::Line(*mDC, mLeft, mBottom, mRight, mBottom); } else { if (mFlip) - mDC->DrawLine(mLeft, mTop, mLeft, mBottom+1); + AColor::Line(*mDC, mLeft, mTop, mLeft, mBottom); else { // These calculations appear to be wrong, and to never have been used (so not tested) prior to MixerBoard. - // mDC->DrawLine(mRect.x-mRect.width, mTop, mRect.x-mRect.width, mBottom+1); + // AColor::Line(*mDC, mRect.x-mRect.width, mTop, mRect.x-mRect.width, mBottom); const int nLineX = mRight - 1; - mDC->DrawLine(nLineX, mTop, nLineX, mBottom+1); + AColor::Line(*mDC, nLineX, mTop, nLineX, mBottom); } } } @@ -1341,18 +1342,18 @@ void Ruler::Draw(wxDC& dc, const TimeTrack* timetrack) { if (mOrientation == wxHORIZONTAL) { if (mFlip) - mDC->DrawLine(mLeft + pos, mTop, + AColor::Line(*mDC, mLeft + pos, mTop, mLeft + pos, mTop + 4); else - mDC->DrawLine(mLeft + pos, mBottom - 4, + AColor::Line(*mDC, mLeft + pos, mBottom - 4, mLeft + pos, mBottom); } else { if (mFlip) - mDC->DrawLine(mLeft, mTop + pos, + AColor::Line(*mDC, mLeft, mTop + pos, mLeft + 4, mTop + pos); else - mDC->DrawLine(mRight - 4, mTop + pos, + AColor::Line(*mDC, mRight - 4, mTop + pos, mRight, mTop + pos); } } @@ -1369,19 +1370,19 @@ void Ruler::Draw(wxDC& dc, const TimeTrack* timetrack) if (mOrientation == wxHORIZONTAL) { if (mFlip) - mDC->DrawLine(mLeft + pos, mTop, + AColor::Line(*mDC, mLeft + pos, mTop, mLeft + pos, mTop + 2); else - mDC->DrawLine(mLeft + pos, mBottom - 2, + AColor::Line(*mDC, mLeft + pos, mBottom - 2, mLeft + pos, mBottom); } else { if (mFlip) - mDC->DrawLine(mLeft, mTop + pos, + AColor::Line(*mDC, mLeft, mTop + pos, mLeft + 2, mTop + pos); else - mDC->DrawLine(mRight - 2, mTop + pos, + AColor::Line(*mDC, mRight - 2, mTop + pos, mRight, mTop + pos); } } @@ -1401,19 +1402,19 @@ void Ruler::Draw(wxDC& dc, const TimeTrack* timetrack) if (mOrientation == wxHORIZONTAL) { if (mFlip) - mDC->DrawLine(mLeft + pos, mTop, + AColor::Line(*mDC, mLeft + pos, mTop, mLeft + pos, mTop + 2); else - mDC->DrawLine(mLeft + pos, mBottom - 2, + AColor::Line(*mDC, mLeft + pos, mBottom - 2, mLeft + pos, mBottom); } else { if (mFlip) - mDC->DrawLine(mLeft, mTop + pos, + AColor::Line(*mDC, mLeft, mTop + pos, mLeft + 2, mTop + pos); else - mDC->DrawLine(mRight - 2, mTop + pos, + AColor::Line(*mDC, mRight - 2, mTop + pos, mRight, mTop + pos); } } @@ -1442,11 +1443,11 @@ void Ruler::DrawGrid(wxDC& dc, int length, bool minor, bool major, int xOffset, gridPos = mMinorLabels[i].pos; if(mOrientation == wxHORIZONTAL) { if((gridPos != 0) && (gridPos != mGridLineLength)) - mDC->DrawLine(gridPos+xOffset, yOffset, gridPos+xOffset, mGridLineLength+yOffset); + AColor::Line(*mDC, gridPos+xOffset, yOffset, gridPos+xOffset, mGridLineLength-1+yOffset); } else { if((gridPos != 0) && (gridPos != mGridLineLength)) - mDC->DrawLine(xOffset, gridPos+yOffset, mGridLineLength+xOffset, gridPos+yOffset); + AColor::Line(*mDC, xOffset, gridPos+yOffset, mGridLineLength-1+xOffset, gridPos+yOffset); } } } @@ -1458,11 +1459,11 @@ void Ruler::DrawGrid(wxDC& dc, int length, bool minor, bool major, int xOffset, gridPos = mMajorLabels[i].pos; if(mOrientation == wxHORIZONTAL) { if((gridPos != 0) && (gridPos != mGridLineLength)) - mDC->DrawLine(gridPos+xOffset, yOffset, gridPos+xOffset, mGridLineLength+yOffset); + AColor::Line(*mDC, gridPos+xOffset, yOffset, gridPos+xOffset, mGridLineLength-1+yOffset); } else { if((gridPos != 0) && (gridPos != mGridLineLength)) - mDC->DrawLine(xOffset, gridPos+yOffset, mGridLineLength+xOffset, gridPos+yOffset); + AColor::Line(*mDC, xOffset, gridPos+yOffset, mGridLineLength-1+xOffset, gridPos+yOffset); } } @@ -1472,11 +1473,11 @@ void Ruler::DrawGrid(wxDC& dc, int length, bool minor, bool major, int xOffset, mDC->SetPen(*wxBLACK_PEN); if(mOrientation == wxHORIZONTAL) { if(zeroPosition != mGridLineLength) - mDC->DrawLine(zeroPosition+xOffset, yOffset, zeroPosition+xOffset, mGridLineLength+yOffset); + AColor::Line(*mDC, zeroPosition+xOffset, yOffset, zeroPosition+xOffset, mGridLineLength-1+yOffset); } else { if(zeroPosition != mGridLineLength) - mDC->DrawLine(xOffset, zeroPosition+yOffset, mGridLineLength+xOffset, zeroPosition+yOffset); + AColor::Line(*mDC, xOffset, zeroPosition+yOffset, mGridLineLength-1+xOffset, zeroPosition+yOffset); } } }