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.

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

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

pdesaulniers
Copy link
Contributor

@pdesaulniers pdesaulniers commented Apr 30, 2019

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 a loadFromSVG(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.
2019-04-29-20:03:55

Copy link
Contributor Author

@pdesaulniers pdesaulniers left a 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);
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.";
Copy link
Contributor Author

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)
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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"
Copy link
Contributor

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];
Copy link
Contributor

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),
Copy link
Contributor

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

@pdesaulniers
Copy link
Contributor Author

@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.

@pdesaulniers
Copy link
Contributor Author

Hold on, it seems like I broke SVG scaling. I'll need to investigate this tomorrow.

@pdesaulniers
Copy link
Contributor Author

pdesaulniers commented May 1, 2019

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.

@falkTX
Copy link
Contributor

falkTX commented Jul 12, 2019

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.
So DPF remains small, but we can have the community bring some useful stuff in there.

No time for this now, just letting you know.
I welcome proposals for the repo name :)

@pdesaulniers
Copy link
Contributor Author

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.

@pdesaulniers
Copy link
Contributor Author

For demonstration purposes, I have updated the piano widget to use keycodes instead of hard-coded QWERTY notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants