Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retina Bitmaps #259

Merged
merged 2 commits into from
Jan 12, 2019
Merged

Retina Bitmaps #259

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aeba1d7
Retina Bitmaps
baconpaul Jan 8, 2019
5c77a49
Respond to @jsakkine review comments with diffs.
baconpaul Jan 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion scripts/macOS/package-au.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ cp -R "$BUNDLE_RES_SRC_LOCATION" "$BUNDLE_DIR/Contents/Resources"
if [[ -z "$SURGE_USE_VECTOR_SKIN" ]]; then
cp $BITMAP_SRC_LOCATION/* "$BUNDLE_DIR/Contents/Resources/"
else
rm "$BUNDLE_DIR/Contents/Resources/bmp?????.png"
rm "$BUNDLE_DIR/Contents/Resources/bmp*.png"
baconpaul marked this conversation as resolved.
Show resolved Hide resolved
cp $VECTOR_BITMAP_SRC_LOCATION/bmp?????.png "$BUNDLE_DIR/Contents/Resources/"
mkdir "$BUNDLE_DIR/Contents/Resources/scalable"
cp $VECTOR_BITMAP_SRC_LOCATION/*png "$BUNDLE_DIR/Contents/Resources/scalable"
fi

5 changes: 4 additions & 1 deletion scripts/macOS/package-vst.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ cp -R "$BUNDLE_RES_SRC_LOCATION" "$BUNDLE_DIR/Contents/Resources"
if [[ -z "$SURGE_USE_VECTOR_SKIN" ]]; then
cp $BITMAP_SRC_LOCATION/* "$BUNDLE_DIR/Contents/Resources/"
else
rm "$BUNDLE_DIR/Contents/Resources/bmp?????.png"
rm "$BUNDLE_DIR/Contents/Resources/bmp*.png"
cp $VECTOR_BITMAP_SRC_LOCATION/bmp?????.png "$BUNDLE_DIR/Contents/Resources/"
mkdir "$BUNDLE_DIR/Contents/Resources/scalable"
cp $VECTOR_BITMAP_SRC_LOCATION/*png "$BUNDLE_DIR/Contents/Resources/scalable"
fi


4 changes: 3 additions & 1 deletion scripts/macOS/package-vst3.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ cp -R "$BUNDLE_RES_SRC_LOCATION" "$BUNDLE_DIR/Contents/Resources"
if [[ -z "$SURGE_USE_VECTOR_SKIN" ]]; then
cp $BITMAP_SRC_LOCATION/* "$BUNDLE_DIR/Contents/Resources/"
else
rm "$BUNDLE_DIR/Contents/Resources/bmp?????.png"
rm "$BUNDLE_DIR/Contents/Resources/bmp*.png"
cp $VECTOR_BITMAP_SRC_LOCATION/bmp?????.png "$BUNDLE_DIR/Contents/Resources/"
mkdir "$BUNDLE_DIR/Contents/Resources/scalable"
cp $VECTOR_BITMAP_SRC_LOCATION/*png "$BUNDLE_DIR/Contents/Resources/scalable"
fi

63 changes: 63 additions & 0 deletions src/common/gui/CScalableBitmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#include "CScalableBitmap.h"
#if MAC
#include <CoreFoundation/CoreFoundation.h>
#endif

CScalableBitmap::CScalableBitmap( CResourceDescription desc ) : CBitmap( desc )
{
int id = 0;
if( desc.type == CResourceDescription::kIntegerType )
id = (int32_t)desc.u.id;

scales = {{ 100, 200 }};
scaleFilePostfixes[ 100 ] = "";
scaleFilePostfixes[ 200 ] = "@2x";

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or?

    scales = {{100, 200}};
    scaleFilePostfixes[100] = "";
    scaleFilePostfixes[200] = "@2x";

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the numbers are not 1 and 2 but instead multiplied by a factor 100?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why you need all this complexity given that you have only two different sizes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per slack convo I will add a comment here explaining a bit more

//fprintf( stderr, "\n" );
for( auto sc : scales )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is commented out cruft that ought to be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I left those in since I want them back for debugging when I go to 1.5x 3x and 4x

{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (auto sc : scales) ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird spacing is everywhere in this commit...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

auto postfix = scaleFilePostfixes[ sc ];
char filename [PATH_MAX];
sprintf (filename, "scalable/bmp%05d%s.png", id, postfix.c_str() );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sprintf() vs sprintf ()?

Copy link
Collaborator

@jarkkojs jarkkojs Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


CBitmap *tmp = new CBitmap( CResourceDescription( filename ) );
//fprintf( stderr, "%s, Scale=%d, Dim=(%lf x %lf)\n", filename, sc, tmp->getWidth(), tmp->getHeight() );

if( tmp->getWidth() > 0 )
{
scaledBitmaps[ sc ] = tmp;
}
else
{
scaledBitmaps[ sc ] = NULL;
}

}
}

void CScalableBitmap::draw (CDrawContext* context, const CRect& rect, const CPoint& offset, float alpha )
{
/*
** For now this is a fixed 'retina' style draw of using 2x bitmaps at 0.5 scale if they exist
*/
if( scaledBitmaps[ 200 ] != NULL )
{
CGraphicsTransform tf = CGraphicsTransform().scale( 0.5, 0.5 );
CGraphicsTransform invtf = tf.inverse();

CDrawContext::Transform tr( *context, tf );

// Have to de-const these to do the transform alas
CRect ncrect = rect;
CRect nr = invtf.transform( ncrect );

CPoint ncoff = offset;
CPoint no = invtf.transform( ncoff );

scaledBitmaps[ 200 ]->draw( context, nr, no, alpha );
}
else
{
CBitmap::draw( context, rect, offset, alpha );
}
}
21 changes: 21 additions & 0 deletions src/common/gui/CScalableBitmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
** CScalableBitmap is an implmentation of VSGTUI::CBitmap which can
** load bitmaps at multiple resolutions and draw them scaled accordingly
*/

#include <vstgui/vstgui.h>

#include <vector>
#include <map>

class CScalableBitmap : public VSTGUI::CBitmap
{
public:
CScalableBitmap( CResourceDescription d );

std::vector< int > scales; // 100, 150, 200, 300 etc... - int percentages
std::map< int, VSTGUI::CBitmap * > scaledBitmaps;
std::map< int, std::string > scaleFilePostfixes;

virtual void draw (CDrawContext* context, const CRect& rect, const CPoint& offset, float alpha);
};
14 changes: 13 additions & 1 deletion src/common/gui/SurgeBitmaps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
#include "SurgeBitmaps.h"
#include <map>

#if MAC && TARGET_AUDIOUNIT // for now
#define USE_SCALABLE_BITMAPS 1
#endif

#ifdef USE_SCALABLE_BITMAPS
#include "CScalableBitmap.h"
#endif

std::map<int, VSTGUI::CBitmap*> bitmap_registry;

static std::atomic_int refCount(0);
Expand All @@ -13,7 +21,7 @@ SurgeBitmaps::SurgeBitmaps()
addEntry(IDB_BUTTON_ABOUT);
addEntry(IDB_ABOUT);
addEntry(IDB_FILTERBUTTONS);
addEntry(IDB_OSCSWITCH);
addEntry(IDB_OSCSWITCH);
addEntry(IDB_FILTERSUBTYPE);
addEntry(IDB_RELATIVE_TOGGLE);
addEntry(IDB_OSCSELECT);
Expand Down Expand Up @@ -72,7 +80,11 @@ void SurgeBitmaps::addEntry(int id)
{
assert(bitmap_registry.find(id) == bitmap_registry.end());

#ifdef USE_SCALABLE_BITMAPS
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

VSTGUI::CBitmap *bitmap = new CScalableBitmap(CResourceDescription(id));
#else
VSTGUI::CBitmap* bitmap = new VSTGUI::CBitmap(CResourceDescription(id));
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you always use CScalableBitmap even if it does not make difference? Do not understand this complexity...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bitmap_registry[id] = bitmap;
}

Expand Down