-
-
Notifications
You must be signed in to change notification settings - Fork 97
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.
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
Add SVG support + basic MidiKeyboard example #141
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few comments in the PR.
By the way, sorry if this PR is too big. I don't mind splitting it in smaller parts if you want.
{ | ||
DISTRHO_SAFE_ASSERT_RETURN(svg.isValid(),) | ||
|
||
loadFromMemory((const char*)svg.getRGBAData(), svg.getSize(), GL_RGBA); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty bad, as the RGBA data becomes invalid when the SVG is freed. The caller needs to keep both the SVG and the Image 'alive'.
What should we do instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a parameter to allocate memory. if true, do malloc and copy data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like this would be a pretty wasteful copy. I guess it could be avoided if there was an object that combined both SVG and Image.
*/ | ||
const char* getDescription() const override | ||
{ | ||
return "Plugin that demonstrates how to make a MIDI keyboard widget using DPF."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this description is a bit clumsy.
Get the key that is under the specified point. | ||
Return nullptr if the point is not hovering any key. | ||
*/ | ||
PianoKey* tryGetHoveredKey(const Point<int>& point) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could easily be optimized, but this approach has the benefit of being simple I guess...
|
||
FILES_UI = \ | ||
resources/MidiKeyboardResources.cpp \ | ||
MidiKeyboardExampleUI.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The KeyboardWidget should probably be split into hpp/cpp files.
ifeq ($(HAVE_OPENGL),true) | ||
TARGETS += lv2_sep | ||
else | ||
TARGETS += lv2_dsp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example doesn't really make any sense without OpenGL support.
/** | ||
Called when a note is pressed on the piano. | ||
*/ | ||
void keyboardKeyPressed(const uint keyIndex) override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there doesn't need to be a separate callback for both 'pressed' and 'released', but I couldn't find a proper name for that callback...
dgl/SVG.hpp
Outdated
#ifndef DGL_SVG_HPP_INCLUDED | ||
#define DGL_SVG_HPP_INCLUDED | ||
|
||
#include "src/nanosvg/nanosvg.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This include is not really needed
dgl/src/SVG.cpp
Outdated
// nsvgParse modifies the input data, so we must use a temporary buffer | ||
const size_t bufferSize = dataSize * 4; | ||
|
||
char tmpBuffer[bufferSize]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might segfault if stack size becomes too big.
as this is not a RT operation, allocating memory seems safer
public: | ||
/* constructor */ | ||
MidiKeyboardExampleUI() | ||
: UI(800, 132), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put the initial width and height as static vars, and then use them in the constructor.
This way we avoid calling getWidth
, as we already know what it is anyway
@falkTX I've polished a few things, and added some way to play the piano using the computer keyboard (very rudimentary for now, will be improved in another PR). I guess you can start merging the PR piece by piece. At least, the keyboard widget seems to behave as expected, and I'm not aware of any terrible issue in the code. |
Hold on, it seems like I broke SVG scaling. I'll need to investigate this tomorrow. |
Nevermind, seems like it's working fine. Scaling would make the keyboard bigger than the UI, and the keyboard would end up outside of the UI due to a uint underflow. |
After some thought I decided that it is best to place these sorta widgets into a new repo, like DPF-Widgets or similar in name. No time for this now, just letting you know. |
Good. Perhaps it would make sense to create a "DISTRHO-Community" organization, and add the repo there. I guess the repo could contain more than just widgets (utility classes, DSP stuff, etc). In that case, the name "DPF-Widgets" would be too specific. As for the piano widget: even if it ends up in a different repo, it would still benefit from key-codes support in pugl. I'll try to create a PR for this soon. |
For demonstration purposes, I have updated the piano widget to use keycodes instead of hard-coded QWERTY notes. |
4fa52aa
to
bd702d4
Compare
7599a6e
to
52c2e78
Compare
59cc6ed
to
470c5b7
Compare
8507aec
to
7cd4819
Compare
This PR adds the ability to load and display SVGs in DGL UIs. It also adds an example that demonstrates this feature, along with
sendNote
.The loading and rastering is done using NanoSVG. For now, I've added an
SVG
class, along with aloadFromSVG(const SVG& svg)
method in the Image class. Later, there should also be some way to display an SVG as a NanoImage or a Cairo image.There is something important to consider: right now, SVGs are converted to RGBA arrays. This means that we need to recreate a new image every time we want to change the scale of the image, otherwise it won't look crisp. This is not really compatible with the current 'scaling' feature of DGL, which will make the SVG look blurry after scaling, like a standard image.
As for the MidiKeyboard example, it's a really bare-bones keyboard that sends notes from C4 to C6.