wxRendererNative draws at wrong position on wxGCDC in wxScrolled (Issue #25966)
rudolfwalter
rudolfwalter created an issue (wxWidgets/wxWidgets#25966)Description
When drawing using wxRendererNative onto the wxGCDC of a wxScrolled, the drawing ends up in the wrong position after scrolling, and gets truncated.
Repro:
diff --git a/samples/scroll/scroll.cpp b/samples/scroll/scroll.cpp index c709332286..65eea1c2f6 100644 --- a/samples/scroll/scroll.cpp +++ b/samples/scroll/scroll.cpp @@ -14,6 +14,8 @@ #include "wx/wx.h" #endif +#include "wx/dcgraph.h" +#include "wx/renderer.h" #include "wx/sizer.h" #include "wx/log.h" #include "wx/tglbtn.h" @@ -52,7 +54,9 @@ public: private: void OnPaint(wxPaintEvent& WXUNUSED(event)) { - wxPaintDC dc(this); + wxPaintDC paintDC(this); + wxGraphicsContext* gc = wxGraphicsRenderer::GetDefaultRenderer()->CreateContext(paintDC); + wxGCDC dc(gc); // this call is vital: it adjusts the dc to account for the current // scroll offset @@ -61,6 +65,15 @@ private: dc.SetPen( *wxRED_PEN ); dc.SetBrush( *wxTRANSPARENT_BRUSH ); dc.DrawRectangle( 0, 0, WIDTH, HEIGHT ); + + constexpr int Y1 = 40, Y2 = 80, X1 = 50, X2 = 90; + + dc.SetPen(*wxBLUE_PEN); + dc.DrawLine(X1, Y1, X2, Y1); + dc.DrawLine(X1, Y2, X2, Y2); + + wxRendererNative::Get().DrawTreeItemButton(this, dc, wxRect(X1 - 30, Y1 - 8, 16, 16), 0); + wxRendererNative::Get().DrawCheckBox(this, dc, wxRect(X2 + 20, Y2 - 12, 24, 24), 0); } };
Correct result, before scrolling:
image.png (view on web)Wrong result, after scrolling:
image.png (view on web)Platform and version information
- wxWidgets version: both 3.2.2.1 and 3.3/master
- wxWidgets port: wxMSW
- OS: Windows 11 24H2
- other: 150% DPI, light mode
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)I do see this, thanks, will try to have a look. It seems to be MSW-specific as this works correctly in wxGTK.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)The linked PR fixes the provided example, please check if it works with your actual code. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
rudolfwalter
rudolfwalter left a comment (wxWidgets/wxWidgets#25966)The PR does seem to fix the issue with the default renderer, however the Direct2D renderer produces duplicate images when scrolling down. (Change GetDefaultRenderer() to GetDirect2DRenderer() in the repro code to see this.)
image.png (view on web)—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)I see this, thanks, but I don't see why does this happen. Transform matrix related code in D2D renderer is rather complex and I'm not sure if the problem is in it or something else. I thought D2D could somehow apply the transform to the native HDC we get from it, but inserting
diff --git a/src/common/dcgraph.cpp b/src/common/dcgraph.cpp index 0604b1063d..0fa7edcafd 100644 --- a/src/common/dcgraph.cpp +++ b/src/common/dcgraph.cpp @@ -119,7 +119,15 @@ WXHDC wxGCDC::AcquireHDC() { wxGraphicsContext* const gc = GetGraphicsContext(); wxCHECK_MSG(gc, nullptr, "can't acquire HDC because there is no wxGraphicsContext"); - return gc->GetNativeHDC(); + WXHDC hdc = gc->GetNativeHDC(); + + POINT ptViewOrg, ptWinOrg; + GetViewportOrgEx(hdc, &ptViewOrg); + GetWindowOrgEx(hdc, &ptWinOrg); + wxLogDebug("wxGCDC::AcquireHDC: ViewportOrg=(%d,%d), WindowOrg=(%d,%d)", + ptViewOrg.x, ptViewOrg.y, ptWinOrg.x, ptWinOrg.y); + + return hdc; } void wxGCDC::ReleaseHDC(WXHDC hdc)
shows that the origin is always 0, so that's not it either. I'm afraid I'll have to abandon this for now as I don't have any other ideas.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
rudolfwalter
rudolfwalter left a comment (wxWidgets/wxWidgets#25966)That's understandable; thanks for your investigation.
I tinkered a bit more with it, so I'll write what I found for future reference.
I suspect this is some form of a buffering/caching issue (rather than a drawing or coordinates issue), because at specific scroll amounts the drawing actually shows up twice:
image.png (view on web)Also, calling Refresh() makes the wrong ones disappear, and calling SetDoubleBuffered(true) makes the problem go away altogether.
So from my perspective your PR is good to be merged; I'll use SetDoubleBuffered(true) to work around the rest.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)Thanks, I'll keep this open with the updated title because the problem is still there.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25966)The problem seems to be caused by GetClipBox() returning invalid clip rectangle after scrolling:
https://github.com/wxWidgets/wxWidgets/blob/f57b17ba5f63db53dd1b465646ade105fee22302/src/msw/graphicsd2d.cpp#L3759-L3764
The initial clip rectangle is correct, though.
This patch fixes the problem for me:
diff --git a/src/msw/graphicsd2d.cpp b/src/msw/graphicsd2d.cpp index 2d7a74643c..08f5e2b3fb 100644 --- a/src/msw/graphicsd2d.cpp +++ b/src/msw/graphicsd2d.cpp @@ -3756,12 +3756,21 @@ protected: &renderTarget); wxCHECK_HRESULT_RET(hr); - // We want draw on the entire device area. - // GetClipBox() retrieves logical size of DC - // what is what we need to pass to BindDC. RECT r; - int status = ::GetClipBox(m_hdc, &r); - wxCHECK_RET( status != ERROR, wxS("Error retrieving DC dimensions") ); + + HWND hWnd = ::WindowFromDC(m_hdc); + if (hWnd) + { + ::GetClientRect(hWnd, &r); + } + else + { + // We want draw on the entire device area. + // GetClipBox() retrieves logical size of DC + // what is what we need to pass to BindDC. + int status = ::GetClipBox(m_hdc, &r); + wxCHECK_RET( status != ERROR, wxS("Error retrieving DC dimensions") ); + } hr = renderTarget->BindDC(m_hdc, &r); if (FAILED(hr))
But I still think we should fix GetClipBox() for a scrolled DC!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)Ah, great find, thanks!
Do we simply need to account for the scroll offset ourselves to "fix" GetClipBox()? I.e. what do you mean by "invalid rect", is it just off or really invalid (empty)?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25966)Ah, great find, thanks!
Do we simply need to account for the scroll offset ourselves to "fix"
GetClipBox()? I.e. what do you mean by "invalid rect", is it just off or really invalid (empty)?
I've applied a slightly modified patch of the original and run it under the debugger:
diff --git a/samples/scroll/scroll.cpp b/samples/scroll/scroll.cpp
index c709332286..76fb211c4f 100644--- a/samples/scroll/scroll.cpp +++ b/samples/scroll/scroll.cpp @@ -14,6 +14,8 @@ #include "wx/wx.h" #endif +#include "wx/dcgraph.h" +#include "wx/renderer.h" #include "wx/sizer.h" #include "wx/log.h" #include "wx/tglbtn.h"
@@ -52,15 +54,31 @@ public:
private: void OnPaint(wxPaintEvent& WXUNUSED(event)) { - wxPaintDC dc(this); + wxPaintDC paintDC(this
); + wxGraphicsContext* gc = wxGraphicsRenderer::GetDirect2DRenderer()->CreateContext(paintDC); + wxGCDC dc(gc);
// this call is vital: it adjusts the dc to account for the current // scroll offset
PrepareDC(dc);
+ wxRect clipRect;
+ dc.GetClippingBox(clipRect);
+ wxLogDebug("clipRect (%d, %d, %d, %d)",
+ clipRect.x, clipRect.y, clipRect.width, clipRect.height);
+
dc.SetPen( *wxRED_PEN );
dc.SetBrush( *wxTRANSPARENT_BRUSH );
dc.DrawRectangle( 0, 0, WIDTH, HEIGHT );
+
+ constexpr int Y1 = 40, Y2 = 80, X1 = 50, X2 = 90;
+
+ dc.SetPen(*wxBLUE_PEN);
+ dc.DrawLine(X1, Y1, X2, Y1);
+ dc.DrawLine(X1, Y2, X2, Y2);
+
+ wxRendererNative::Get().DrawTreeItemButton(this, dc, wxRect(X1 - 30, Y1 - 8, 16, 16), 0);
+ wxRendererNative::Get().DrawCheckBox(this, dc, wxRect(X2 + 20, Y2 + 120, 24, 24), 0);
}
};
diff --git a/src/msw/graphicsd2d.cpp b/src/msw/graphicsd2d.cpp
index 2d7a74643c..3e50d4870b 100644
--- a/src/msw/graphicsd2d.cpp
+++ b/src/msw/graphicsd2d.cpp
@@ -3756,12 +3756,26 @@ protected:
&renderTarget);
wxCHECK_HRESULT_RET(hr);
- // We want draw on the entire device area.
- // GetClipBox() retrieves logical size of DC
- // what is what we need to pass to BindDC.
RECT r;
- int status = ::GetClipBox(m_hdc, &r);
- wxCHECK_RET( status != ERROR, wxS("Error retrieving DC dimensions") ); +#if 0
+ HWND hWnd = ::WindowFromDC(m_hdc); + if (hWnd) + { + ::GetClientRect(hWnd, &r); + } + else
+#endif
+ {
+ // We want draw on the entire device area.+ // GetClipBox() retrieves logical size of DC + // what is what we need to pass to BindDC. + int status = ::GetClipBox(m_hdc, &r); + wxCHECK_RET( status != ERROR, wxS("Error retrieving DC dimensions"
) ); + } + + wxLogDebug("r (%d, %d, %d, %d)", + r.left, r.top, + r.right-r.left, r.bottom-r.top); hr = renderTarget->BindDC(m_hdc, &r); if (FAILED(hr))
I noticed that clipRect is correct after scrolling whereas the rectangle returned by ::GetClipBox() is not!!
Unfortunately, I don't know why ::GetClipBox() behaves like this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25966)After thinking more about this, my conclusion, IMHO, is : the patch in #25966 (comment) is the right thing to do, i.e.: pass the client rectangle (returned by GetClientRect()) to BindDC() if we are drawing on a DC bound to a window because GetClipBox() returns the intersection of:
- visible region
- current clip region
- update region (during
WM_PAINT)
And because we are inside a paint handler, GetClipBox() returns only the bounding rectangle of the invalid region, not the entire device area, and this is what I'm seeing in the scroll sample.
Am I missing something here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)Sorry, do you mean that the update region has been already validated by the time GetClipBox() is called? If so, this would indeed explain it, but I don't see where does it happen...
But regardless of this, I think using the full size should indeed be fine and we should do it to fix the bug. Could we use wxDC::GetSize() instead of checking for the window? Or just do what wxMSWDCImpl::GetDeviceSize() if all we have here is an HDC.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25966)Sorry, do you mean that the update region has been already validated by the time
GetClipBox()is called? If so, this would indeed explain it, but I don't see where does it happen...
I think so, and I think this is done by Windows internally because the rectangle returned by GetClipBox() after scrolling is exactly the same as rcUpdate rectangle returned by ScrollWindowEx()
diff --git a/src/msw/window.cpp b/src/msw/window.cpp index 9632bd1a1c..05ed375e98 100644 --- a/src/msw/window.cpp +++ b/src/msw/window.cpp @@ -1154,7 +1154,12 @@ void wxWindowMSW::ScrollWindow(int dx, int dy, const wxRect *prect) } - ::ScrollWindow(GetHwnd(), dx, dy, pr, pr); + RECT rcUpdate; + ::ScrollWindowEx(GetHwnd(), dx, dy, pr, pr, nullptr, &rcUpdate, SW_INVALIDATE|SW_ERASE); + + wxLogMessage("rcUpdate (%d, %d, %d, %d)", + rcUpdate.left, rcUpdate.top, + rcUpdate.right-rcUpdate.left, rcUpdate.bottom-rcUpdate.top); } static bool ScrollVertically(HWND hwnd, int kind, int count)
Quoting MSDN:
prcUpdate: Pointer to a RECT structure that receives the boundaries of the rectangle invalidated by scrolling.
But regardless of this, I think using the full size should indeed be fine and we should do it to fix the bug. Could we use
wxDC::GetSize()instead of checking for the window? Or just do whatwxMSWDCImpl::GetDeviceSize()if all we have here is anHDC.
Yes, passing wxDC::GetSize() to wxD2DDCRenderTargetResourceHolder works fine. And I prefer it over wxMSWDCImpl::GetDeviceSize() because the size returned by the latter (to create the internal bitmap) may be unnecessarily too large compared to the former, and from MSDN:
When you use an ID2D1DCRenderTarget, it renders Direct2D content to an internal bitmap, and then renders the bitmap to the DC with GDI.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25966)Yes, passing
wxDC::GetSize()towxD2DDCRenderTargetResourceHolderworks fine.
Will you make a PR with this change or should I?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25966)Yes, passing
wxDC::GetSize()towxD2DDCRenderTargetResourceHolderworks fine.Will you make a PR with this change or should I?
Sure, I'll prepare a PR and push it soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25966)@vadz Sorry for not making a PR but with the following patch the bug desappears:
diff --git a/src/msw/graphicsd2d.cpp b/src/msw/graphicsd2d.cpp index 2d7a74643c..04e0139d97 100644 --- a/src/msw/graphicsd2d.cpp +++ b/src/msw/graphicsd2d.cpp @@ -3763,6 +3763,8 @@ protected: int status = ::GetClipBox(m_hdc, &r);
wxCHECK_RET( status != ERROR, wxS("Error retrieving DC dimensions"
) ); + r.left = r.top = 0; + hr = renderTarget->BindDC(m_hdc, &r); if (FAILED(hr)) { @@ -3771,9 +3773,6 @@ protected: return; } - renderTarget->SetTransform( - D2D1::Matrix3x2F::Translation(-r.left, -r.top)); - m_nativeResource = renderTarget; } @@ -4136,9 +4140,13 @@ wxD2DContext::wxD2DContext(wxGraphicsRenderer* renderer, , m_direct2dFactory(direct2dFactory) , m_renderTargetHolder(new wxD2DDCRenderTargetResourceHolder(direct2dFactory, dc.GetHDC(), alphaMode)) { - const wxSize dcSize = dc.GetSize(); - m_width = dcSize.GetWidth(); - m_height = dcSize.GetHeight(); + RECT r; + if ( ::GetClipBox(dc.GetHDC(), &r) == ERROR ) + { + wxLogLastError("GetClipBox"); + } + m_width = r.right - r.left; + m_height = r.bottom - r.top; // We don't set HDC origin at MSW level in wxDC because this limits it to // 2^27 range and we prefer to handle it ourselves to allow using the full
IOW, the only important information of the rectangle passed to BindDC() is its size. the offset is already stored in m_inheritedTransform in the constructor. Please apply it if it looks correct to you.
Here is another modified patch for testing:
diff --git a/samples/scroll/scroll.cpp b/samples/scroll/scroll.cpp
index c709332286..00fd1947f2 100644--- a/samples/scroll/scroll.cpp +++ b/samples/scroll/scroll.cpp @@ -14,6 +14,8 @@ #include "wx/wx.h" #endif +#include "wx/dcgraph.h" +#include "wx/renderer.h" #include "wx/sizer.h" #include "wx/log.h" #include "wx/tglbtn.h"
@@ -52,15 +54,32 @@ public:
private: void OnPaint(wxPaintEvent& WXUNUSED(event)) { - wxPaintDC dc(this); + wxPaintDC paintDC(this
); + wxGraphicsContext* gc = wxGraphicsRenderer::GetDirect2DRenderer()->CreateContext(paintDC); + wxGCDC dc(gc);
// this call is vital: it adjusts the dc to account for the current // scroll offset
PrepareDC(dc);
+ dc.SetFont( *wxSWISS_FONT );
+ dc.DrawText("This is a header", 100, 5);
+
dc.SetPen( *wxRED_PEN );
dc.SetBrush( *wxTRANSPARENT_BRUSH );
dc.DrawRectangle( 0, 0, WIDTH, HEIGHT );
+
+ constexpr int Y1 = 40, Y2 = 80, X1 = 50, X2 = 90;
+
+ dc.SetPen(*wxBLUE_PEN);
+ dc.DrawLine(X1, Y1, X2, Y1);
+ dc.DrawLine(X1, Y2, X2, Y2);
+
+ wxRendererNative::Get().DrawTreeItemButton(this, dc, wxRect(X1 - 30, Y1 - 8, 16, 16), 0);
+ wxRendererNative::Get().DrawCheckBox(this, dc, wxRect(X2 + 20, Y2 + 120, 24, 24), 0);
+
+ dc.SetPen( *wxGREEN_PEN );
+ dc.DrawText("This is a footer", 100, 250);
}
};
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.