Use correct wxBitmap size for STC (PR #22465)

Maarten

unread,

May 27, 2022, 2:06:15 PM5/27/22

to wx-...@googlegroups.com, Subscribed

This fixes rendering artifacts.

On Windows, the bitmap should have the width and height as provided to InitPixMap(), and initialized with the correct scale factor to prevent performance regressions. CreateWithDIPSize() in combination with ToDIP(width, height) cannot be used, because the scaled size will be rounded and can cause the final bitmap to become too large.

On macOS (retina) CreateWithDIPSize has to be used, so the internal bitmap context has the correct size and scale.

Remove obsolete mdc->GetImpl()->SetWindow(), this is not needed anymore because the DPI is now determined from the associated bitmap content scale factor, and not from the wxWindow.

See #22450

You can view, comment on, or merge this pull request online at:

https://github.com/wxWidgets/wxWidgets/pull/22465

Commit Summary

4ea0bd9 Use correct wxBitmap size for STC

File Changes

(1 file)

M src/stc/PlatWX.cpp (8)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

May 27, 2022, 7:14:59 PM5/27/22

to wx-...@googlegroups.com, Subscribed

Thanks, I guess we should apply this because it fixes a real problem, but it looks like we're still missing some API if we need to use preprocessor checks for what looks like a perfectly reasonable operation. Should we add some other wxBitmap::CreateXXX() for this and what should "XXX" be if we do?

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 5:30:04 AM5/28/22

to wx-...@googlegroups.com, Subscribed

It would have to be a function something like:

bool wxBitmap::CreateWithScaleFactors(const wxSize& size, double contentScale, double dpiScale, int depth)
{
    if ( !Create(size * contentScale, depth) )
        return false;

    GetBitmapData()->m_scaleFactor = dpiScale;

    return true;
}

With the current API, something like this also works:

    bitmap = new wxBitmap();
    bitmap->CreateWithDIPSize(width, height, GETWIN(winid)->GetContentScaleFactor());
    bitmap->SetScaleFactor(GETWIN(winid)->GetDPIScaleFactor());

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 28, 2022, 10:28:58 AM5/28/22

to wx-...@googlegroups.com, Subscribed

But do we really need to pass the content scale to this function? AFAICS it's always either the same as DPI scale or 1, depending on the platform. So I think it should be enough to pass just the DIP scale, but only multiply the size by it on the platforms where wxHAS_DPI_INDEPENDENT_PIXELS. But this makes it very similar to the existing CreateWithDIPSize() which, in turn, makes me wonder if the existing function could be just implemented wrongly? What is so different about its use in wxSTC code and in src/common/dcbufcmn.cpp, for example? Is wxBufferedDC also broken right now?

BTW, on a completely unrelated note, why do we allocate bitmap on the heap here? We should be just using a bitmap object and check for bitmap.IsOk() instead...

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 11:27:02 AM5/28/22

to wx-...@googlegroups.com, Subscribed

wxBufferedDC seems to works fine. The problem is that Scintilla expects an actual 8x8 bitmap. And it is not possible to create this at 175% scaling.

I'll commit another proposal, where the bitmap DIP size can be specified as wxRealPoint.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 12:49:38 PM5/28/22

to wx-...@googlegroups.com, Push

@MaartenBent pushed 2 commits.

0a85d77 Allow to use wxRealPoint size in CreateWithDIPSize
ffb1baf Use correct wxBitmap size for STC

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 12:57:40 PM5/28/22

to wx-...@googlegroups.com, Push

@MaartenBent pushed 2 commits.

f260907 Allow to use wxRealPoint size in CreateWithDIPSize
48d1f4b Use correct wxBitmap size for STC

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

May 28, 2022, 1:35:02 PM5/28/22

to wx-...@googlegroups.com, Subscribed

Thanks, I think this is definitely better than the first version, but -- and please forgive me for being so picky -- it still feels like something is not quite right if we have to divide by the scale just to ensure that multiplying by it later gives the expected result.

If we're always going to use wxRound(sz*scale) why can't we just pass win->ToPhys(sz) as the bitmap size (and then set its scale factor)? This would make sense to me: the size we have is in logical pixels (isn't it?) and we need to get the bitmap size, which is always in physical pixels, from it. So why do we really need to add the new function and deal with rounding here?

If anything, we might want to add wxBitmap::CreateWithPhysicalSizeAndScaleFactor() (abbreviated to something more reasonable), but if we consider this to be something only rarely needed, using the simple ctor/Create() taking physical size followed by SetScaleFactor() call should be fine too, AFAICS.

Or am I still 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.

Maarten

unread,

May 28, 2022, 1:58:46 PM5/28/22

to wx-...@googlegroups.com, Subscribed

If we're always going to use wxRound(sz*scale) why can't we just pass win->ToPhys(sz) as the bitmap size (and then set its scale factor)? This would make sense to me: the size we have is in logical pixels (isn't it?) and we need to get the bitmap size, which is always in physical pixels, from it. So why do we really need to add the new function and deal with rounding here?

If I understand correctly, you mean:

    wxSize size(wxMax(1, width), wxMax(1, height));
    bitmap = new wxBitmap(GETWIN(winid)->ToPhys(wxSize(width, height)));
    bitmap->SetScaleFactor(GETWIN(winid)->GetDPIScaleFactor());

On Windows this works, but not on macOS. You can't change the scale factor after the bitmap is created. Because SetScaleFactor() does not change CGContextScaleCTM in the internal wxBitmapRefData.
I'll try to fix this.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 2:12:46 PM5/28/22

to wx-...@googlegroups.com, Push

@MaartenBent pushed 2 commits.

5b78c09 Fix wxBitmap::SetScaleFactor on macOS
931c43a Use correct wxBitmap size for STC

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 2:14:11 PM5/28/22

to wx-...@googlegroups.com, Subscribed

I based wxBitmapRefData::SetScaleFactor on wxBitmapRefData::UseAlpha (it also calls CGContextScaleCTM). Maybe a macOS experts can confirm this is a correct solution or if something simpler is possible.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 28, 2022, 2:20:21 PM5/28/22

to wx-...@googlegroups.com, Subscribed

If I understand correctly, you mean:

    wxSize size(wxMax(1, width), wxMax(1, height));
    bitmap = new wxBitmap(GETWIN(winid)->ToPhys(wxSize(width, height)));
    bitmap->SetScaleFactor(GETWIN(winid)->GetDPIScaleFactor());

Yes, exactly!

On Windows this works, but not on macOS. You can't change the scale factor after the bitmap is created.

Ah, this is the part I was missing. We probably should add wxBitmap::CreateWithPhysSize(wxSize, double) to make it possible to set it directly when creating the bitmap as even with your changes changing it is not free, as it involves copying all the bitmap data.

Because SetScaleFactor() does not change CGContextScaleCTM in the internal wxBitmapRefData. I'll try to fix this.

But we still should apply your fix, thank you! I'd just add a note to the documentation saying that calling SetScaleFactor() can be expensive under at least macOS for big bitmaps. And, if we're really nitpicking, maybe we should refactor this function and UseAlpha() to extract this bitmap recreating code in some common helper?

Thanks again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 2:50:17 PM5/28/22

to wx-...@googlegroups.com, Subscribed

I found a solution that doesn't require copying the data. Undo the current scale, and then apply the new scale:

    CGContextScaleCTM( hBitmap, 1 / GetScaleFactor(), -1 / GetScaleFactor() );
    m_scaleFactor = scale;
    CGContextScaleCTM( hBitmap, GetScaleFactor(), -GetScaleFactor() );

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 3:52:37 PM5/28/22

to wx-...@googlegroups.com, Push

@MaartenBent pushed 2 commits.

4b7178e Fix wxBitmap::SetScaleFactor on macOS
d609f5b Use correct wxBitmap size for STC

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Maarten

unread,

May 28, 2022, 4:07:08 PM5/28/22

to wx-...@googlegroups.com, Push

@MaartenBent pushed 2 commits.

655afd6 Fix wxBitmap::SetScaleFactor on macOS
31de97a Use correct wxBitmap size for STC

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

May 28, 2022, 7:08:13 PM5/28/22

to wx-...@googlegroups.com, Subscribed

Great, this looks perfect now. I'm going to merge it (probably on Monday) if there are no objections, thanks again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 30, 2022, 11:58:41 AM5/30/22

to wx-...@googlegroups.com, Subscribed

Merged #22465 into master.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward