Retina Bitmaps #259
Retina Bitmaps #259
Conversation
If scalable bitmaps are present we can begin to implement zoom. Step one is to implement Mac Retina display. We do that here by loading 2x bitmaps if available and drawing them at 0.5x scale. On retina displays this looks great. On regular displays it is a no-op basically. See surge-synthesizer#226 for more. None of this is activated if the skins aren't installed and by default it falls back to the standard bitmaps.
| scales = {{ 100, 200 }}; | ||
| scaleFilePostfixes[ 100 ] = ""; | ||
| scaleFilePostfixes[ 200 ] = "@2x"; | ||
|
|
There was a problem hiding this comment.
Or?
scales = {{100, 200}};
scaleFilePostfixes[100] = "";
scaleFilePostfixes[200] = "@2x";
There was a problem hiding this comment.
Why the numbers are not 1 and 2 but instead multiplied by a factor 100?
There was a problem hiding this comment.
And why you need all this complexity given that you have only two different sizes?
There was a problem hiding this comment.
I would just have something like
int scaleFilePostfixes[2];
VSTGUI::CBitmap *scaleBitmaps[2];
The need to use std::map comes only from the fact that you have factored your scales.
There was a problem hiding this comment.
I don't have 2 sizes; I have 4 or 5 (1, 1.5, 2, 3, 4 at least). I chose an int because the 1.5 size is a float and I didn't want to hash float. When we do zoom properly this will be a longer list so I built it to scale to that.
There was a problem hiding this comment.
Per slack convo I will add a comment here explaining a bit more
src/common/gui/CScalableBitmap.cpp
Outdated
| { | ||
| auto postfix = scaleFilePostfixes[ sc ]; | ||
| char filename [PATH_MAX]; | ||
| sprintf (filename, "scalable/bmp%05d%s.png", id, postfix.c_str() ); |
There was a problem hiding this comment.
sprintf() vs sprintf ()?
There was a problem hiding this comment.
BTW, why not just std::string for filename? If we are in C++, why don't we then just use proudly std::ostringstream? I mean in many companies, projects and organizations (many of the have so called secure programming practices) the of sprintf() is strictly forbidden because it does not do any range checks. When using libc, the practice is to always use snprintf(). I understand this kind of security measures are maybe an overkill for a synth plug-in but high-level C++ constructs would provide all the security measures in an easy-to-use package.
| @@ -72,7 +80,11 @@ void SurgeBitmaps::addEntry(int id) | |||
| { | |||
| assert(bitmap_registry.find(id) == bitmap_registry.end()); | |||
|
|
|||
| #ifdef USE_SCALABLE_BITMAPS | |||
There was a problem hiding this comment.
BTW, could add some prefix for all feature flags i.e. compilation flags that enable/disable a feature. Actually USE_ is quite common and would be fine but can be take it as a convention?
|
|
||
| //fprintf( stderr, "\n" ); | ||
| for( auto sc : scales ) | ||
| { |
There was a problem hiding this comment.
for (auto sc : scales) ?
There was a problem hiding this comment.
Weird spacing is everywhere in this commit...
There was a problem hiding this comment.
You mean "correct spacing is everywhere in this commit and the rest of the world uses weird spacing" surely ;)
That spacing will never leave my fingers. I'll clean it up when I clean up other review comments.
src/common/gui/CScalableBitmap.cpp
Outdated
| scaleFilePostfixes[ 200 ] = "@2x"; | ||
|
|
||
| //fprintf( stderr, "\n" ); | ||
| for( auto sc : scales ) |
There was a problem hiding this comment.
There is commented out cruft that ought to be removed?
There was a problem hiding this comment.
Yeah I left those in since I want them back for debugging when I go to 1.5x 3x and 4x
| VSTGUI::CBitmap* bitmap = new VSTGUI::CBitmap(CResourceDescription(id)); | ||
| #endif |
There was a problem hiding this comment.
Couldn't you always use CScalableBitmap even if it does not make difference? Do not understand this complexity...
There was a problem hiding this comment.
The "if mac" has to go somewhere until we sort out resources and loading them on windows and linux. I put it outside the class. I could put it inside the class. Outside the class seemed cleaner.
Activate for VST2 and VST3 also, which was in the scripts, but not the code.
|
OK I incorporated @jsakkine review comments and had a lengthy conversation with him about it on slack. As a step on the way and as optionally off, he understood. So I'll merge this so I can put skin switchers in now we have @itsmedavep skin. |
…s-226 Retina Bitmaps Former-commit-id: becb7af8c2a497ac23ec072b85500aa6fe3a5dd4 [formerly 26adc86] Former-commit-id: 1321aff292859206bc8d0647825f2a36077caa76 Former-commit-id: c0936314c03067958a8ac586b713e110ba2d5395
@kurasu and others - I would really appreciate a review of this. While it works, I made design choices you may not like, so please take a look. I will not sweep it without a positive ack from @kurasu.
And note that in default configuration this does nothing.
If scalable bitmaps are present we can begin to implement zoom.
Step one is to implement Mac Retina display. We do that here
by loading 2x bitmaps if available and drawing them at 0.5x
scale. On retina displays this looks great. On regular displays
it is a no-op basically. See #226 for more.
None of this is activated if the skins aren't installed and
by default it falls back to the standard bitmaps.