From 78da0eed687a514a51dd2f31a87079836d3488c2 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 18:35:44 +0100 Subject: [PATCH 01/20] Refactor wxBitmapBundleImplSet::GetPreferredBitmapSizeAtScale() Extract the logic determining the scale to use in a reusable DoGetPreferredSize() function to allow reusing it in other places. There are no real changes here, this commit just moved the existing code to the new function, but because it also changed it from using wxSize to double, even git --color-moved doesn't show it as an actual move. --- include/wx/bmpbndl.h | 8 ++++ interface/wx/bmpbndl.h | 52 ++++++++++++++++++++- src/common/bmpbndl.cpp | 103 ++++++++++++++++++++++++++--------------- 3 files changed, 124 insertions(+), 39 deletions(-) diff --git a/include/wx/bmpbndl.h b/include/wx/bmpbndl.h index 248dd63274..db07bea47f 100644 --- a/include/wx/bmpbndl.h +++ b/include/wx/bmpbndl.h @@ -224,6 +224,14 @@ wxBitmapBundle wxBitmapBundle::FromImage(const wxImage& image) class WXDLLIMPEXP_CORE wxBitmapBundleImpl : public wxRefCounter { protected: + // Standard implementation of GetPreferredBitmapSizeAtScale(): choose the + // scale closest to the given one from the available bitmap scales. + // + // Note that scales *must* be sorted in increasing scale order and there + // must be at least 1 of them. + wxSize + DoGetPreferredSize(double scale, size_t n, const double *availableScales) const; + virtual ~wxBitmapBundleImpl(); public: diff --git a/interface/wx/bmpbndl.h b/interface/wx/bmpbndl.h index 901b56d20b..06ce457999 100644 --- a/interface/wx/bmpbndl.h +++ b/interface/wx/bmpbndl.h @@ -442,7 +442,8 @@ public: return GetDefaultSize()*scale; ... otherwise, an existing bitmap of the size closest to the - one above would need to be found and its size returned ... + one above would need to be found and its size returned, + possibly by letting DoGetPreferredSize() choose it ... } wxBitmap GetBitmap(const wxSize& size) wxOVERRIDE @@ -488,6 +489,55 @@ public: on demand and cache it. */ virtual wxBitmap GetBitmap(const wxSize& size) = 0; + +protected: + /** + Helper for implementing GetPreferredBitmapSizeAtScale() in the derived + classes. + + This function implements the standard algorithm used inside wxWidgets + itself and tries to find the scale closest to the given one, while also + trying to choose one of the available scales, to avoid actually + rescaling the bitmaps. + + Typically this function is used in the derived classes implementation, + e.g. + @code + class MyCustomBitmapBundleImpl : public wxBitmapBundleImpl + { + public: + wxSize GetPreferredBitmapSizeAtScale(double scale) const wxOVERRIDE + { + std::vector vec; + vec.push_back(1); + + // Note that the vector must be sorted, so we must add this + // scale before adding 2 below. + if ( MyIntermediateBitmapIsAvailable() ) + vec.push_back(1.5); + + vec.push_back(2); + + return DoGetPreferredSize(scale, vec.size(), vec.data()); + } + + ... + }; + @endcode + + @param scale The required scale, typically the same one as passed to + GetPreferredBitmapSizeAtScale(). + @param n The number of elements in @a availableScales, must be strictly + positive (i.e. there must always be at least one available scale). + @param availableScales The scales in which bitmaps are available, i.e. + scales such that GetBitmap() wouldn't need to scale the bitmap if + it were called with them. This array @e must be in sorted order, + with 1 being its first element. + + @since 3.1.7 + */ + wxSize + DoGetPreferredSize(double scale, size_t n, const double *availableScales) const; }; /** diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 51317d3d9d..18c713ad65 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -237,50 +237,16 @@ wxSize wxBitmapBundleImplSet::GetDefaultSize() const wxSize wxBitmapBundleImplSet::GetPreferredBitmapSizeAtScale(double scale) const { - // Target size is the ideal size we'd like the bitmap to have at this scale. - const wxSize sizeTarget = GetDefaultSize()*scale; + const double baseY = GetDefaultSize().y; const size_t n = m_entries.size(); + wxVector scales(n); for ( size_t i = 0; i < n; ++i ) { - const wxSize sizeThis = m_entries[i].bitmap.GetSize(); - - // Keep looking for the exact match which we still can hope to find - // while the current bitmap is smaller. - if ( sizeThis.y < sizeTarget.y ) - continue; - - // If we've found the exact match, just return it. - if ( sizeThis.y == sizeTarget.y ) - return sizeThis; - - // We've found the closest bigger bitmap. - - // If there is no smaller bitmap, we have no choice but to use this one. - if ( i == 0 ) - return sizeThis; - - // Decide whether we should use this one or the previous smaller one - // depending on which of them is closer to the target size, breaking - // the tie in favour of the bigger size. - const wxSize sizeLast = m_entries[i - 1].bitmap.GetSize(); - - return sizeThis.y - sizeTarget.y <= sizeTarget.y - sizeLast.y - ? sizeThis - : sizeLast; - + scales[i] = m_entries[i].bitmap.GetSize().y / baseY; } - // We only get here if the target size is bigger than all the available - // sizes, in which case we have no choice but to use the biggest bitmap. - const wxSize sizeMax = m_entries.back().bitmap.GetSize(); - - // But check how far is it from the requested scale: if it's more than 1.5 - // times smaller, we should still scale it, notably to ensure that bitmaps - // of standard size are scaled when 2x DPI scaling is used. - return static_cast(sizeTarget.y) / sizeMax.y >= 1.5 - ? sizeTarget - : sizeMax; + return DoGetPreferredSize(scale, n, &scales[0]); } wxBitmap wxBitmapBundleImplSet::GetBitmap(const wxSize& size) @@ -715,6 +681,67 @@ wxBitmapBundle::CreateImageList(wxWindow* win, // wxBitmapBundleImpl implementation // ============================================================================ +wxSize +wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, + size_t n, + const double* availableScales) const +{ + wxCHECK_MSG( n > 0, wxSize(), wxS("must have at least one scale") ); + + double scaleBest = 0.0; + + for ( size_t i = 0; i < n; ++i ) + { + const double scaleThis = availableScales[i]; + + // Keep looking for the exact match which we still can hope to find + // while the current bitmap is smaller. + if ( scaleThis < scaleTarget ) + continue; + + // If we've found the exact match, just return it. + if ( scaleThis == scaleTarget ) + { + scaleBest = scaleThis; + break; + } + + // We've found the closest bigger bitmap. + + // If there is no smaller bitmap, we have no choice but to use this one. + if ( i == 0 ) + { + scaleBest = scaleThis; + break; + } + + // Decide whether we should use this one or the previous smaller one + // depending on which of them is closer to the target size, breaking + // the tie in favour of the bigger size. + const double scaleLast = availableScales[i - 1]; + + scaleBest = scaleThis - scaleTarget <= scaleTarget - scaleLast + ? scaleThis + : scaleLast; + break; + } + + if ( scaleBest == 0.0 ) + { + // We only get here if the target scale is bigger than all the + // available scales, in which case we have no choice but to use the + // biggest bitmap. + const double scaleMax = availableScales[n - 1]; + + // But check how far is it from the requested scale: if it's more than + // 1.5 times smaller, we should still scale it, notably to ensure that + // bitmaps of standard size are scaled when 2x DPI scaling is used. + scaleBest = scaleTarget / scaleMax >= 1.5 ? scaleTarget : scaleMax; + } + + return GetDefaultSize()*scaleBest; +} + wxBitmapBundleImpl::~wxBitmapBundleImpl() { #ifdef __WXOSX__ From 028a1266feaa4d648da70ad5f2256fddda6b7aa1 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 18:45:50 +0100 Subject: [PATCH 02/20] Don't use generated bitmaps when looking for preferred scale The already scaled bitmaps shouldn't be used for deciding to which size we can scale the bitmaps with good results, as scaling them again would definitely be a bad idea. --- src/common/bmpbndl.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 18c713ad65..a5300fbb9d 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -240,13 +240,17 @@ wxSize wxBitmapBundleImplSet::GetPreferredBitmapSizeAtScale(double scale) const const double baseY = GetDefaultSize().y; const size_t n = m_entries.size(); - wxVector scales(n); + wxVector scales; + scales.reserve(n); for ( size_t i = 0; i < n; ++i ) { - scales[i] = m_entries[i].bitmap.GetSize().y / baseY; + if ( m_entries[i].generated ) + continue; + + scales.push_back(m_entries[i].bitmap.GetSize().y / baseY); } - return DoGetPreferredSize(scale, n, &scales[0]); + return DoGetPreferredSize(scale, scales.size(), &scales[0]); } wxBitmap wxBitmapBundleImplSet::GetBitmap(const wxSize& size) From e03297b6fa3c4261f0cbb344812e9d2e0ef4a62a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 18:48:27 +0100 Subject: [PATCH 03/20] Use the same preferred size algorithm in wxBitmapBundleImplRC Reuse DoGetPreferredSize() in this implementation in order to ensure that we do the same thing when using bitmaps from disk files with wxBitmapBundleImplSet or bitmaps from MSW resources with this one. --- src/msw/bmpbndl.cpp | 44 +++++--------------------------------------- 1 file changed, 5 insertions(+), 39 deletions(-) diff --git a/src/msw/bmpbndl.cpp b/src/msw/bmpbndl.cpp index f5ec7b25dd..6ecd42a78e 100644 --- a/src/msw/bmpbndl.cpp +++ b/src/msw/bmpbndl.cpp @@ -191,48 +191,14 @@ wxSize wxBitmapBundleImplRC::GetDefaultSize() const wxSize wxBitmapBundleImplRC::GetPreferredBitmapSizeAtScale(double scale) const { - // Optimistically assume we're going to use this exact scale by default. - double scalePreferred = scale; - - for ( size_t i = 0; ; ++i ) + const size_t n = m_resourceInfos.size(); + wxVector scales(n); + for ( size_t i = 0; i < n; ++i ) { - if ( i == m_resourceInfos.size() ) - { - // The requested scale is bigger than anything we have, so use the - // biggest available one. - scalePreferred = m_resourceInfos[i - 1].scale; - break; - } - - const double scaleThis = m_resourceInfos[i].scale; - - // Keep looking for the exact match which we still can hope to find - // while the current scale is smaller. - if ( scaleThis < scale ) - continue; - - // If we've found the exact match, just use it. - if ( scaleThis == scale ) - break; - - // We've found the closest bigger scale. - - // If there is no smaller one, we have no choice but to use this one. - if ( i == 0 ) - break; - - // Decide whether we should use this one or the previous smaller one - // depending on which of them is closer to the target scale, breaking - // the tie in favour of the bigger one. - const double scaleLast = m_resourceInfos[i - 1].scale; - - scalePreferred = scaleThis - scale <= scale - scaleLast - ? scaleThis - : scaleLast; - break; + scales[i] = m_resourceInfos[i].scale; } - return GetDefaultSize()*scalePreferred; + return DoGetPreferredSize(scale, n, &scales[0]); } wxBitmap From 0b0e95296a8a91aa61de00c0c903804555c884a2 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 19:12:16 +0100 Subject: [PATCH 04/20] Upscale images by integer factors only Doing anything else results in really poor results, so don't even try scaling images by 2.25, for example -- scaling by 2 looks much better and the size difference is relatively insignificant and definitely not worth the difference in quality. --- src/common/bmpbndl.cpp | 13 +++++++++++-- tests/graphics/bmpbundle.cpp | 34 +++++++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index a5300fbb9d..bd00552003 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -738,9 +738,18 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, const double scaleMax = availableScales[n - 1]; // But check how far is it from the requested scale: if it's more than - // 1.5 times smaller, we should still scale it, notably to ensure that + // 1.5 times larger, we should still scale it, notably to ensure that // bitmaps of standard size are scaled when 2x DPI scaling is used. - scaleBest = scaleTarget / scaleMax >= 1.5 ? scaleTarget : scaleMax; + if ( scaleTarget >= 1.5*scaleMax ) + { + // However scaling by non-integer scales doesn't work well at all, so + // round it to the closest integer in this case. + scaleBest = wxRound(scaleTarget); + } + else // Target scale is not much greater than the biggest one we have. + { + scaleBest = scaleMax; + } } return GetDefaultSize()*scaleBest; diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index a703fe0bec..17367b02b4 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -51,13 +51,36 @@ TEST_CASE("BitmapBundle::FromBitmaps", "[bmpbundle]") TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") { - CHECK( wxBitmapBundle().GetPreferredBitmapSizeAtScale(1) == wxDefaultSize ); + // Check that empty bundle doesn't have any preferred size. + wxBitmapBundle b; + CHECK( b.GetPreferredBitmapSizeAtScale(1) == wxDefaultSize ); const wxSize normal(32, 32); const wxSize bigger(64, 64); + const wxSize triple(96, 96); - const wxBitmapBundle - b = wxBitmapBundle::FromBitmaps(wxBitmap(normal), wxBitmap(bigger)); + + // Then check what happens if there is only a single bitmap. + b = wxBitmapBundle::FromBitmap(wxBitmap(normal)); + + // We should avoid scaling as long as the size is close enough to the + // actual bitmap size. + CHECK( b.GetPreferredBitmapSizeAtScale(0 ) == normal ); + CHECK( b.GetPreferredBitmapSizeAtScale(1 ) == normal ); + CHECK( b.GetPreferredBitmapSizeAtScale(1.25) == normal ); + CHECK( b.GetPreferredBitmapSizeAtScale(1.4 ) == normal ); + + // Once it becomes too big, we're going to need to scale, but we should be + // scaling by an integer factor. + CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == bigger ); + CHECK( b.GetPreferredBitmapSizeAtScale(1.75) == bigger ); + CHECK( b.GetPreferredBitmapSizeAtScale(2 ) == bigger ); + CHECK( b.GetPreferredBitmapSizeAtScale(2.25) == bigger ); + CHECK( b.GetPreferredBitmapSizeAtScale(2.5 ) == triple ); + + + // Now check what happens when there is also a double size bitmap. + b = wxBitmapBundle::FromBitmaps(wxBitmap(normal), wxBitmap(bigger)); // Check that the existing bitmaps are used without scaling for most of the // typical scaling values. @@ -71,8 +94,9 @@ TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") CHECK( b.GetPreferredBitmapSizeAtScale(2.5 ) == bigger ); // This scale is too big to use any of the existing bitmaps, so they will - // be scaled. - CHECK( b.GetPreferredBitmapSizeAtScale(3 ) == 3*normal ); + // be scaled, but use integer factors. + CHECK( b.GetPreferredBitmapSizeAtScale(3 ) == triple ); + CHECK( b.GetPreferredBitmapSizeAtScale(3.33) == triple ); } #ifdef wxHAS_DPI_INDEPENDENT_PIXELS From 30fb836136e0d516f99d5f4c0cccb449c72feb73 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 19:15:36 +0100 Subject: [PATCH 05/20] Break the tie in favour of smaller bitmaps at 1.5x factor Prefer to use 1x bitmap to 2x one at 150% scaling, as it seems better to use smaller (but still readable) bitmaps than overlarge ones in this case. --- src/common/bmpbndl.cpp | 7 ++++--- tests/graphics/bmpbundle.cpp | 6 +++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index bd00552003..9b376a230c 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -721,10 +721,11 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, // Decide whether we should use this one or the previous smaller one // depending on which of them is closer to the target size, breaking - // the tie in favour of the bigger size. + // the tie in favour of the smaller size as it's arguably better to use + // slightly smaller bitmaps than too big ones. const double scaleLast = availableScales[i - 1]; - scaleBest = scaleThis - scaleTarget <= scaleTarget - scaleLast + scaleBest = scaleThis - scaleTarget < scaleTarget - scaleLast ? scaleThis : scaleLast; break; @@ -740,7 +741,7 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, // But check how far is it from the requested scale: if it's more than // 1.5 times larger, we should still scale it, notably to ensure that // bitmaps of standard size are scaled when 2x DPI scaling is used. - if ( scaleTarget >= 1.5*scaleMax ) + if ( scaleTarget > 1.5*scaleMax ) { // However scaling by non-integer scales doesn't work well at all, so // round it to the closest integer in this case. diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index 17367b02b4..5d9da96b44 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -69,10 +69,10 @@ TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") CHECK( b.GetPreferredBitmapSizeAtScale(1 ) == normal ); CHECK( b.GetPreferredBitmapSizeAtScale(1.25) == normal ); CHECK( b.GetPreferredBitmapSizeAtScale(1.4 ) == normal ); + CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == normal ); // Once it becomes too big, we're going to need to scale, but we should be // scaling by an integer factor. - CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == bigger ); CHECK( b.GetPreferredBitmapSizeAtScale(1.75) == bigger ); CHECK( b.GetPreferredBitmapSizeAtScale(2 ) == bigger ); CHECK( b.GetPreferredBitmapSizeAtScale(2.25) == bigger ); @@ -88,14 +88,14 @@ TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") CHECK( b.GetPreferredBitmapSizeAtScale(1 ) == normal ); CHECK( b.GetPreferredBitmapSizeAtScale(1.25) == normal ); CHECK( b.GetPreferredBitmapSizeAtScale(1.4 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == bigger ); + CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == normal ); CHECK( b.GetPreferredBitmapSizeAtScale(1.75) == bigger ); CHECK( b.GetPreferredBitmapSizeAtScale(2 ) == bigger ); CHECK( b.GetPreferredBitmapSizeAtScale(2.5 ) == bigger ); + CHECK( b.GetPreferredBitmapSizeAtScale(3 ) == bigger ); // This scale is too big to use any of the existing bitmaps, so they will // be scaled, but use integer factors. - CHECK( b.GetPreferredBitmapSizeAtScale(3 ) == triple ); CHECK( b.GetPreferredBitmapSizeAtScale(3.33) == triple ); } From 3414e6a1d49648ca500c7c4344980d1baa0caf30 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 22:34:32 +0100 Subject: [PATCH 06/20] Extend wxBitmapBundle unit tests for bitmap scaling Check not only that the resulting bitmap has the right size, but also that it was obtained by scaling the correct bitmap. Define the required machinery to allow using CATCH CHECK_THAT() macro to succinctly verify both. Note that this doesn't change the existing tests, but just rewrites them in a new form and also adds more tests for a bundle with 3 bitmaps. --- tests/graphics/bmpbundle.cpp | 245 +++++++++++++++++++++++++++++++---- 1 file changed, 221 insertions(+), 24 deletions(-) diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index 5d9da96b44..c9a179174a 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -15,6 +15,7 @@ #include "wx/bmpbndl.h" #include "wx/artprov.h" +#include "wx/dcmemory.h" #include "asserthelper.h" @@ -49,54 +50,250 @@ TEST_CASE("BitmapBundle::FromBitmaps", "[bmpbundle]") CHECK( b.GetBitmap(wxSize(24, 24)).GetSize() == wxSize(24, 24) ); } +// Helper functions for the test below. +namespace +{ + +// Default size here doesn't really matter. +const wxSize BITMAP_SIZE(16, 16); + +// The choice of colours here is arbitrary too, but they need to be all +// different to allow identifying which bitmap got scaled. +struct ColourAtScale +{ + double scale; + wxUint32 rgb; +}; + +const ColourAtScale colours[] = +{ + { 1.0, 0x000000ff }, + { 1.5, 0x0000ff00 }, + { 2.0, 0x00ff0000 }, +}; + +// Return the colour used for the (original) bitmap at the given scale. +wxColour GetColourForScale(double scale) +{ + wxColour col; + for ( size_t n = 0; n < WXSIZEOF(colours); ++n ) + { + if ( colours[n].scale == scale ) + { + col.SetRGB(colours[n].rgb); + return col; + } + } + + wxFAIL_MSG("no colour for this scale"); + + return col; +} + +double GetScaleFromColour(const wxColour& col) +{ + const wxUint32 rgb = col.GetRGB(); + for ( size_t n = 0; n < WXSIZEOF(colours); ++n ) + { + if ( colours[n].rgb == rgb ) + return colours[n].scale; + } + + wxFAIL_MSG(wxString::Format("no scale for colour %s", col.GetAsString())); + + return 0.0; +} + +double SizeToScale(const wxSize& size) +{ + return static_cast(size.y) / BITMAP_SIZE.y; +} + +wxBitmap MakeSolidBitmap(double scale) +{ + wxBitmap bmp(BITMAP_SIZE*scale); + + wxMemoryDC dc(bmp); + dc.SetBackground(GetColourForScale(scale)); + dc.Clear(); + + return bmp; +} + +wxColour GetBitmapColour(const wxBitmap& bmp) +{ + const wxImage img = bmp.ConvertToImage(); + + // We just assume the bitmap is solid colour, we could check it, but it + // doesn't seem really useful to do it. + return wxColour(img.GetRed(0, 0), img.GetGreen(0, 0), img.GetBlue(0, 0)); +} + +// This struct exists just to allow using it conveniently in CHECK_THAT(). +struct BitmapAtScale +{ + BitmapAtScale(const wxBitmapBundle& b, double scale) + : size(b.GetPreferredBitmapSizeAtScale(scale)), + bitmap(b.GetBitmap(size)) + { + } + + const wxSize size; + const wxBitmap bitmap; +}; + +class BitmapAtScaleMatcher : public Catch::MatcherBase +{ +public: + explicit BitmapAtScaleMatcher(double scale, double scaleOrig) + : m_scale(scale), + m_scaleOrig(scaleOrig) + { + } + + bool match(const BitmapAtScale& bitmapAtScale) const wxOVERRIDE + { + const wxBitmap& bmp = bitmapAtScale.bitmap; + + if ( SizeToScale(bitmapAtScale.size) != m_scale || + SizeToScale(bmp.GetSize()) != m_scale ) + { + m_diffDesc.Printf("should have scale %.1f", m_scale); + } + + if ( GetBitmapColour(bmp) != GetColourForScale(m_scaleOrig) ) + { + if ( m_diffDesc.empty() ) + m_diffDesc = "should be "; + else + m_diffDesc += " and be "; + + m_diffDesc += wxString::Format("created from x%.1f", m_scaleOrig); + } + + return m_diffDesc.empty(); + } + + std::string describe() const wxOVERRIDE + { + return m_diffDesc.utf8_string(); + } + +private: + const double m_scale; + const double m_scaleOrig; + mutable wxString m_diffDesc; +}; + +// The first parameter here determines the size of the expected bitmap and the +// second one, which defaults to the first one if it's not specified, the size +// of the bitmap which must have been scaled to create the bitmap of the right +// size. +BitmapAtScaleMatcher SameAs(double scale, double scaleOrig = 0.0) +{ + if ( scaleOrig == 0.0 ) + scaleOrig = scale; + + return BitmapAtScaleMatcher(scale, scaleOrig); +} + +} // anonymous namespace + +namespace Catch +{ + template <> + struct StringMaker + { + static std::string convert(const BitmapAtScale& bitmapAtScale) + { + const wxBitmap& bmp = bitmapAtScale.bitmap; + + wxString scaleError; + if ( bmp.GetSize() != bitmapAtScale.size ) + { + scaleError.Printf(" (DIFFERENT from expected %.1f)", + SizeToScale(bitmapAtScale.size)); + } + + return wxString::Format + ( + "x%.1f bitmap%s created from x%.1f", + SizeToScale(bmp.GetSize()), + scaleError, + GetScaleFromColour(GetBitmapColour(bmp)) + ).utf8_string(); + } + }; +} + TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") { // Check that empty bundle doesn't have any preferred size. wxBitmapBundle b; CHECK( b.GetPreferredBitmapSizeAtScale(1) == wxDefaultSize ); - const wxSize normal(32, 32); - const wxSize bigger(64, 64); - const wxSize triple(96, 96); + const wxBitmap normal = MakeSolidBitmap(1.0); + const wxBitmap middle = MakeSolidBitmap(1.5); + const wxBitmap bigger = MakeSolidBitmap(2.0); // Then check what happens if there is only a single bitmap. - b = wxBitmapBundle::FromBitmap(wxBitmap(normal)); + b = wxBitmapBundle::FromBitmap(normal); // We should avoid scaling as long as the size is close enough to the // actual bitmap size. - CHECK( b.GetPreferredBitmapSizeAtScale(0 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.25) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.4 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == normal ); + CHECK_THAT( BitmapAtScale(b, 0 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.25), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.4 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.5 ), SameAs(1) ); // Once it becomes too big, we're going to need to scale, but we should be // scaling by an integer factor. - CHECK( b.GetPreferredBitmapSizeAtScale(1.75) == bigger ); - CHECK( b.GetPreferredBitmapSizeAtScale(2 ) == bigger ); - CHECK( b.GetPreferredBitmapSizeAtScale(2.25) == bigger ); - CHECK( b.GetPreferredBitmapSizeAtScale(2.5 ) == triple ); + CHECK_THAT( BitmapAtScale(b, 1.75), SameAs(2, 1) ); + CHECK_THAT( BitmapAtScale(b, 2 ), SameAs(2, 1) ); + CHECK_THAT( BitmapAtScale(b, 2.25), SameAs(2, 1) ); + CHECK_THAT( BitmapAtScale(b, 2.5 ), SameAs(3, 1) ); // Now check what happens when there is also a double size bitmap. - b = wxBitmapBundle::FromBitmaps(wxBitmap(normal), wxBitmap(bigger)); + b = wxBitmapBundle::FromBitmaps(normal, bigger); // Check that the existing bitmaps are used without scaling for most of the // typical scaling values. - CHECK( b.GetPreferredBitmapSizeAtScale(0 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.25) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.4 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.5 ) == normal ); - CHECK( b.GetPreferredBitmapSizeAtScale(1.75) == bigger ); - CHECK( b.GetPreferredBitmapSizeAtScale(2 ) == bigger ); - CHECK( b.GetPreferredBitmapSizeAtScale(2.5 ) == bigger ); - CHECK( b.GetPreferredBitmapSizeAtScale(3 ) == bigger ); + CHECK_THAT( BitmapAtScale(b, 0 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.25), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.4 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.5 ), SameAs(1) ); + CHECK_THAT( BitmapAtScale(b, 1.75), SameAs(2) ); + CHECK_THAT( BitmapAtScale(b, 2 ), SameAs(2) ); + CHECK_THAT( BitmapAtScale(b, 2.5 ), SameAs(2) ); + CHECK_THAT( BitmapAtScale(b, 3 ), SameAs(2) ); // This scale is too big to use any of the existing bitmaps, so they will // be scaled, but use integer factors. - CHECK( b.GetPreferredBitmapSizeAtScale(3.33) == triple ); + CHECK_THAT( BitmapAtScale(b, 3.33), SameAs(3, 2) ); + CHECK_THAT( BitmapAtScale(b, 4 ), SameAs(4, 2) ); + CHECK_THAT( BitmapAtScale(b, 5 ), SameAs(5, 2) ); + + + // Finally check that things work as expected when we have 3 versions. + wxVector bitmaps; + bitmaps.push_back(normal); + bitmaps.push_back(middle); + bitmaps.push_back(bigger); + b = wxBitmapBundle::FromBitmaps(bitmaps); + + CHECK_THAT( BitmapAtScale(b, 0 ), SameAs(1.0) ); + CHECK_THAT( BitmapAtScale(b, 1 ), SameAs(1.0) ); + CHECK_THAT( BitmapAtScale(b, 1.25), SameAs(1.0) ); + CHECK_THAT( BitmapAtScale(b, 1.4 ), SameAs(1.5) ); + CHECK_THAT( BitmapAtScale(b, 1.5 ), SameAs(1.5) ); + CHECK_THAT( BitmapAtScale(b, 1.75), SameAs(1.5) ); + CHECK_THAT( BitmapAtScale(b, 2 ), SameAs(2.0) ); + CHECK_THAT( BitmapAtScale(b, 2.5 ), SameAs(2.0) ); + CHECK_THAT( BitmapAtScale(b, 3 ), SameAs(2.0) ); } #ifdef wxHAS_DPI_INDEPENDENT_PIXELS From 2d782443a4ab27fa98e9a7ac867d52485b708cdf Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 23:09:10 +0100 Subject: [PATCH 07/20] Choose the best bitmap to rescale to the target size It's not necessarily the largest bitmap, but one which may be scaled using an integer factor. Fix the tests to expect the correct results, now that they actually pass. --- src/common/bmpbndl.cpp | 35 ++++++++++++++++++++++++++--------- src/msw/bmpbndl.cpp | 25 ++++++++++++++++++------- tests/graphics/bmpbundle.cpp | 14 +++++++++++--- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 9b376a230c..6809b3ab4f 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -292,18 +292,35 @@ wxBitmap wxBitmapBundleImplSet::GetBitmap(const wxSize& size) // We only get here if the requested size is larger than the size of all // the bitmaps we have, in which case we have no choice but to upscale one - // of the bitmaps, so find the largest available non-generated bitmap. - for ( size_t i = n; n > 0; --i ) + // of the bitmaps, so find the largest available non-generated bitmap which + // can be scaled using an integer factor or, failing that, just the largest + // non-generated bitmap. + const Entry* entryToRescale = NULL; + const Entry* entryLastNotGen = NULL; + for ( size_t i = 0; i < n; ++i ) { - const Entry& entry = m_entries[i - 1]; - if ( !entry.generated ) - { - const Entry entryNew(entry, size); + const Entry& entry = m_entries[i]; + if ( entry.generated ) + continue; - m_entries.push_back(entryNew); + entryLastNotGen = &entry; - return entryNew.bitmap; - } + const double + scale = static_cast(size.y) / entry.bitmap.GetSize().y; + if ( scale == wxRound(scale) ) + entryToRescale = &entry; + } + + if ( !entryToRescale ) + entryToRescale = entryLastNotGen; + + if ( entryToRescale ) + { + const Entry entryNew(*entryToRescale, size); + + m_entries.push_back(entryNew); + + return entryNew.bitmap; } // We should have at least one non-generated bitmap. diff --git a/src/msw/bmpbndl.cpp b/src/msw/bmpbndl.cpp index 6ecd42a78e..fbd707f040 100644 --- a/src/msw/bmpbndl.cpp +++ b/src/msw/bmpbndl.cpp @@ -269,17 +269,28 @@ wxBitmap wxBitmapBundleImplRC::GetBitmap(const wxSize& size) const wxSize sizeThis = sizeDef*info.scale; - // Use this bitmap if it's the first one bigger than the requested size - // or if it's the last item as in this case we're not going to find any - // bitmap bigger than the given one anyhow and we don't have any choice - // but to upscale the largest one we have. - if ( sizeThis.y >= size.y || i + 1 == m_resourceInfos.size() ) + // Use this bitmap if it's the first one bigger than the requested size. + if ( sizeThis.y >= size.y ) return AddBitmap(info, sizeThis, size); } - wxFAIL_MSG( wxS("unreachable") ); + // We have to upscale some bitmap because we don't have any bitmaps larger + // than the requested size. Try to find one which can be upscaled using an + // integer factor. + const ResourceInfo* infoToRescale = NULL; + for ( size_t i = 0; i < m_resourceInfos.size(); ++i ) + { + const ResourceInfo& info = m_resourceInfos[i]; - return wxBitmap(); + const double scale = size.y / sizeDef.y*info.scale; + if ( scale == wxRound(scale) ) + infoToRescale = &info; + } + + if ( !infoToRescale ) + infoToRescale = &m_resourceInfos.back(); + + return AddBitmap(*infoToRescale, sizeDef*infoToRescale->scale, size); } // ============================================================================ diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index c9a179174a..174c9085ab 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -272,10 +272,14 @@ TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") CHECK_THAT( BitmapAtScale(b, 3 ), SameAs(2) ); // This scale is too big to use any of the existing bitmaps, so they will - // be scaled, but use integer factors. - CHECK_THAT( BitmapAtScale(b, 3.33), SameAs(3, 2) ); + // be scaled, but use integer factors and, importantly, scale the correct + // bitmap using them: we need to scale the small bitmap by a factor of 3, + // rather than scaling the larger bitmap by a factor of 1.5 here, but we + // must also scale the larger one by a factor of 2 rather than scaling the + // small one by a factor of 4. + CHECK_THAT( BitmapAtScale(b, 3.33), SameAs(3, 1) ); CHECK_THAT( BitmapAtScale(b, 4 ), SameAs(4, 2) ); - CHECK_THAT( BitmapAtScale(b, 5 ), SameAs(5, 2) ); + CHECK_THAT( BitmapAtScale(b, 5 ), SameAs(5, 1) ); // Finally check that things work as expected when we have 3 versions. @@ -294,6 +298,10 @@ TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") CHECK_THAT( BitmapAtScale(b, 2 ), SameAs(2.0) ); CHECK_THAT( BitmapAtScale(b, 2.5 ), SameAs(2.0) ); CHECK_THAT( BitmapAtScale(b, 3 ), SameAs(2.0) ); + + CHECK_THAT( BitmapAtScale(b, 3.33), SameAs(3.0, 1.5) ); + CHECK_THAT( BitmapAtScale(b, 4.25), SameAs(4.0, 2.0) ); + CHECK_THAT( BitmapAtScale(b, 5 ), SameAs(5.0, 1.0) ); } #ifdef wxHAS_DPI_INDEPENDENT_PIXELS From 66d4f75c866fa72e11c8b5ce1b6c71dc3f158f34 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 23:19:54 +0100 Subject: [PATCH 08/20] Access availableScales only sequentially in DoGetPreferredSize() No real changes, just refactor the code to only access the array elements consecutively in this function, in preparation to changing its API. --- src/common/bmpbndl.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 6809b3ab4f..8ef96d135a 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -26,6 +26,7 @@ #include "wx/icon.h" #include "wx/iconbndl.h" #include "wx/imaglist.h" +#include "wx/scopeguard.h" #include "wx/window.h" #include "wx/private/bmpbndl.h" @@ -710,11 +711,15 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, wxCHECK_MSG( n > 0, wxSize(), wxS("must have at least one scale") ); double scaleBest = 0.0; + double scaleLast = 0.0; for ( size_t i = 0; i < n; ++i ) { const double scaleThis = availableScales[i]; + // Ensure we remember the last used scale value. + wxON_BLOCK_EXIT_SET(scaleLast, scaleThis); + // Keep looking for the exact match which we still can hope to find // while the current bitmap is smaller. if ( scaleThis < scaleTarget ) @@ -740,8 +745,6 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, // depending on which of them is closer to the target size, breaking // the tie in favour of the smaller size as it's arguably better to use // slightly smaller bitmaps than too big ones. - const double scaleLast = availableScales[i - 1]; - scaleBest = scaleThis - scaleTarget < scaleTarget - scaleLast ? scaleThis : scaleLast; @@ -752,13 +755,12 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, { // We only get here if the target scale is bigger than all the // available scales, in which case we have no choice but to use the - // biggest bitmap. - const double scaleMax = availableScales[n - 1]; + // biggest bitmap, which corresponds to the last used scale. // But check how far is it from the requested scale: if it's more than // 1.5 times larger, we should still scale it, notably to ensure that // bitmaps of standard size are scaled when 2x DPI scaling is used. - if ( scaleTarget > 1.5*scaleMax ) + if ( scaleTarget > 1.5*scaleLast ) { // However scaling by non-integer scales doesn't work well at all, so // round it to the closest integer in this case. @@ -766,7 +768,7 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, } else // Target scale is not much greater than the biggest one we have. { - scaleBest = scaleMax; + scaleBest = scaleLast; } } From 36abfe973a8f412ad6b17596020749661db18276 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Jun 2022 23:41:34 +0100 Subject: [PATCH 09/20] Change DoGetPreferredSize() to use a callback function Instead of taking an array of scales, call GetNextAvailableScale() to get them. This allows centralising the logic for returning the available scales in a single place, where it will be reused in the upcoming commits. --- include/wx/bmpbndl.h | 17 ++++++-- interface/wx/bmpbndl.h | 53 ++++++++++++++++--------- src/common/bmpbndl.cpp | 90 ++++++++++++++++++++++++------------------ src/msw/bmpbndl.cpp | 10 ++++- 4 files changed, 107 insertions(+), 63 deletions(-) diff --git a/include/wx/bmpbndl.h b/include/wx/bmpbndl.h index db07bea47f..3bf8e0b88d 100644 --- a/include/wx/bmpbndl.h +++ b/include/wx/bmpbndl.h @@ -227,10 +227,19 @@ protected: // Standard implementation of GetPreferredBitmapSizeAtScale(): choose the // scale closest to the given one from the available bitmap scales. // - // Note that scales *must* be sorted in increasing scale order and there - // must be at least 1 of them. - wxSize - DoGetPreferredSize(double scale, size_t n, const double *availableScales) const; + // If this function is used, GetNextAvailableScale() must be overridden! + wxSize DoGetPreferredSize(double scale) const; + + // Override this function if DoGetPreferredSize() is used: it can use the + // provided parameter as an internal index, it's guaranteed to be 0 when + // calling this function for the first time. When there are no more scales, + // return 0. + // + // This function is not pure virtual because it doesn't need to be + // implemented if DoGetPreferredSize() is never used, but it will assert if + // it's called. + virtual double GetNextAvailableScale(size_t& i) const; + virtual ~wxBitmapBundleImpl(); diff --git a/interface/wx/bmpbndl.h b/interface/wx/bmpbndl.h index 06ce457999..795dd649f8 100644 --- a/interface/wx/bmpbndl.h +++ b/interface/wx/bmpbndl.h @@ -500,25 +500,28 @@ protected: trying to choose one of the available scales, to avoid actually rescaling the bitmaps. - Typically this function is used in the derived classes implementation, - e.g. + It relies on GetNextAvailableScale() to get information about the + available bitmaps, so that function must be overridden if this one is + used. + + Typically this function is used in the derived classes implementation + to forward GetPreferredBitmapSizeAtScale() to it, e.g. @code class MyCustomBitmapBundleImpl : public wxBitmapBundleImpl { public: wxSize GetPreferredBitmapSizeAtScale(double scale) const wxOVERRIDE { - std::vector vec; - vec.push_back(1); + return DoGetPreferredSize(scale); + } - // Note that the vector must be sorted, so we must add this - // scale before adding 2 below. - if ( MyIntermediateBitmapIsAvailable() ) - vec.push_back(1.5); + protected: + double GetNextAvailableScale(size_t& i) const wxOVERRIDE + { + const double availableScales[] = { 1, 1.5, 2, 0 }; - vec.push_back(2); - - return DoGetPreferredSize(scale, vec.size(), vec.data()); + // We can rely on not being called again once we return 0. + return availableScales[i++]; } ... @@ -527,17 +530,29 @@ protected: @param scale The required scale, typically the same one as passed to GetPreferredBitmapSizeAtScale(). - @param n The number of elements in @a availableScales, must be strictly - positive (i.e. there must always be at least one available scale). - @param availableScales The scales in which bitmaps are available, i.e. - scales such that GetBitmap() wouldn't need to scale the bitmap if - it were called with them. This array @e must be in sorted order, - with 1 being its first element. @since 3.1.7 */ - wxSize - DoGetPreferredSize(double scale, size_t n, const double *availableScales) const; + wxSize DoGetPreferredSize(double scale) const; + + /** + Return information about the available bitmaps. + + Overriding this function is optional and only needs to be done if + DoGetPreferredSize() is called. If you do override it, this function + must return the next available scale or 0.0 if there are no more. + + The returned scales must be in ascending order and the first returned + scale, for the initial @a i value of 0, should be 1. The function must + change @a i, but the values of this index don't have to be consecutive + and it's only used by this function itself, the caller only initializes + it to 0 before the first call. + + See DoGetPreferredSize() for an example of implementing this function. + + @since 3.1.7 + */ + virtual double GetNextAvailableScale(size_t& i) const; }; /** diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 8ef96d135a..8af75ec2a9 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -133,6 +133,9 @@ public: virtual wxSize GetPreferredBitmapSizeAtScale(double scale) const wxOVERRIDE; virtual wxBitmap GetBitmap(const wxSize& size) wxOVERRIDE; +protected: + virtual double GetNextAvailableScale(size_t& i) const wxOVERRIDE; + private: // Struct containing bitmap itself as well as a flag indicating whether we // generated it by rescaling the existing bitmap or not. @@ -236,22 +239,24 @@ wxSize wxBitmapBundleImplSet::GetDefaultSize() const return m_sizeDefault; } -wxSize wxBitmapBundleImplSet::GetPreferredBitmapSizeAtScale(double scale) const +double wxBitmapBundleImplSet::GetNextAvailableScale(size_t& i) const { - const double baseY = GetDefaultSize().y; - - const size_t n = m_entries.size(); - wxVector scales; - scales.reserve(n); - for ( size_t i = 0; i < n; ++i ) + while ( i < m_entries.size() ) { - if ( m_entries[i].generated ) + const Entry& entry = m_entries[i++]; + + if ( entry.generated ) continue; - scales.push_back(m_entries[i].bitmap.GetSize().y / baseY); + return static_cast(entry.bitmap.GetSize().y) / GetDefaultSize().y; } - return DoGetPreferredSize(scale, scales.size(), &scales[0]); + return 0.0; +} + +wxSize wxBitmapBundleImplSet::GetPreferredBitmapSizeAtScale(double scale) const +{ + return DoGetPreferredSize(scale); } wxBitmap wxBitmapBundleImplSet::GetBitmap(const wxSize& size) @@ -703,19 +708,47 @@ wxBitmapBundle::CreateImageList(wxWindow* win, // wxBitmapBundleImpl implementation // ============================================================================ -wxSize -wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, - size_t n, - const double* availableScales) const +double +wxBitmapBundleImpl::GetNextAvailableScale(size_t& WXUNUSED(i)) const { - wxCHECK_MSG( n > 0, wxSize(), wxS("must have at least one scale") ); + wxFAIL_MSG( wxS("must be overridden if called") ); + return 0.0; +} + +wxSize +wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget) const +{ double scaleBest = 0.0; double scaleLast = 0.0; - for ( size_t i = 0; i < n; ++i ) + for ( size_t i = 0;; ) { - const double scaleThis = availableScales[i]; + const double scaleThis = GetNextAvailableScale(i); + if ( scaleThis == 0.0 ) + { + // We only get here if the target scale is bigger than all the + // available scales, in which case we have no choice but to use the + // biggest bitmap, which corresponds to the last used scale that we + // should have by now. + wxASSERT_MSG( scaleLast != 0.0, "must have some available scales" ); + + // But check how far is it from the requested scale: if it's more than + // 1.5 times larger, we should still scale it, notably to ensure that + // bitmaps of standard size are scaled when 2x DPI scaling is used. + if ( scaleTarget > 1.5*scaleLast ) + { + // However scaling by non-integer scales doesn't work well at all, so + // round it to the closest integer in this case. + scaleBest = wxRound(scaleTarget); + } + else // Target scale is not much greater than the biggest one we have. + { + scaleBest = scaleLast; + } + + break; + } // Ensure we remember the last used scale value. wxON_BLOCK_EXIT_SET(scaleLast, scaleThis); @@ -735,7 +768,7 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, // We've found the closest bigger bitmap. // If there is no smaller bitmap, we have no choice but to use this one. - if ( i == 0 ) + if ( scaleLast == 0.0 ) { scaleBest = scaleThis; break; @@ -751,27 +784,6 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget, break; } - if ( scaleBest == 0.0 ) - { - // We only get here if the target scale is bigger than all the - // available scales, in which case we have no choice but to use the - // biggest bitmap, which corresponds to the last used scale. - - // But check how far is it from the requested scale: if it's more than - // 1.5 times larger, we should still scale it, notably to ensure that - // bitmaps of standard size are scaled when 2x DPI scaling is used. - if ( scaleTarget > 1.5*scaleLast ) - { - // However scaling by non-integer scales doesn't work well at all, so - // round it to the closest integer in this case. - scaleBest = wxRound(scaleTarget); - } - else // Target scale is not much greater than the biggest one we have. - { - scaleBest = scaleLast; - } - } - return GetDefaultSize()*scaleBest; } diff --git a/src/msw/bmpbndl.cpp b/src/msw/bmpbndl.cpp index fbd707f040..e9eaf87a09 100644 --- a/src/msw/bmpbndl.cpp +++ b/src/msw/bmpbndl.cpp @@ -144,6 +144,9 @@ public: virtual wxSize GetPreferredBitmapSizeAtScale(double scale) const wxOVERRIDE; virtual wxBitmap GetBitmap(const wxSize& size) wxOVERRIDE; +protected: + virtual double GetNextAvailableScale(size_t& i) const wxOVERRIDE; + private: // Load the bitmap from the given resource and add it m_bitmaps, after // rescaling it to the given size if necessary, i.e. if the needed size is @@ -189,6 +192,11 @@ wxSize wxBitmapBundleImplRC::GetDefaultSize() const return m_bitmaps[0].GetSize(); } +double wxBitmapBundleImplRC::GetNextAvailableScale(size_t& i) const +{ + return i < m_resourceInfos.size() ? m_resourceInfos[i++].scale : 0.0; +} + wxSize wxBitmapBundleImplRC::GetPreferredBitmapSizeAtScale(double scale) const { const size_t n = m_resourceInfos.size(); @@ -198,7 +206,7 @@ wxSize wxBitmapBundleImplRC::GetPreferredBitmapSizeAtScale(double scale) const scales[i] = m_resourceInfos[i].scale; } - return DoGetPreferredSize(scale, n, &scales[0]); + return DoGetPreferredSize(scale); } wxBitmap From d86c1a8c466ba1539eb61b9faa66ead8b26371eb Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 01:08:21 +0100 Subject: [PATCH 10/20] Add wxBitmapBundleImpl::GetIndexToUpscale() Ensure that wxBitmapBundleImplSet and wxBitmapBundleImplRC use the same logic for actually selecting the bitmap to upscale, and not just for deciding the size that it must have, too. No real changes, but this should make impossible for these functions to diverge once again -- and also make it simpler to reuse the same logic in any other wxBitmapBundleImpl-derived classes in the future. --- include/wx/bmpbndl.h | 14 +++++++--- interface/wx/bmpbndl.h | 46 ++++++++++++++++++++++++++++--- src/common/bmpbndl.cpp | 61 ++++++++++++++++++++---------------------- src/msw/bmpbndl.cpp | 15 +++-------- 4 files changed, 85 insertions(+), 51 deletions(-) diff --git a/include/wx/bmpbndl.h b/include/wx/bmpbndl.h index 3bf8e0b88d..1b137e588a 100644 --- a/include/wx/bmpbndl.h +++ b/include/wx/bmpbndl.h @@ -230,10 +230,16 @@ protected: // If this function is used, GetNextAvailableScale() must be overridden! wxSize DoGetPreferredSize(double scale) const; - // Override this function if DoGetPreferredSize() is used: it can use the - // provided parameter as an internal index, it's guaranteed to be 0 when - // calling this function for the first time. When there are no more scales, - // return 0. + // Helper for implementing GetBitmap(): if we need to upscale a bitmap, + // uses GetNextAvailableScale() to find the index of the best bitmap to + // use, where "best" is defined as "using scale which is a divisor of the + // given one", as upscaling by an integer factor is strongly preferable. + size_t GetIndexToUpscale(const wxSize& size) const; + + // Override this function if DoGetPreferredSize() or GetIndexToUpscale() is + // used: it can use the provided parameter as an internal index, it's + // guaranteed to be 0 when calling this function for the first time. When + // there are no more scales, return 0. // // This function is not pure virtual because it doesn't need to be // implemented if DoGetPreferredSize() is never used, but it will assert if diff --git a/interface/wx/bmpbndl.h b/interface/wx/bmpbndl.h index 795dd649f8..959919c3c4 100644 --- a/interface/wx/bmpbndl.h +++ b/interface/wx/bmpbndl.h @@ -505,16 +505,42 @@ protected: used. Typically this function is used in the derived classes implementation - to forward GetPreferredBitmapSizeAtScale() to it, e.g. + to forward GetPreferredBitmapSizeAtScale() to it and when this is done, + GetBitmap() may also use GetIndexToUpscale() to choose the bitmap to + upscale if necessary: @code class MyCustomBitmapBundleImpl : public wxBitmapBundleImpl { public: + wxSize GetDefaultSize() const + { + return wxSize(32, 32); + } + wxSize GetPreferredBitmapSizeAtScale(double scale) const wxOVERRIDE { return DoGetPreferredSize(scale); } + wxBitmap GetBitmap(const wxSize& size) wxOVERRIDE + { + // For consistency with GetNextAvailableScale(), we must have + // bitmap variants for 32, 48 and 64px sizes. + const wxSize availableSizes[] = { 32, 48, 64 }; + if ( size.y <= 64 ) + { + ... get the bitmap from somewhere ... + } + else + { + size_t n = GetIndexToUpscale(size); + bitmap = ... get bitmap for availableSizes[n] ...; + wxBitmap::Rescale(bitmap, size); + } + + return bitmap; + } + protected: double GetNextAvailableScale(size_t& i) const wxOVERRIDE { @@ -535,12 +561,26 @@ protected: */ wxSize DoGetPreferredSize(double scale) const; + /** + Return the index of the available scale most suitable to be upscaled to + the given size. + + See DoGetPreferredSize() for an example of using this function. + + @param size The required size, typically the same one as passed to + GetBitmap() + + @since 3.1.7 + */ + size_t GetIndexToUpscale(const wxSize& size) const; + /** Return information about the available bitmaps. Overriding this function is optional and only needs to be done if - DoGetPreferredSize() is called. If you do override it, this function - must return the next available scale or 0.0 if there are no more. + either DoGetPreferredSize() or GetIndexToUpscale() are called. If you + do override it, this function must return the next available scale or + 0.0 if there are no more. The returned scales must be in ascending order and the first returned scale, for the initial @a i value of 0, should be 1. The function must diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 8af75ec2a9..e73cff491a 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -298,41 +298,14 @@ wxBitmap wxBitmapBundleImplSet::GetBitmap(const wxSize& size) // We only get here if the requested size is larger than the size of all // the bitmaps we have, in which case we have no choice but to upscale one - // of the bitmaps, so find the largest available non-generated bitmap which - // can be scaled using an integer factor or, failing that, just the largest - // non-generated bitmap. - const Entry* entryToRescale = NULL; - const Entry* entryLastNotGen = NULL; - for ( size_t i = 0; i < n; ++i ) - { - const Entry& entry = m_entries[i]; - if ( entry.generated ) - continue; + // of the bitmaps, so find the most appropriate one for doing it. + const size_t i = GetIndexToUpscale(size); - entryLastNotGen = &entry; + const Entry entryNew(m_entries[i], size); - const double - scale = static_cast(size.y) / entry.bitmap.GetSize().y; - if ( scale == wxRound(scale) ) - entryToRescale = &entry; - } + m_entries.push_back(entryNew); - if ( !entryToRescale ) - entryToRescale = entryLastNotGen; - - if ( entryToRescale ) - { - const Entry entryNew(*entryToRescale, size); - - m_entries.push_back(entryNew); - - return entryNew.bitmap; - } - - // We should have at least one non-generated bitmap. - wxFAIL_MSG( wxS("unreachable") ); - - return wxBitmap(); + return entryNew.bitmap; } #ifdef __WXOSX__ @@ -787,6 +760,30 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget) const return GetDefaultSize()*scaleBest; } +size_t wxBitmapBundleImpl::GetIndexToUpscale(const wxSize& size) const +{ + // Our best hope is to find a scale dividing the given one evenly. + size_t indexBest = (size_t)-1; + + // In the worst case, we will use the largest index, as it should hopefully + // result in the least bad results. + size_t indexLast = 0; + + const wxSize sizeDef = GetDefaultSize(); + for ( size_t i = 0;; indexLast = i) + { + const double scaleThis = GetNextAvailableScale(i); + if ( scaleThis == 0.0 ) + break; + + const double scale = size.y / (sizeDef.y*scaleThis); + if (wxRound(scale) == scale) + indexBest = indexLast; + } + + return indexBest != (size_t)-1 ? indexBest : indexLast; +} + wxBitmapBundleImpl::~wxBitmapBundleImpl() { #ifdef __WXOSX__ diff --git a/src/msw/bmpbndl.cpp b/src/msw/bmpbndl.cpp index e9eaf87a09..11bea81577 100644 --- a/src/msw/bmpbndl.cpp +++ b/src/msw/bmpbndl.cpp @@ -285,20 +285,11 @@ wxBitmap wxBitmapBundleImplRC::GetBitmap(const wxSize& size) // We have to upscale some bitmap because we don't have any bitmaps larger // than the requested size. Try to find one which can be upscaled using an // integer factor. - const ResourceInfo* infoToRescale = NULL; - for ( size_t i = 0; i < m_resourceInfos.size(); ++i ) - { - const ResourceInfo& info = m_resourceInfos[i]; + const size_t i = GetIndexToUpscale(size); - const double scale = size.y / sizeDef.y*info.scale; - if ( scale == wxRound(scale) ) - infoToRescale = &info; - } + const ResourceInfo& info = m_resourceInfos[i]; - if ( !infoToRescale ) - infoToRescale = &m_resourceInfos.back(); - - return AddBitmap(*infoToRescale, sizeDef*infoToRescale->scale, size); + return AddBitmap(info, sizeDef*info.scale, size); } // ============================================================================ From 6d5bd15d12c5462cf5db7fb80bdd74e0bbc2f60d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 01:34:25 +0100 Subject: [PATCH 11/20] Use non-integer scale that is exact multiple of an available one Always rounding non-integer scale when upscaling is wrong, as we could be able to upscale an existing x1.5 image to e.g. 300% DPI scaling with relatively good results, so only do it if there is no available scale that exactly divides the requested one. Add a previously failing unit test which passes now. --- src/common/bmpbndl.cpp | 39 +++++++++++++++++++++++++++++++++--- tests/graphics/bmpbundle.cpp | 1 + 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index e73cff491a..0f83b4da24 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -711,9 +711,42 @@ wxBitmapBundleImpl::DoGetPreferredSize(double scaleTarget) const // bitmaps of standard size are scaled when 2x DPI scaling is used. if ( scaleTarget > 1.5*scaleLast ) { - // However scaling by non-integer scales doesn't work well at all, so - // round it to the closest integer in this case. - scaleBest = wxRound(scaleTarget); + // However scaling by non-integer scales doesn't work well at + // all, so try to find a bitmap that we may rescale by an + // integer factor. + // + // Note that this is similar to GetIndexToUpscale(), but we + // don't want to fall back on the largest bitmap here, so we + // can't reuse it. + // + // Also, while we reenter GetNextAvailableScale() here, it + // doesn't matter because we're not going to continue using it + // in the outer loop any more. + for ( i = 0;; ) + { + const double scale = GetNextAvailableScale(i); + if ( scale == 0.0 ) + break; + + const double factor = scaleTarget / scale; + if ( wxRound(factor) == factor ) + { + scaleBest = scaleTarget; + + // We don't need to keep going: if there is a bigger + // bitmap which can be scaled using an integer factor + // to the target scale, our GetIndexToUpscale() will + // find it, we don't need to do it here. + break; + } + } + + // If none of the bitmaps can be upscaled by an integer factor, + // round the target scale itself, as we can be sure to be able + // to scale at least the base bitmap to it using an integer + // factor then. + if ( scaleBest == 0.0 ) + scaleBest = wxRound(scaleTarget); } else // Target scale is not much greater than the biggest one we have. { diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index 174c9085ab..c40e50124a 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -301,6 +301,7 @@ TEST_CASE("BitmapBundle::GetPreferredSize", "[bmpbundle]") CHECK_THAT( BitmapAtScale(b, 3.33), SameAs(3.0, 1.5) ); CHECK_THAT( BitmapAtScale(b, 4.25), SameAs(4.0, 2.0) ); + CHECK_THAT( BitmapAtScale(b, 4.50), SameAs(4.5, 1.5) ); CHECK_THAT( BitmapAtScale(b, 5 ), SameAs(5.0, 1.0) ); } From e9b3fdaf278728cb3cdc769e37dd8878cfe99428 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 18:24:28 +0100 Subject: [PATCH 12/20] Use integer upscaling only in wxBitmapBundleImplArt This still won't work perfectly for all existing custom bitmap providers, but it shouldn't result in anything really catastrophic and should help things for the providers that can return either a normal or high resolution (i.e. x2) version of the same image. --- src/common/artprov.cpp | 48 +++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/src/common/artprov.cpp b/src/common/artprov.cpp index 24d85dd622..4038307bc8 100644 --- a/src/common/artprov.cpp +++ b/src/common/artprov.cpp @@ -154,7 +154,7 @@ public: const wxSize& size) : m_artId(id), m_artClient(client), - m_sizeDefault(GetValidSize(id, client, size)) + m_sizeDefault(size) { } @@ -165,8 +165,8 @@ public: virtual wxSize GetPreferredBitmapSizeAtScale(double scale) const wxOVERRIDE { - // We have no preferred sizes. - return m_sizeDefault*scale; + // Use the standard logic for integer-factor upscaling. + return DoGetPreferredSize(scale); } virtual wxBitmap GetBitmap(const wxSize& size) wxOVERRIDE @@ -174,33 +174,20 @@ public: return wxArtProvider::GetBitmap(m_artId, m_artClient, size); } -private: - static wxSize GetValidSize(const wxArtID& id, - const wxArtClient& client, - const wxSize& size) +protected: + virtual double GetNextAvailableScale(size_t& i) const wxOVERRIDE { - // If valid size is provided, just use it. - if ( size != wxDefaultSize ) - return size; - - // Otherwise, try to get the size we'd use without creating a bitmap - // immediately. - const wxSize sizeHint = wxArtProvider::GetSizeHint(client); - if ( sizeHint != wxDefaultSize ) - return sizeHint; - - // If we really have to, do create a bitmap just to get its size. Note - // we need the size in logical pixels here, it will be scaled later if - // 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; } +private: const wxArtID m_artId; const wxArtClient m_artClient; const wxSize m_sizeDefault; @@ -438,9 +425,12 @@ wxBitmapBundle wxArtProvider::GetBitmapBundle(const wxArtID& id, // lower priority one: even if this means that the bitmap will be // scaled, at least we'll be using the expected bitmap rather than // potentially using a bitmap of a different style. - if ( provider->CreateBitmap(id, client, size).IsOk() ) + const wxBitmap& bitmap = provider->CreateBitmap(id, client, size); + if ( bitmap.IsOk() ) { - bitmapbundle = wxBitmapBundle::FromImpl(new wxBitmapBundleImplArt(id, client, size)); + bitmapbundle = wxBitmapBundle::FromImpl( + new wxBitmapBundleImplArt(id, client, bitmap.GetSize()) + ); break; } } From 0bd2a6c10552026fbdc249d11a9833fa5c961420 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 23:30:25 +0200 Subject: [PATCH 13/20] Use correct default size in wxArtProvider::GetBitmapBundle() The returned bundle must have the specified default size, it's a postcondition of this function, but the bitmap created by the art provider does not have to be of the same size, so only use it if the desired size was not explicitly specified. --- src/common/artprov.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/common/artprov.cpp b/src/common/artprov.cpp index 4038307bc8..27e07ef535 100644 --- a/src/common/artprov.cpp +++ b/src/common/artprov.cpp @@ -428,8 +428,16 @@ wxBitmapBundle wxArtProvider::GetBitmapBundle(const wxArtID& id, const wxBitmap& bitmap = provider->CreateBitmap(id, client, size); if ( bitmap.IsOk() ) { + // The returned bitmap bundle must have the requested default + // size if it is provided, but if not, use the size of the + // bitmap, which may be different from this size. bitmapbundle = wxBitmapBundle::FromImpl( - new wxBitmapBundleImplArt(id, client, bitmap.GetSize()) + new wxBitmapBundleImplArt( + id, + client, + size.IsFullySpecified() ? size + : bitmap.GetSize() + ) ); break; } From 86c366dcaed84bc350b2a77bf8a8b9621ded82b8 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 23:37:12 +0200 Subject: [PATCH 14/20] Remove unnecessary code from wxArtProvider::CreateBitmap() Derived art providers don't need to scale the returned bitmaps as this is already done by the base class GetBitmap() anyhow if necessary. There should be no changes in behaviour, except that we may be using our own bitmap scaling instead of GTK in some cases now. --- src/gtk/artgtk.cpp | 17 +---------------- src/msw/artmsw.cpp | 19 ------------------- 2 files changed, 1 insertion(+), 35 deletions(-) diff --git a/src/gtk/artgtk.cpp b/src/gtk/artgtk.cpp index 7523452e01..985b83d77c 100644 --- a/src/gtk/artgtk.cpp +++ b/src/gtk/artgtk.cpp @@ -306,22 +306,7 @@ wxBitmap wxGTK2ArtProvider::CreateBitmap(const wxArtID& id, if (stocksize == GTK_ICON_SIZE_INVALID) stocksize = GTK_ICON_SIZE_BUTTON; - GdkPixbuf *pixbuf = CreateGtkIcon(stockid.utf8_str(), stocksize, size); - - if (pixbuf && size != wxDefaultSize && - (size.x != gdk_pixbuf_get_width(pixbuf) || - size.y != gdk_pixbuf_get_height(pixbuf))) - { - GdkPixbuf *p2 = gdk_pixbuf_scale_simple(pixbuf, size.x, size.y, - GDK_INTERP_BILINEAR); - if (p2) - { - g_object_unref (pixbuf); - pixbuf = p2; - } - } - - return wxBitmap(pixbuf); + return wxBitmap(CreateGtkIcon(stockid.utf8_str(), stocksize, size)); } wxIconBundle diff --git a/src/msw/artmsw.cpp b/src/msw/artmsw.cpp index 3acbca568f..dc597c9839 100644 --- a/src/msw/artmsw.cpp +++ b/src/msw/artmsw.cpp @@ -210,18 +210,6 @@ static wxBitmap CreateFromStdIcon(const char *iconName, wxBitmap bmp; bmp.CopyFromIcon(icon); - // The standard native message box icons are in message box size (32x32). - // If they are requested in any size other than the default or message - // box size, they must be rescaled first. - if ( client != wxART_MESSAGE_BOX && client != wxART_OTHER ) - { - const wxSize size = wxArtProvider::GetNativeSizeHint(client); - if ( size != wxDefaultSize ) - { - wxBitmap::Rescale(bmp, size); - } - } - return bmp; } @@ -251,14 +239,7 @@ wxBitmap wxWindowsArtProvider::CreateBitmap(const wxArtID& id, bitmap = MSWGetBitmapFromIconLocation(sii.szPath, sii.iIcon, sizeNeeded); if ( bitmap.IsOk() ) - { - if ( bitmap.GetSize() != sizeNeeded ) - { - wxBitmap::Rescale(bitmap, sizeNeeded); - } - return bitmap; - } } } #endif // wxHAS_SHGetStockIconInfo From 451e482d99fa23807e33744baa2af98833635a98 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 23:41:34 +0200 Subject: [PATCH 15/20] Remove useless CreateFromStdIcon() from wxMSW art provider This function has become completely trivial after the previous commit, so just remove it and create the bitmap from the icon directly in CreateBitmap(). --- src/msw/artmsw.cpp | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/msw/artmsw.cpp b/src/msw/artmsw.cpp index dc597c9839..6f18c09fdd 100644 --- a/src/msw/artmsw.cpp +++ b/src/msw/artmsw.cpp @@ -203,16 +203,6 @@ protected: const wxSize& size) wxOVERRIDE; }; -static wxBitmap CreateFromStdIcon(const char *iconName, - const wxArtClient& client) -{ - wxIcon icon(iconName); - wxBitmap bmp; - bmp.CopyFromIcon(icon); - - return bmp; -} - wxBitmap wxWindowsArtProvider::CreateBitmap(const wxArtID& id, const wxArtClient& client, const wxSize& size) @@ -284,7 +274,7 @@ wxBitmap wxWindowsArtProvider::CreateBitmap(const wxArtID& id, name = "wxICON_QUESTION"; if ( name ) - return CreateFromStdIcon(name, client); + return wxIcon(name); } // for anything else, fall back to generic provider: From 512d20c554e37b531f3ddbf17cab8db667a0f33e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 22:58:57 +0100 Subject: [PATCH 16/20] Fix confusion between width and height in wxArtProvider Amazingly, the fact that the bitmap height was compared with the required width went unnoticed even though it was there since this code was added back in e53a95bcb1 (Applied patch for ArtProvider., 2005-03-12) and even survived migration to a different file in 2b7e668221 (Move bitmap resizing from wxDefaultArtProvider to base class, 2022-02-05). That this has never been a problem seems to conclusively prove that non-square icons simply don't exist, but still fix the comparisons just to be tidy. --- src/common/artprov.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/artprov.cpp b/src/common/artprov.cpp index 27e07ef535..fff39e7370 100644 --- a/src/common/artprov.cpp +++ b/src/common/artprov.cpp @@ -309,7 +309,7 @@ wxArtProvider::RescaleOrResizeIfNeeded(wxBitmap& bmp, const wxSize& sizeNeeded) return; #if wxUSE_IMAGE - if ((bmp_h <= sizeNeeded.x) && (bmp_w <= sizeNeeded.y)) + if ((bmp_w <= sizeNeeded.x) && (bmp_h <= sizeNeeded.y)) { // the caller wants default size, which is larger than // the image we have; to avoid degrading it visually by From f56b8a99a6283ad1e4ebec1306ed03208e42f93a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Jun 2022 23:06:12 +0100 Subject: [PATCH 17/20] Allow upscaling wxArtProvider bitmaps by integer scale factor While upscaling the bitmaps by factors smaller than 2 results in truly awful results, upscaling them by 2, or 3, is better on high DPI screens than using tiny bitmaps that are difficult to read in high resolution, so do scale them up in this case. This finally makes wxBitmapBundleImplArt::GetBitmap() work as expected, as it now returns the upscaled bitmap rather than the original small bitmap in the middle of a bigger canvas. --- src/common/artprov.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/common/artprov.cpp b/src/common/artprov.cpp index fff39e7370..e2aaa50a2a 100644 --- a/src/common/artprov.cpp +++ b/src/common/artprov.cpp @@ -309,7 +309,11 @@ wxArtProvider::RescaleOrResizeIfNeeded(wxBitmap& bmp, const wxSize& sizeNeeded) return; #if wxUSE_IMAGE - if ((bmp_w <= sizeNeeded.x) && (bmp_h <= sizeNeeded.y)) + // Allow upscaling by an integer factor: this looks not too horribly and is + // needed to use reasonably-sized bitmaps in the code not yet updated to + // use wxBitmapBundle but using custom art providers. + if ((bmp_w <= sizeNeeded.x) && (bmp_h <= sizeNeeded.y) && + (sizeNeeded.x % bmp_w || sizeNeeded.y % bmp_h)) { // the caller wants default size, which is larger than // the image we have; to avoid degrading it visually by @@ -319,7 +323,7 @@ wxArtProvider::RescaleOrResizeIfNeeded(wxBitmap& bmp, const wxSize& sizeNeeded) img.Resize(sizeNeeded, offset); bmp = wxBitmap(img); } - else // scale (down or mixed, but not up) + else // scale (down or mixed, but not up, or at least not by an int factor) #endif // wxUSE_IMAGE { wxBitmap::Rescale(bmp, sizeNeeded); From 538eafc78bd6a103201a287e925f43e5b8a62f79 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Jun 2022 00:44:59 +0100 Subject: [PATCH 18/20] Fix bug with wrong GetIndexToUpscale() return value Only assign known good values of the index to "indexLast" as otherwise we could end up returning an invalid index value from it if "indexBest" was never set and the last index wasn't a valid one, as it happened in wxBitmapBundleImplSet containing an original bitmap and a x2 scaled copy of it, which shouldn't be used for further scaling. Add a unit test checking that this bug is fixed now. --- src/common/bmpbndl.cpp | 8 +++++++- tests/graphics/bmpbundle.cpp | 9 +++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/common/bmpbndl.cpp b/src/common/bmpbndl.cpp index 0f83b4da24..3316c0b14e 100644 --- a/src/common/bmpbndl.cpp +++ b/src/common/bmpbndl.cpp @@ -803,12 +803,18 @@ size_t wxBitmapBundleImpl::GetIndexToUpscale(const wxSize& size) const size_t indexLast = 0; const wxSize sizeDef = GetDefaultSize(); - for ( size_t i = 0;; indexLast = i) + for ( size_t i = 0;; ) { + // Save it before it's updated by GetNextAvailableScale(). + size_t indexPrev = i; + const double scaleThis = GetNextAvailableScale(i); if ( scaleThis == 0.0 ) break; + // Only update it now, knowing that this index could have been used. + indexLast = indexPrev; + const double scale = size.y / (sizeDef.y*scaleThis); if (wxRound(scale) == scale) indexBest = indexLast; diff --git a/tests/graphics/bmpbundle.cpp b/tests/graphics/bmpbundle.cpp index c40e50124a..c90e022e6d 100644 --- a/tests/graphics/bmpbundle.cpp +++ b/tests/graphics/bmpbundle.cpp @@ -50,6 +50,15 @@ TEST_CASE("BitmapBundle::FromBitmaps", "[bmpbundle]") CHECK( b.GetBitmap(wxSize(24, 24)).GetSize() == wxSize(24, 24) ); } +TEST_CASE("BitmapBundle::GetBitmap", "[bmpbundle]") +{ + const wxBitmapBundle b = wxBitmapBundle::FromBitmap(wxBitmap(16, 16)); + + CHECK( b.GetBitmap(wxSize(16, 16)).GetSize() == wxSize(16, 16) ); + CHECK( b.GetBitmap(wxSize(32, 32)).GetSize() == wxSize(32, 32) ); + CHECK( b.GetBitmap(wxSize(24, 24)).GetSize() == wxSize(24, 24) ); +} + // Helper functions for the test below. namespace { From 98635d1ef89e14a8b92f80262bc50af3604c6049 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Jun 2022 01:33:31 +0100 Subject: [PATCH 19/20] Document some issues involved in adding high DPI support Notably mention that wxToolBar::SetToolBitmapSize() shouldn't be used. --- docs/doxygen/overviews/high_dpi.md | 18 ++++++++++++++++++ interface/wx/toolbar.h | 7 +++++++ 2 files changed, 25 insertions(+) diff --git a/docs/doxygen/overviews/high_dpi.md b/docs/doxygen/overviews/high_dpi.md index 4d33ed102a..ffc6e8fed0 100644 --- a/docs/doxygen/overviews/high_dpi.md +++ b/docs/doxygen/overviews/high_dpi.md @@ -285,6 +285,24 @@ XRC format has been updated to allow specifying wxBitmapBundle with multiple bitmaps or a single SVG image can be used. +Adapting Existing Code To High DPI {#high_dpi_existing_code} +================================== + +Generally speaking, adding support for high DPI to the existing wxWidgets +programs involves doing at least the following: + +1. Not using any hard-coded pixel values outside of `FromDIP()` (note that + this does _not_ apply to XRC). +2. Using wxBitmapBundle containing at least 2 (normal and high DPI) bitmaps + instead of wxBitmap and wxImageList when setting bitmaps. +3. Updating any custom art providers to override + wxArtProvider::CreateBitmapBundle() (and, of course, return multiple bitmaps + from it) instead of wxArtProvider::CreateBitmap(). +4. Removing any calls to wxToolBar::SetToolBitmapSize() or, equivalently, + `` attributes from the XRC, as this forces unwanted scaling. + + + Platform-Specific Build Issues {#high_dpi_platform_specific} ============================== diff --git a/interface/wx/toolbar.h b/interface/wx/toolbar.h index 8392a2cec7..c021c1ec56 100644 --- a/interface/wx/toolbar.h +++ b/interface/wx/toolbar.h @@ -882,6 +882,10 @@ public: It is usually unnecessary to call this function, as the tools will always be made big enough to fit the size of the bitmaps used in them. + Moreover, calling it may force wxToolBar to scale its images, even + using non-integer scaling factor, which will usually look bad, instead + of adapting the image size to the current DPI scaling in order to avoid + doing this. If you do call it, it must be done before toolbar is Realize()'d. @@ -894,6 +898,9 @@ public: toolbar->Realize(); @endcode + Note that this example would scale bitmaps to 48 pixels when using 150% + DPI scaling, which wouldn't happen without calling SetToolBitmapSize(). + @param size The size of the bitmaps in the toolbar in logical pixels. From 38dfcf05aba7b59cfabe1d6f38ad365dd756297b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 4 Jun 2022 15:39:47 +0100 Subject: [PATCH 20/20] Handle non-default scale factor in wxBitmapBundleImplArt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing code only worked correctly if overridden wxArtProvider::CreateBitmap() returned bitmaps with scale factor of 1, but not for anything else. Account for the bitmap scale factor when determining the default size and available scales correctly to fix this. Co-authored-by: Václav Slavík --- src/common/artprov.cpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/common/artprov.cpp b/src/common/artprov.cpp index e2aaa50a2a..1cde929a62 100644 --- a/src/common/artprov.cpp +++ b/src/common/artprov.cpp @@ -149,12 +149,19 @@ namespace class wxBitmapBundleImplArt : public wxBitmapBundleImpl { public: - wxBitmapBundleImplArt(const wxArtID& id, + wxBitmapBundleImplArt(const wxBitmap& bitmap, + const wxArtID& id, const wxArtClient& client, - const wxSize& size) + const wxSize& sizeRequested) : m_artId(id), m_artClient(client), - m_sizeDefault(size) + // The bitmap bundle must have the requested size if it was + // specified, but if it wasn't just use the (scale-independent) + // bitmap size. + m_sizeDefault(sizeRequested.IsFullySpecified() + ? sizeRequested + : bitmap.GetDIPSize()), + m_bitmapScale(bitmap.GetScaleFactor()) { } @@ -182,15 +189,16 @@ protected: // 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; + // return the original bitmap scale, but hope that perhaps the provider + // will have other (e.g. x2) scales too, when our GetBitmap() is called. + return i++ ? 0.0 : m_bitmapScale; } private: const wxArtID m_artId; const wxArtClient m_artClient; const wxSize m_sizeDefault; + const double m_bitmapScale; wxDECLARE_NO_COPY_CLASS(wxBitmapBundleImplArt); }; @@ -432,16 +440,8 @@ wxBitmapBundle wxArtProvider::GetBitmapBundle(const wxArtID& id, const wxBitmap& bitmap = provider->CreateBitmap(id, client, size); if ( bitmap.IsOk() ) { - // The returned bitmap bundle must have the requested default - // size if it is provided, but if not, use the size of the - // bitmap, which may be different from this size. bitmapbundle = wxBitmapBundle::FromImpl( - new wxBitmapBundleImplArt( - id, - client, - size.IsFullySpecified() ? size - : bitmap.GetSize() - ) + new wxBitmapBundleImplArt(bitmap, id, client, size) ); break; }