Improve wxBitmapBundle scaling logic and ensure it's consistent for files and MSW resources (PR #22481)
VZ
This is related to #22449, please see the discussion there for the full context, but the brief summary is that we now do our utmost to only upscale bitmaps by integer scale factors, as this is the only thing which works reasonably well (i.e. not horribly) in practice.
Please also check the new unit tests that describe in a hopefully relatively clear the expected behaviour (note that many of them would have failed without the rest of the changes here).
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22481
Commit Summary
- 78da0ee Refactor wxBitmapBundleImplSet::GetPreferredBitmapSizeAtScale()
- 028a126 Don't use generated bitmaps when looking for preferred scale
- e03297b Use the same preferred size algorithm in wxBitmapBundleImplRC
- 0b0e952 Upscale images by integer factors only
- 30fb836 Break the tie in favour of smaller bitmaps at 1.5x factor
- 3414e6a Extend wxBitmapBundle unit tests for bitmap scaling
- 2d78244 Choose the best bitmap to rescale to the target size
- 66d4f75 Access availableScales only sequentially in DoGetPreferredSize()
- 36abfe9 Change DoGetPreferredSize() to use a callback function
- 966898c Add wxBitmapBundleImpl::GetIndexToUpscale()
- b34f304 Use non-integer scale that is exact multiple of an available one
File Changes
(5 files)
- M include/wx/bmpbndl.h (23)
- M interface/wx/bmpbndl.h (107)
- M src/common/bmpbndl.cpp (208)
- M src/msw/bmpbndl.cpp (68)
- M tests/graphics/bmpbundle.cpp (260)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22481.patch
- https://github.com/wxWidgets/wxWidgets/pull/22481.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- 0e3ae90 fixup! Add wxBitmapBundleImpl::GetIndexToUpscale()
- 16c2c95 Use integer upscaling only in wxBitmapBundleImplArt
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 7fa8e01 REVERT: add debugging code to wxGTK2ArtProvider
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- 9e7480b Revert "REVERT: add debugging code to wxGTK2ArtProvider"
- 54bcec6 Fix size of bitmaps returned by wxDefaultArtProvider
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 8 commits.
- d86c1a8 Add wxBitmapBundleImpl::GetIndexToUpscale()
- 6d5bd15 Use non-integer scale that is exact multiple of an available one
- e9b3fda Use integer upscaling only in wxBitmapBundleImplArt
- 0bd2a6c Use correct default size in wxArtProvider::GetBitmapBundle()
- 86c366d Remove unnecessary code from wxArtProvider::CreateBitmap()
- 451e482 Remove useless CreateFromStdIcon() from wxMSW art provider
- 512d20c Fix confusion between width and height in wxArtProvider
- f56b8a9 Allow upscaling wxArtProvider bitmaps by integer scale factor
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vslavik You're going to laugh (well, I hope you aren't going to cry, which seems to be the only other sane reaction to my vacillations), but I think I do finally have my preferred solution to the wxBitmapBundleImplArt problem. It consists of several pieces, all of which are needed:
- As previously mentioned, use integer upscaling only (e9b3fda).
- Use the bitmap size but iff no explicit size was specified (0bd2a6c).
- Actually upscale bitmaps returned from
GetBitmap()(f56b8a9).
I believe that this results in the most reasonable behaviour and don't plan to make any other changes to this. Please let me know why I'm wrong... or I'm going to put this in 3.1.7 :-/
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 538eafc Fix bug with wrong GetIndexToUpscale() return value
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
@vslavik commented on this pull request.
Generally, this looks good to me - thanks a lot!
The biggest change that I can see in testing is that the breakages are different - a lot of very large bitmaps (because they didn't have scale factor set and the art provider returned them at 2x). That's good-ish, I think - it is very obvious where the issue is.
Less good is then I still see some non-integer scaling in Poedit - this is will the code updated to set the correct scale factor on its bitmaps:
- Running at 150%, all toolbar icons are slightly corrupted (the art provider has 1.5x versions). No idea why yet.
- Running at 175% (1.5x versions used), the previously observed problematic bitmap that is converted out of
wxBitmapBundleImplArtto the default impl, is still problematic and non-integer-scaled at 1.15 factor. There still is some difference between the two impls and I couldn't yet figure out what is going on.
> + if ( !entryToRescale )
+ entryToRescale = entryLastNotGen;
+
+ if ( entryToRescale )
+ {
+ const Entry entryNew(*entryToRescale, size);
+
+ m_entries.push_back(entryNew);
+
+ return entryNew.bitmap;
Is my understanding correct that this code does allow non-integer scaling, but it shouldn't happen because GetPreferredBitmapSizeAtScale() will only ever allow "good" sizes?
> bitmapbundle = wxBitmapBundle::FromImpl( - new wxBitmapBundleImplArt(id, client, bitmap.GetSize()) + new wxBitmapBundleImplArt( + id, + client, + size.IsFullySpecified() ? size + : bitmap.GetSize()
This should be GetDIPSize(), no?
Without it, I'm seeing twice-large toolbar buttons (but curiously, not anything else!) if the art provider returned 2x bitmap with correctly set scale factor.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
Less good is then I still see some non-integer scaling in Poedit - this is will the code updated to set the correct scale factor on its bitmaps:
OK, both of these are related to me changing GetSize() to GetDIPSize() as per the review comment. So it seems I was wrong about that. But without doing that, I don't know how should I return correctly sized bitmaps from wxArtProvider?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + if ( !entryToRescale )
+ entryToRescale = entryLastNotGen;
+
+ if ( entryToRescale )
+ {
+ const Entry entryNew(*entryToRescale, size);
+
+ m_entries.push_back(entryNew);
+
+ return entryNew.bitmap;
The contract of GetBitmap(size) is that it will return the bitmap of exactly this size. So if it's called with a size which can't be obtained by integer scaling, it will scale the bitmap using non-integer factor. I think it might be actually better to do what wxArtProvider::RescaleOrResizeIfNeeded() does in this case, see https://github.com/wxWidgets/wxWidgets/blob/73d425b34285b1ba150a1748c2b9146d58125e09/src/common/artprov.cpp#L327,L333 but it will still do something to ensure that the bitmap has the requested size.
However normally only integer scaling should be used because only the bitmaps of "preferred" size should be requested. There are some exceptions, however. The one I've already encountered is that wxToolBar with a fixed tool bitmap size will always request bitmaps of this size. I'm not sure what to do about this, so far I've documented (in a a not yet pushed commit) that using SetToolBitmapSize() is just not recommended and that you should let toolbar adapt to the size of the bitmaps it has.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> bitmapbundle = wxBitmapBundle::FromImpl( - new wxBitmapBundleImplArt(id, client, bitmap.GetSize()) + new wxBitmapBundleImplArt( + id, + client, + size.IsFullySpecified() ? size + : bitmap.GetSize()
Yes, thank you, you're right. But OTOH I don't expect many legacy art providers set any scale factor for the bitmaps they create, so this shouldn't matter.
But, again, I will still change this to GetDIPSize().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Less good is then I still see some non-integer scaling in Poedit - this is will the code updated to set the correct scale factor on its bitmaps:
OK, both of these are related to me changing GetSize() to GetDIPSize() as per the review comment. So it seems I was wrong about that.
No, I think you're right. But I don't understand why does this create problems in Poedit.
But without doing that, I don't know how should I return correctly sized bitmaps from wxArtProvider?
I think any really DPI-aware applications should override CreateBitmapBundle() and not CreateBitmap() anyhow. E.g. you can't distinguish between being asked for a normal bitmap of size 32 and a high resolution version of a bitmap of size 16 in CreateBitmap(), but they may need to be different bitmaps.
And you also can't determine the appropriate size if size == wxDefaultSize because you don't have the window the bitmap will be used for, so you don't know its scale factor on a multi-monitor system.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
@vslavik commented on this pull request.
> - // necessary, so use GetDIPSize() and not GetSize(). - const wxBitmap bitmap = wxArtProvider::GetBitmap(id, client); - if ( bitmap.IsOk() ) - return bitmap.GetDIPSize(); - - // We really need some default size, so just return this hardcoded - // value if all else fails -- what else can we do. - return wxSize(16, 16); + // Unfortunately we don't know what bitmap sizes are available here as + // there is simply nothing in wxArtProvider API that returns this (and + // adding something to the API doesn't make sense as all this is only + // used for compatibility with the existing custom art providers -- new + // ones should just override CreateBitmapBundle() directly), so we only + // return the original bitmap size, but hope that perhaps the provider + // will have a x2 version too, when our GetBitmap() is called. + return i++ ? 0.0 : 1.0;
I missed this in my initial review, but apparently was bothered by it subconsciously enough to wake up knowing it was the cause of my scaling issues. This shouldn't read "1.0", but should use the bitmap's scaling factor.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
No, I think you're right. But I don't understand why does this create problems in Poedit.
See my other comment, there were two places where the wrong size/scale was used.
I think any really DPI-aware applications should override CreateBitmapBundle() and not CreateBitmap() anyhow.
…
And you also can't determine the appropriate size if size == wxDefaultSize because you don't have the window the bitmap will be used for, so you don't know its scale factor on a multi-monitor system.
Yes, overriding CreateBitmapBundle is the "proper" way to upgrade to wx-3.2 and yes, any existing user code with HiDPI support will be suboptimal and full of compromises. But I got the impression from the other PR that you cared about such code too and want me to review with an eye towards it.
And with the one change suggested above, I really like this PR in that regard. The scaling issues are now easy to pinpoint (twice enlarged bitmaps vs subtly mis-scaled in different ways) and can be trivially fixed by setting the correct scaling factor. It makes it easy to upgrade Poedit (and I assume anybody doing any HiDPI work with wx will be in a similar situation) to 3.2's better way of doing things incrementally — quick fixes right away, then proper rewrite.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> - // necessary, so use GetDIPSize() and not GetSize(). - const wxBitmap bitmap = wxArtProvider::GetBitmap(id, client); - if ( bitmap.IsOk() ) - return bitmap.GetDIPSize(); - - // We really need some default size, so just return this hardcoded - // value if all else fails -- what else can we do. - return wxSize(16, 16); + // Unfortunately we don't know what bitmap sizes are available here as + // there is simply nothing in wxArtProvider API that returns this (and + // adding something to the API doesn't make sense as all this is only + // used for compatibility with the existing custom art providers -- new + // ones should just override CreateBitmapBundle() directly), so we only + // return the original bitmap size, but hope that perhaps the provider + // will have a x2 version too, when our GetBitmap() is called. + return i++ ? 0.0 : 1.0;
Oops, yes, of course, thanks for finding this!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- 98635d1 Document some issues involved in adding high DPI support
- 38dfcf0 Handle non-default scale factor in wxBitmapBundleImplArt
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
No, I think you're right. But I don't understand why does this create problems in Poedit.
See my other comment, there were two places where the wrong size/scale was used.
I hope I've fixed them both in the latest commit, please let me know if I did anything wrong, thanks again!
Yes, overriding CreateBitmapBundle is the "proper" way to upgrade to wx-3.2 and yes, any existing user code with HiDPI support will be suboptimal and full of compromises. But I got the impression from the other PR that you cared about such code too and want me to review with an eye towards it.
Yes, absolutely, I care about making things work as well as possible in this case, but I don't expect the impossible and I have probably misunderstood you as I thought you wanted to keep the existing code, overriding CreateBitmap() only.
And with the one change suggested above, I really like this PR in that regard. The scaling issues are now easy to pinpoint (twice enlarged bitmaps vs subtly mis-scaled in different ways) and can be trivially fixed by setting the correct scaling factor. It makes it easy to upgrade Poedit (and I assume anybody doing any HiDPI work with wx will be in a similar situation) to 3.2's better way of doing things incrementally — quick fixes right away, then proper rewrite.
Great to hear, thanks again for your help with it! And I'll merge this soon unless you (or anybody else, of course) see any problems with the last commit.
One thing I'm still not sure about is the tool bitmap size handling in wxToolBar. I think setting it may be quite common (even though it's totally unnecessary) and I wonder if saying "don't do this" is really the best we can do. Because in principle we could use integer scaling here too:
i.e. sizeOrig*wxRound(scale) instead of using ToDIP(). Do you see any reason not to do this?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalph
--
You received this message because you are subscribed to the Google Groups "wx-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to wx-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/wx-dev/wxWidgets/wxWidgets/pull/22481/c1146627116%40github.com.
Václav Slavík
Great to hear, thanks again for your help with it! And I'll merge this soon unless you (or anybody else, of course) see any problems with the last commit.
I don't, thanks!
i.e. sizeOrig*int(scale) instead of using ToDIP(). Do you see any reason not to do this?
I'm more than a little confused by the code involved:
The default size is 16x15 — how is that not causing problems, but other sizes are?(because default size is minimum)- Why is the code explicitly allowing downscaling only in the very edge case of a) different tool sizes, b) with equal count of both, c) that doesn't happen to match the ideal size https://github.com/wxWidgets/wxWidgets/blob/731d29baceda04c5781c6e204cec3d9df8fe9d53/src/common/bmpbndl.cpp#L670-L677 I mean, why even bother?
- Why is it feeding the determined size back as the ideal size next time this function runs? Should the input to GetConsensusSizeFor be
m_requestedBitmapSizeonly?
I would make a different choice in the snippet above - in a tie, I would pick the size closer to sizeIdeal, instead of the larger one.
As for using sizeOrig*int(scale) instead of ToDIP(), unless I'm missing something it would be incorrect:
sizeOrig(aka m_defaultWidth/m_defaultHeight) is expressed in physical pixelsToDIP(sizeOrig)converts it to DIP- In
GetConsensusSizeForit is immediately converted back by multiplying it bywin->GetDPIScaleFactor() - (…and isn't actually used for much)
In any case, it wouldn't have any effect except in an edge case. The actual cause of the problem is further down after obtaining the consensus, I think:
https://github.com/wxWidgets/wxWidgets/blob/73d425b34285b1ba150a1748c2b9146d58125e09/src/common/tbarbase.cpp#L497-L500
You're right that calling SetToolBitmapSize() may be common. Unless I misunderstand the problem, I would:
- Outright deprecate
SetToolBitmapSize() - Either ignore the value provided or only use it as hint for
GetConsensusSizeFor, but don'tIncToto it (possibly only if scaling factor > 1.0) - When adjusting the size, use resize, not rescale — I'm not sure if it is feasible, if this is done in win32, probably not
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, I'm going to merge this one because it already has so many commits/changes, but I'll create another PR addressing the toolbar bitmap size issues and taking your comments into account a.s.a.p.
For the record, at least parts of this code are needed in order to make the toolbar sample, which illustrates showing bitmaps of different sizes, work correctly when moving it between the displays using different DPIs. I think that simplifying it too much would break it again (it was already broken more than once...), but I'll test it to be certain.
And thanks again for the review/discussion!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #22481 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Vadim Zeitlin
R> Searching all of github, I found 8,440 calls to SetToolBitmapSize in .cpp
R> files -- not all of those are wxWidgets apps, but it looks like most of
R> them are. Fortunately, only one of the 5 active UI designers automatically
R> sets this property, but they all make it readily available, and most of
R> them state the default is 16x15 unless otherwise set (which is what the
R> documentation for SetToolBitmapSize used to state) -- which would make
R> it more likely for users to set if that's not the default bitmap size they
R> are using. So yeah, I suspect lots of folks are using this call...
I've created https://github.com/wxWidgets/wxWidgets/pull/22488 improving
the behaviour of SetToolBitmapSize() as I agree that we can't just tell
people to remove all of its occurrences (well, we can tell them, but I
don't think everybody would listen...).
This is another non-trivial changes that I plan to merge just before the
release (what could possibly go wrong?), so any testing would be very much
appreciated.
Thanks again,
VZ