Add Qt toolkit #693

yeonoson · 2017-09-17T17:25:20Z

Hello everyone,

this pull request adds support for the Qt toolkit.

In the last months I've examined some music engraving libraries and found Verovio to be the best suitable. Its layouting engine with the collision detection works really well and the code is clean and well structured.

I started by using Verovio as is and did all the rendering to SVG. Unfortunately SVG support in Qt is not the best, e.g. only supports SVG Tiny (see #332) and does not allow to update already painted elements (except by repainting). Therefore I quickly came up with the idea to provide a custom DeviceContext for Qt to directly draw to Qt's scene-graph and profit from direct OpenGL painting.

As I did not want to bloat the clean C++ library with conditionals I created the verovio-qt library as a wrapper around the original library. The main part is the QtSceneGraphDeviceContext which inherits the abstract DeviceContext. A user of the verovio-qt library uses the VerovioDoc and VerovioPage classes. They can also directly be used in QML. To showcase the usage of the verovio-qt library I also included a small demo application.

The Qt toolkit is mainly feature complete. I left out some rarely used draw instructions such as DrawEllipticArc.

Best regards

Jonathan

- Source of Bravura-1.204.otf: https://www.smufl.org/files/bravura-1.204.zip - Source of Leipzig-5.2.ttf: Converted from Leipzig-5.2.sfd with https://everythingfonts.com/sfd-to-ttf License: Both fonts are licensed under the SIL Open Font License.

lpugin · 2017-09-21T18:53:58Z

Thanks for this, it looks pretty good! Could you fill the CLA https://github.com/rism-ch/verovio/blob/master/doc/contributing.md ?

yeonoson · 2017-09-21T19:58:47Z

I just sent the CLA to info@rism-ch.org

lpugin · 2017-09-21T20:47:24Z

Thanks. I can compile both the library and the demo but the demo crashes on

view.setSource(QUrl("qrc:/main.qml"));

Any clue?

lpugin · 2017-09-21T20:51:54Z

qt/lib/qtscenegraphdevicecontext.h

+#define __QTSCENEGRAPHDEVICECONTEXT_H__
+
+//----------------------------------------------------------------------------
+


I had to include <cmath> for this file to compile

I added <cmath> to the .cpp file which uses std::sin etc. I couldn't find anything in the .h file that might require .

yeonoson · 2017-09-21T20:57:24Z

Which version of Qt are you using? I mainly tried it with 5.9.1, and did a bit of testing with 5.8.0.
You could try to reduce the multisample antialiasing value in main.cpp:34 or just comment that line.

lpugin · 2017-09-21T21:01:23Z

I am using 5.9.1. Commenting avoiding it to crash. I now shows an empty window. Do I need to pass a file as parameter?

yeonoson · 2017-09-21T21:04:28Z

It should actually start with the only MEI file in the project: "doc/tests/mei/01_mensural/01_durations.mei"
If not you can open the Options (button at the top right) to choose a file or drag-and-drop a file.

That's what it should look like:

yeonoson · 2017-09-22T06:37:20Z

The last commit fixes some issues with the multisample antialiasing, especially on macOS where some things have to be done at an earlier stage.
If you still run into problem with an empty window: Which OS are you using? Are there any messages to the console output in QtCreator?

lpugin · 2017-09-22T14:24:44Z

The issue was actually the running directory that had to be set to ./qt/demo (the fonts where not loaded). This is now what I see

It is getting there but the font size it wrong. I am on OS X 10.12.5

…ll OS.

yeonoson · 2017-09-22T15:16:20Z

That looks familiar... I had the same problem when running the demo on Android. The problem is that the point size is given in pixels which I try to convert to point size for having higher accuracy.

But as this seems to be platform-dependent, I've changed it back to pixel size for now. A better solution would be if Verovio could use point size throughout. This would also make more sense because the functions in FontInfo are called GetPointSize().

…urce path and file content.

craigsapp · 2017-09-23T04:35:10Z

I get this after compiling:

I am using MacOS 10.12.5 and Qt 5.9.1.

The application is 80 KB, are the dynamic libraries and/or the verovio fonts also supposed to be include in it?

The application error was that the dynamic library verovio-qt.1.dylib was not found:

I had compiled it in .../qt/lib and symbolically linked verovio-qt.1.dylib to .../qt/build

Can the libraries be included in the application and/or compiled statically into the app executable?

Where will the fonts be accessed? It would also probably be useful to include in the app rather relying on the system installation since there could be different versions of verovio associated with different compiled apps.

lpugin · 2017-09-24T07:10:04Z

qt/lib/include/verovio-qt/veroviodoc.h

+/**
+ * This class represents one Verovio document.
+ */
+class VerovioDoc : public QObject {


Since this class is a wrapper around the Verovio toolkit I would not call it VerovioDoc but instead something like QtVrvToolkit

Yes, that makes sense. Instead of using a namespace in the class name (QtVrvToolkit) I've decided to put all classes of verovio-qt in its own namespace vrv_qt. I first thought about using a nested namespace (vrv::qt) but that leads to indenting everying inside of that namespace by the used clang-format.
The file has been renamed to verovioqttoolkit.h/cpp. I would have preferred to just name it toolkit.h/cpp but that's not possible because the Verovio library requires include/vrv to be in the INCLUDEPATH and thus pollutes the . include path.

lpugin · 2017-09-24T07:11:25Z

qt/lib/include/verovio-qt/veroviodoc.h

+    void setFileName(QString fileName);
+    void setFileContent(QString fileContent);
+    void setMusicFont(QString musicFont);
+    void setPageWidth(int pageWidth);


Why not having one single method for a this, namely setOptions that takes a JSON string an pass it to the vrv::toolkit?

These setters are directly used by the Q_PROPERTY definition which exposes the fields as QML attributes. If using a JSON string instead, constructing the required options from QML would be a lot more cumbersome.
See: http://doc.qt.io/qt-5/qtqml-cppintegration-exposecppattributes.html

lpugin · 2017-09-24T07:12:05Z

qt/lib/include/verovio-qt/veroviodoc.h

+
+private:
+    // configuration parameters for Verovio
+    int m_pageWidth;


With setOptions passed to the toolkit I think you would not need to have all these members duplicated.

That's a good idea. Even without using a JSON string all function calls can be directly delegated to the corresponding function in vrv::Toolkit.
One exception is the pageWidth and pageHeight which actually have a different meaning in verovio-qt, because they take into account the current scale. Thus I renamed them to displayWidth and displayHeight.

lpugin · 2017-09-24T07:15:02Z

qt/lib/include/verovio-qt/veroviopage.h

+/**
+ * This class represents one page of a Verovio document.
+ */
+class VerovioPage : public QQuickItem {


I am not sure about the name either. It creates confusion with vrv::Page. It looks more like a View to me. So maybe QtVrvView?

I got confused by view_page and page, so yet this class actually represents a vrv::View. Thus I renamed it to vrv_qt::View.

lpugin · 2017-09-24T07:16:57Z

tools/CMakeLists.txt

+         ${verovio_SRC}
+         ${hum_SRC}
+         ${midi_SRC}
+             ../src/pugi/pugixml.cpp


We have too much code duplication here. We need to find a way to have the list of common source files in a variable.

Changed that!

In addition I've added a new custom target for the header files. That is required for QtCreator, otherwise all .h files are not shown in the project tree. The custom target should not have any consequences to other IDEs.

lpugin · 2017-09-24T07:22:35Z

We need the demo to be the most basic possible example. Just loading a file and displaying it. The current demo is great but should sit somewhere else, maybe in another repository.

We need to add a note for the license compliance to make it clear that this can be used only with the LGPL version of Qt (and not the commercial license).

Finally, we will not be able to maintain this at the RISM-CH and will have to rely on the community.

yeonoson · 2017-09-24T17:32:24Z

@craigsapp: If you compile and run the demo with QtCreator, it takes care of setting the library path. Just make sure that in the project settings -> Run, the option "Add library path to LD_LIBRARY_PATH" (or similar) is enabled.

If you want to run the demo without QtCreator you have to specify the library path yourself. On Mac you can use DYLD_LIBRARY_PATH for this. As I don't have a Mac I can't tell you the exact commands, but maybe @lpugin can help with this.

For reference, on Linux, starting the verovio-qt-demo works with the following command:

LD_LIBRARY_PATH="../../build;../build" ./verovio-qt-demo

This has to be executed in the directory of the verovio-qt-demo. ../../build specifies the directory with libverovio.so and ../build the directory with libverovio-qt.so.

Alternatively copying the two libraries to the directory of verovio-qt-demo and starting it as follows also works:

LD_LIBRARY_PATH=. ./verovio-qt-demo

Using static libraries would of course also work, but as Verovio is licensed under LGPL, using dynamic linking should be the more popular case.

In my commit, just before your comment, I've added the QML properties resourcesDataPath and fontDirPath that can be used to specify the font-related directories.

- Strip down verovio-qt-demo - Rename classes - Remove configuration class members from vrv_qt::Toolkit - Improve Verovio's CMakeLists.txt

yeonoson · 2017-09-24T19:54:27Z

@lpugin: Thank you very much for your valuable feedback.

I've stripped down the verovio-qt-demo a lot and considered your feedback with my latest commit.

Regarding the commercial Qt license: I am not a lawyer, but to my understanding the license of Qt does not matter as long as verovio and verovio-qt are included as shared libraries and thus allow it to be replaced by another version of that library.

lpugin

Thanks for the previous changes. A few more requests here. Also, changing the font to Leipzig or Gootville does not work for me (it shows squares for all glyphs). Any idea?

lpugin · 2017-09-25T07:14:38Z

include/vrv/toolkit.h

+     * Render the page to the deviceContext.
+     * Page number is 1-based.
+     */
+    bool RenderToCustomDevice(int pageNo, DeviceContext *deviceContext);


Please rename this to ::RenderToDeviceContext

lpugin · 2017-09-25T07:16:27Z

qt/lib/include/verovio-qt/textquickitem.h

+// Copyright (c) Authors and others. All rights reserved.
+/////////////////////////////////////////////////////////////////////////////
+
+#ifndef __TEXTQUICKITEM_H__


Should be __VRV_QT_TEXTQUICKITEM_H__

lpugin · 2017-09-25T07:18:02Z

qt/lib/include/verovio-qt/qtscenegraphdevicecontext.h

+// Copyright (c) Authors and others. All rights reserved.
+/////////////////////////////////////////////////////////////////////////////
+
+#ifndef __QTSCENEGRAPHDEVICECONTEXT_H__


Should be __VRV_QT_SCENEGRAPH_DC_H__

lpugin · 2017-09-25T07:18:47Z

qt/lib/include/verovio-qt/qtscenegraphdevicecontext.h

+
+#include "devicecontext.h"
+
+namespace vrv_qt {


change to vrvQt and also match the directory to ./include/vrvqt (and not verovio-qt)

lpugin · 2017-09-25T07:21:13Z

qt/lib/include/verovio-qt/verovioqttoolkit.h

+// Copyright (c) Authors and others. All rights reserved.
+/////////////////////////////////////////////////////////////////////////////
+
+#ifndef __VEROVIOQTTOOLKIT_H__


Should be __VRV_QT_TOOLKIT_H__. Also, since there is the namespace it should now be just toolkit.h/cpp

I already commented on a previous change request from you that I'd also prefer toolkit.h/cpp, but thats not possible:

The file has been renamed to verovioqttoolkit.h/cpp. I would have preferred to just name it toolkit.h/cpp but that's not possible because the Verovio library requires include/vrv to be in the INCLUDEPATH and thus pollutes the . include path.

Changing that would require a bigger refactoring of the Verovio library:

Do not specify include/vrv, include/midi, ... as include directories but only include/.

Change all .h/.cpp files to include the subdirectory of the main include. E.g. #include "vrv/page.h" instead of #include "page.h"

I see. Since we are in the extension of the Verovio library I would suggest qttoolkit.h/cpp. OK?

In fact you could refactor it the other way, that is, have only INCLUDEPATH += include/ in the Qt project (and not include/vrvqt) and then do #include "vrvqt/toolkit.h". I am fine either way.

You can have both path included, and then use give the subdirectory only for files that have a on conflict.

INCLUDEPATH += include/ INCLUDEPATH += include/vrvqt { INCLUDEPATH += ../../include/ INCLUDEPATH += ../../include/vrv INCLUDEPATH += ../../include/pugi INCLUDEPATH += ../../include/utf8 INCLUDEPATH += ../../libmei LIBS += -L../../build -lverovio }

And then do either:

#include "vrv/toolkit.h" #include "vrvqt/toolkit.h"

For other files:

#include "textquickitem.h"

lpugin · 2017-09-25T07:22:04Z

qt/lib/include/verovio-qt/verovioqtview.h

+// Copyright (c) Authors and others. All rights reserved.
+/////////////////////////////////////////////////////////////////////////////
+
+#ifndef __VEROVIOQTVIEW_H__


Same here (just vrvQt::View) and __VRV_QT_VIEW_H__

lpugin · 2017-09-25T09:43:20Z

qt/lib/include/verovio-qt/verovioqttoolkit.h

+/**
+ * This class is a wrapper around the Verovio Toolkit (vrv::Toolkit).
+ */
+class Toolkit : public QObject {


One more comment: I think you could do even better with a multiple inheritance with something like:
class Toolkit : public QObject, public vrv::Toolkit and avoid member duplication. You will probably have to make some member of vrv::Toolkit protected (and not private)

- Renaming of functions, classes, namespaces, directories, include-guards.

yeonoson · 2017-09-27T20:16:29Z

I've considered all but the last review comments. I prefer to keep vrvQt::Toolkit and vrv::Toolkit linked by composition instead of inheritance for following reasons:

Mixing multiple domains (Qt-domain with Verovio-domain) by multiple inheritance is considered bad style. In this case it would also be very obvious because it would mix upper-case (Verovio) with lower-case (Qt) function names.
I already removed all duplicated class members in the previous commit by referring to the Getter-functions of vrv::Toolkit.
I'd need to overload most of the Setter-functions of vrv::Toolkit, which thus would have to be marked virtual. At the moment vrv::Toolkit does not (apart from destructor) have any virtual functions.

Regarding the squares when using Leipzig or Gootville font: The fonts are loaded from the fontDirPath, specified in main.qml:53. Maybe it works if you specify an absolute path. The filenames are hard-coded in toolkit.cpp and should be Bravura-1.204.otf, Leipzig-5.2.ttf and Gootville-1.2.otf. In this branch these files exist.

lpugin · 2017-09-28T17:54:49Z

Thanks for the changes. It seems that the problem with the font is not related to the path but to the fact that it has to be installed on the computer. Leipzig works if I install it on my computer (os x 10.12). Could you double check it

yeonoson · 2017-09-28T18:58:28Z

On my system I don't have any of the four used fonts installed. That's why I use QFontDatabase::addApplicationFont to add the fonts to the running application.

Did you try to use an absolute path in main.qml for fontDirPath and it still didn't help?

With my latest commit I added some warning outputs when adding a font. It will either output that the file cannot be found or a generic "Could not add font", which means the file exists, but there was some other problem.

…nts.

yeonoson · 2017-09-30T08:38:03Z

I've now removed the fontDirPath property, instead the font paths to the music font and verovio text font have to be set directly. I included the fonts in the resources of the verovio-qt-demo. This should improve compatibility on different platforms.

yeonoson added 4 commits September 17, 2017 17:42

Changes to the existing files required for the Qt toolkit.

ae38082

Add verovio-qt library.

240e047

Add font files required by verovio-qt:

03417e4

- Source of Bravura-1.204.otf: https://www.smufl.org/files/bravura-1.204.zip - Source of Leipzig-5.2.ttf: Converted from Leipzig-5.2.sfd with https://everythingfonts.com/sfd-to-ttf License: Both fonts are licensed under the SIL Open Font License.

Add a small Qt demo which showcases the usage of verovio-qt.

2f6f87e

lpugin reviewed Sep 21, 2017

View reviewed changes

Better handling for multisample antialiasing.

3620cc7

Change point size to pixel size because conversion does not work on a…

6b9497d

…ll OS.

yeonoson added 2 commits September 22, 2017 17:19

Merge branch 'develop' into develop-qt

b229d74

Move files to typical library structure. Add properties for font/reso…

746c9f2

…urce path and file content.

lpugin requested changes Sep 24, 2017

View reviewed changes

Consider feedback from lpugin:

3d1165f

- Strip down verovio-qt-demo - Rename classes - Remove configuration class members from vrv_qt::Toolkit - Improve Verovio's CMakeLists.txt

lpugin approved these changes Sep 25, 2017

View reviewed changes

lpugin requested changes Sep 25, 2017

View reviewed changes

lpugin reviewed Sep 25, 2017

View reviewed changes

Consider feedback from lpugin:

8d17876

- Renaming of functions, classes, namespaces, directories, include-guards.

Output warning if font cannot be added.

bcf9618

yeonoson added 2 commits September 29, 2017 10:21

Rename "verovio document" to "toolkit" at all places.

3d629a0

1) Add setters for setting font paths. 2) Use Qt resource to store fo…

b315871

…nts.

lpugin merged commit b315871 into rism-digital:develop Oct 3, 2017

yeonoson deleted the develop-qt branch October 4, 2017 18:18

yeonoson mentioned this pull request Jun 10, 2020

Update vrvQt #1494

Merged

		#define __QTSCENEGRAPHDEVICECONTEXT_H__

		//----------------------------------------------------------------------------

Add Qt toolkit #693

Add Qt toolkit #693

Conversation

yeonoson commented Sep 17, 2017

lpugin commented Sep 21, 2017

yeonoson commented Sep 21, 2017

lpugin commented Sep 21, 2017

Choose a reason for hiding this comment

yeonoson Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

yeonoson commented Sep 21, 2017 • edited Loading

lpugin commented Sep 21, 2017

yeonoson commented Sep 21, 2017 • edited Loading

yeonoson commented Sep 22, 2017

lpugin commented Sep 22, 2017 • edited by craigsapp Loading

yeonoson commented Sep 22, 2017

craigsapp commented Sep 23, 2017

Choose a reason for hiding this comment

yeonoson Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpugin commented Sep 24, 2017

yeonoson commented Sep 24, 2017

yeonoson commented Sep 24, 2017

lpugin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpugin Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

lpugin Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeonoson commented Sep 27, 2017

lpugin commented Sep 28, 2017

yeonoson commented Sep 28, 2017

yeonoson commented Sep 30, 2017

yeonoson Sep 22, 2017 •

edited

Loading

yeonoson commented Sep 21, 2017 •

edited

Loading

yeonoson commented Sep 21, 2017 •

edited

Loading

lpugin commented Sep 22, 2017 •

edited by craigsapp

Loading

yeonoson Sep 24, 2017 •

edited

Loading

lpugin Sep 25, 2017 •

edited

Loading

lpugin Sep 25, 2017 •

edited

Loading