1518 articulations #1946

jimka2001 · 2015-04-13T17:27:47Z

Implementation of several ornaments/articulations including Trill, Turn, ReverseTurn
refactored Mordent, and Prall to use the new function renderArticulation()

Jojo-Schmitz · 2015-04-13T17:33:52Z

Is this intended to replace #1939 or accompany it?

lasconic · 2015-04-13T17:36:26Z

Replace it. I closed the other one.

MarcSabatella · 2015-04-13T18:28:16Z

libmscore/rendermidi.cpp

+            qreal chordTimeMS = (chord->actualTicks() / ticksPerSecond) * 1000;
+            ontime = qMin(500, static_cast<int>((graceTimeMS / chordTimeMS) * 1000));
+        }
+        else if (chord->dots() == 1) {


It's been suggested we tweak this to use 333 if not in compound meter (eg, check time signature, it's compound if denominator >= 8 and numerator % 3 = 0. I have no real insight into that, but thought it worth mentioning here.

Hi Marc, is there is issue in the issue tracking dbase related to changing this to use 333?
Maybe there is some justification there I can investigate.

It's not an issue but a forum post - https://musescore.org/en/node/52716

Justification is clear enough to me: the commonly stated advice of 2/3 length for appoggiaturas on dotted notes probably presupposes you are in compound meter. If not, then maybe that rule shouldn't apply. On the other hand, maybe it should. Other than some YouTube recording of one particular piece that do indeed support the idea, I don't see anything solid to go on here.

jimka2001 · 2015-04-14T10:22:31Z

I've appended to this branch.
It now limits the fastest note of the trill (or any such articulation) to the amount of time corresponding to 1/32nd note at 4/4 time with quarter note = 120bpm.
I tested this withhttps://musescore.org/sites/musescore.org/files/ABRSM%20piano%202015%208A6%20Scarlatti_sonata%20in%20B%20minor%20(K497)_0.mscz
and it sounds nice to my ear, whereas it sounded really bad previously.

Jojo-Schmitz · 2015-04-16T07:48:21Z

Seems to fail mtests in tst_midi

jimka2001 · 2015-04-16T08:21:00Z

Hi Joachim, I don’t want to try to refactor any tests until we agree on the functionality which is necessary.
Furthermore, I suspect that several tests will need to either be made obsolete, or updates as the midi out will change. In fact that’s the
point, we want to change the way ornament articulations render for midi, right?

That being the case, I do understand that we(I) have to make the tests pass in order to add this enhancement to to the released version.

Jim

On 16 Apr 2015, at 09:48, Joachim Schmitz notifications@github.com wrote:

Seems to fail mtests in tst_midi

—
Reply to this email directly or view it on GitHub #1946 (comment).

jimka2001 · 2015-04-18T13:54:21Z

The mtest target on xcode seems to do something very strange. I get 25/40 failing tests, even on the upstream master. I need to figure out how to make this work before I can really proceed with updating the test cases for the code I'm intending to change.

Is anyone running the tests with xcode on the mac?

MarcSabatella · 2015-04-18T14:54:34Z

The problem seems to be that the mtests cannot find the style & template files, so there are errors relating to chord symbol and instrument definitions. I know on Windows I once had to manually copy these files to a particular location, but I don't seem to be doing that any more (on Linux or Windows) and I don't remember for sure how this worked.

lasconic · 2015-05-05T09:41:33Z

libmscore/property.h

@@ -240,6 +242,7 @@ enum class P_TYPE : char {
      COLOR,
      DIRECTION,      // enum class MScore::Direction
      DIRECTION_H,    // enum class MScore::DirectionH
+      ORNAMENT_STYLE, // enum class MScore::OrnamentStyle


Why twice ORNAMENT_STYLE ?

What do you mean? Once is P_TYPE::ORNAMENT_STYLE and the other is P_ID::ORNAMENT_STYLE

Is that what you mean?

On 05 May 2015, at 11:41, Nicolas Froment notifications@github.com wrote:

In libmscore/property.h #1946 (comment):

@@ -240,6 +242,7 @@ enum class P_TYPE : char {
COLOR,
DIRECTION, // enum class MScore::Direction
DIRECTION_H, // enum class MScore::DirectionH

ORNAMENT_STYLE, // enum class MScore::OrnamentStyle
Why twice ORNAMENT_STYLE ?

—
Reply to this email directly or view it on GitHub https://github.com/musescore/MuseScore/pull/1946/files#r29654920.

Ok. I just overlooked it.

jimka2001 · 2015-05-06T10:20:05Z

Looks like my recent commit causes a guitar test to fail. I'll look into it.

lasconic · 2015-05-06T10:46:20Z

It's not your commit. The test on volta.gp5.mscx is also failing on master.

jimka2001 · 2015-05-06T11:06:37Z

OK, is the 15180-articulations branch tested as merged into master? or is
it tested as I committed it?
Maybe I rebased onto master without remembering I did?

On Wed, May 6, 2015 at 12:46 PM, Nicolas Froment notifications@github.com
wrote:

It's not your commit. The test on volta.gp5.mscx is also failing on master.

—
Reply to this email directly or view it on GitHub
#1946 (comment).

Jojo-Schmitz · 2015-05-06T11:25:43Z

it gets tested against latest master when you add a new commit, whether rebased or not

lasconic · 2015-05-11T15:39:20Z

libmscore/property.cpp

@@ -169,6 +169,10 @@ static const PropertyData propertyList[] = {
      { P_ID::LASSO_SIZE,          false, 0,               P_TYPE::SIZE_MM  },

      { P_ID::TIME_STRETCH,        false, 0,               P_TYPE::REAL     },
+      { P_ID::ORNAMENT_STYLE,      false, "ornamentStyle", P_TYPE::ORNAMENT_STYLE},


As stated in the comment at the start of the array, the first element here needs to be in the exact same order than the enum class P_ID in property.h.

Isn’t there a way to enforce that? Or at least check it at runtime?

On 11 May 2015, at 17:39, Nicolas Froment notifications@github.com wrote:

In libmscore/property.cpp #1946 (comment):

@@ -169,6 +169,10 @@ static const PropertyData propertyList[] = {
{ P_ID::LASSO_SIZE, false, 0, P_TYPE::SIZE_MM },

{ P_ID::TIME_STRETCH, false, 0, P_TYPE::REAL },

{ P_ID::ORNAMENT_STYLE, false, "ornamentStyle", P_TYPE::ORNAMENT_STYLE},
As stated in the comment at the start of the array, the first element here needs to be in the exact same order than the enum class P_ID in property.h.

—
Reply to this email directly or view it on GitHub https://github.com/musescore/MuseScore/pull/1946/files#r30050097.

ahhh
that’s what this terse comment means?

//
// always: propertyList[subtype].id == subtype
//
//

On 11 May 2015, at 17:39, Nicolas Froment notifications@github.com wrote:

In libmscore/property.cpp #1946 (comment):

@@ -169,6 +169,10 @@ static const PropertyData propertyList[] = {
{ P_ID::LASSO_SIZE, false, 0, P_TYPE::SIZE_MM },

{ P_ID::TIME_STRETCH, false, 0, P_TYPE::REAL },

{ P_ID::ORNAMENT_STYLE, false, "ornamentStyle", P_TYPE::ORNAMENT_STYLE},
As stated in the comment at the start of the array, the first element here needs to be in the exact same order than the enum class P_ID in property.h.

—
Reply to this email directly or view it on GitHub https://github.com/musescore/MuseScore/pull/1946/files#r30050097.

hoping to solve an unreproducable casting problem reported by shoogle. updated midi test as per current state of mordent implementation

…e and Performance A Handbook, by Robert Donington,(c) 1982 implemented tied ornament notes added some documentation for the OrnamentExcursion data structure and removed some old debug comments refactored articulationExcursion into local function

refactored some code into local function makeEvent trying to simply to debug non-homogeneous trills updated test case for mordent updated xml read and write to use strings rather than numbers for ornamentStyle white space comments

…bles

added play button for glissando also updated test case updated glissando accidentals to work correctly based on end note, even if in different staff

and added test cases for this situation

lasconic · 2015-05-18T10:36:39Z

libmscore/rendermidi.cpp

@@ -731,6 +735,478 @@ bool Score::isSubdivided(ChordRest* chord, int swingUnit)
            return false;
      }

+void renderTremolo(Chord *chord, QList<NoteEventList> & ell){


Indentation of these new functions is completely off. There is no comment before the function. Idem for renderArpeggio()

Hi Nicolas, yes these functions don’t have comments.

The functions are their because I took one huge function that spanned many screen-fulls, and divided them into
individual functions the names of which describe what the code does.
The code is more or less the same as before. The code had no comments before I touched it, and still does not.

I can try to invent a comment for the functions, but in my opinion, if the deserve a comment now, then they
already deserved a comment before I touched the code.

What do you think?

On 18 May 2015, at 12:36, Nicolas Froment notifications@github.com wrote:

In libmscore/rendermidi.cpp #1946 (comment):

@@ -731,6 +735,478 @@ bool Score::isSubdivided(ChordRest* chord, int swingUnit)
return false;
}

+void renderTremolo(Chord *chord, QList & ell){
Indentation of these new functions is completely off. There is no comment before the function. Idem for renderArpeggio()

—
Reply to this email directly or view it on GitHub https://github.com/musescore/MuseScore/pull/1946/files#r30496906.

Hi Nicolas, as far as the indentation. I can fix it, in as much as I understand the indentation rules.
However, I tried to preserve the indentation which was already their. I basically just move some code in an overbloated function
into several smaller functions, making the original function much easier to read.

On 18 May 2015, at 12:36, Nicolas Froment notifications@github.com wrote:

In libmscore/rendermidi.cpp #1946 (comment):

@@ -731,6 +735,478 @@ bool Score::isSubdivided(ChordRest* chord, int swingUnit)
return false;
}

+void renderTremolo(Chord *chord, QList & ell){
Indentation of these new functions is completely off. There is no comment before the function. Idem for renderArpeggio()

—
Reply to this email directly or view it on GitHub https://github.com/musescore/MuseScore/pull/1946/files#r30496906.

the factorization into functions is great. Thank you for making it. As you will see all around the code, every function has an header

//--------------------------------------------------------- // functionName // optional explanation on multiple line if needed //---------------------------------------------------------

Regarding the indentation, we use 6 spaces to indent. (the code is 4 spaces indented in your PR) The { after a functionName are on a new line. We use if (nospace) { and idem for while, case etc... I started to reindent it so don't bother for this one.

See https://musescore.org/en/developers-handbook/musescore-coding-rules#Indentation-%26-Whitespaces

�Apparently here are the controls xcode allows me.

I’ve changed the tab width to 6.
Any idea what the others mean and how I should set them?

//---------------------------------------------------------
// functionName
// optional explanation on multiple line if needed
//---------------------------------------------------------
Regarding the indentation, we use 6 spaces to indent. (the code is 4 spaces indented in your PR) The { after a functionName are on a new line. We use if (nospace) { and idem for while, case etc... I started to reindent it so don't bother for this one.

For QtCreator we have something that does it automagically, but not for xcode I think

something that works from the command line would be great as well.

On 18 May 2015, at 14:33, Joachim Schmitz notifications@github.com wrote:

In libmscore/rendermidi.cpp #1946 (comment):

@@ -731,6 +735,478 @@ bool Score::isSubdivided(ChordRest* chord, int swingUnit)
return false;
}

+void renderTremolo(Chord *chord, QList & ell){
For QtCreator we have something that does it automagically, but not for xcode I think

—
Reply to this email directly or view it on GitHub https://github.com/musescore/MuseScore/pull/1946/files#r30502743.

if you commit your indentation changes, I can merge that into my own to avoid us having conflicting indentation changes.

On 18 May 2015, at 14:20, Nicolas Froment notifications@github.com wrote:

//---------------------------------------------------------
We use if (nospace) { and idem for while, case etc... I started to reindent it so don't bother for this one.

—

indent? Need a couple options...

1518 articulations

lasconic · 2015-05-18T14:49:18Z

I just merged it but I discovered you didn't sign the MuseScore CLA yet. Could you http://musescore.org/en/cla ? Thanks !

jimka2001 · 2015-05-18T14:53:00Z

OK

On 18 May 2015, at 16:49, Nicolas Froment notifications@github.com wrote:

I just merged it but I discover you didn't sign the MuseScore CLA yet. Could you http://musescore.org/en/cla http://musescore.org/en/cla ? Thanks !

—
Reply to this email directly or view it on GitHub #1946 (comment).

lasconic · 2015-05-18T14:59:42Z

Great! Good to go.
You will see that my next commit 1dc19c4 is fixing several "problems".

The indentation is now correct, using 6 spaces, if/while/switch etc have the right number of spaces. I tried to removed {} when the if/else statement has a single line but added them with the if/else statement is a single expression on single line (like a long for() loop).
I removed as many local functions as possible. As I said, we don't use local functions anywhere else (or it's exceptional), we don't use function as parameter of other function either. I agree it's a matter of taste... I'm just enforcing the guidelines from Werner.

never use function pointers when not absolutely necessary; i experimented a lot with them and they produce big slow code. In addition they are hard to debug, code is hard to read...

jimka2001 · 2015-05-18T15:35:14Z

thanks. needless to say, i don’t think they make code harder to read. on the contrary i think they can make the code easier.

Are you sure the idea of function pointers applies to local functions? Local functions are not pointers any more than global functions are pointers, right? I mean they are all readlly pointers when you get down to the low level.

Perhaps I need to take up the discussion with Werner?

Perhaps he is right about the slow down.

I also noticed you change the spelling of the xml values. now none of my files read correctly :-(.
I need to figure out how to convert the .mscz to .mscx so I can edit them with a text editor.
I think there was an email exchange about this topic.

On 18 May 2015, at 16:59, Nicolas Froment notifications@github.com wrote:

Great! Good to go.
You will see that my next commit 1dc19c4 1dc19c4 is fixing several "problems".

The indentation is now correct, using 6 spaces, if/while/switch etc have the right number of spaces. I tried to removed {} when the if/else statement has a single line but added them with the if/else statement is a single expression on single line (like a long for() loop).
I removed as many local functions as possible. As I said, we don't use local functions anywhere else (or it's exceptional), we don't use function as parameter of other function either. I agree it's a matter of taste... I'm just enforcing the guidelines from Werner.

never use function pointers when not absolutely necessary; i experimented a lot with them and they produce big slow code. In addition they are hard to debug, code is hard to read...
—
Reply to this email directly or view it on GitHub #1946 (comment).

lasconic · 2015-05-18T15:37:37Z

MSCZ are just zip files. On the command line you can run unzip file.mscz
Then you should be able to do the substitution with your tool of choice, for example sed?

jimka2001 · 2015-05-18T15:39:46Z

I see you removed my mod7 function and replaced it by adding 700 before moding.
Isn’t there a function which will take the positive remainer after division.
In list there are two such functions mod always returns a positive number
and rem seems to do what the % operator in C++ does.

https://www.cs.cmu.edu/Groups/AI/html/hyperspec/HyperSpec/Body/fun_modcm_rem.html https://www.cs.cmu.edu/Groups/AI/html/hyperspec/HyperSpec/Body/fun_modcm_rem.html

(rem -1 5) => -1
(mod -1 5) => 4
(mod 13 4) => 1
(rem 13 4) => 1
(mod -13 4) => 3
(rem -13 4) => -1
(mod 13 -4) => -3
(rem 13 -4) => 1
(mod -13 -4) => -1
(rem -13 -4) => -1
(mod 13.4 1) => 0.4
(rem 13.4 1) => 0.4
(mod -13.4 1) => 0.6
(rem -13.4 1) => -0.4

lasconic · 2015-05-18T15:41:53Z

I just know that it's the way we do it anywhere else in MuseScore :) and that was one less local function.
I don't know of a better way in C++

jimka2001 · 2015-05-18T16:08:52Z

there are two other files zipped together in the mscz,

Can I simply ignore the container.xml and the .png?

M Filemode Length Date Time File

-rw------- 155 18-May-2015 18:04:24 META-INF/container.xml
-rw------- 34275 18-May-2015 18:04:24 Thumbnails/thumbnail.png
-rw------- 229235 18-May-2015 18:04:44 andante.mscx

            263665                         3 files

On 18 May 2015, at 17:37, Nicolas Froment notifications@github.com wrote:

MSCZ are just zip files. On the command line you can run unzip file.mscz
Then you should be able to do the substitution with your tool of choice, for example sed?

—
Reply to this email directly or view it on GitHub #1946 (comment).

lasconic · 2015-05-18T16:09:46Z

Yes

jimka2001 · 2015-05-19T10:16:31Z

Hi Nicolas, there was one think missing from your refactoring.
I’ve push a single commit patch. the commit is 6db9e28
it is on a branch named remove-useless-variable.
This branch should be rebase-able onto master.

The issue is that the code WAS passing around a useless variable, passing by value, and performing flow control based on its value.
The variable actually was useless in that its value was changing in a predictable way, not based on any parameter values.
I eliminated the variable. This effects five or six lines of code, and shortens a function argument list.

On 18 May 2015, at 16:59, Nicolas Froment notifications@github.com wrote:

Great! Good to go.
You will see that my next commit 1dc19c4 1dc19c4 is fixing several "problems".

The indentation is now correct, using 6 spaces, if/while/switch etc have the right number of spaces. I tried to removed {} when the if/else statement has a single line but added them with the if/else statement is a single expression on single line (like a long for() loop).
I removed as many local functions as possible. As I said, we don't use local functions anywhere else (or it's exceptional), we don't use function as parameter of other function either. I agree it's a matter of taste... I'm just enforcing the guidelines from Werner.

never use function pointers when not absolutely necessary; i experimented a lot with them and they produce big slow code. In addition they are hard to debug, code is hard to read...
—
Reply to this email directly or view it on GitHub #1946 (comment).

lasconic · 2015-05-19T10:21:27Z

Can you make a PR with this commit ?

jimka2001 · 2015-05-19T10:23:03Z

are you on IRC? I needed to discuss that with you.

On 19 May 2015, at 12:21, Nicolas Froment notifications@github.com wrote:

Can you make a PR with this commit ?

—
Reply to this email directly or view it on GitHub #1946 (comment).

lasconic mentioned this pull request Apr 13, 2015

1518 trill articulation #1939

Closed

MarcSabatella reviewed Apr 13, 2015
View reviewed changes

jimka2001 force-pushed the 1518-articulations branch 2 times, most recently from 57382c0 to 6ad95b4 Compare April 16, 2015 17:16

jimka2001 force-pushed the 1518-articulations branch 3 times, most recently from 4286350 to 3f5aa10 Compare April 24, 2015 11:48

jimka2001 force-pushed the 1518-articulations branch 7 times, most recently from 3afa135 to ca426b4 Compare May 4, 2015 16:04

lasconic reviewed May 5, 2015
View reviewed changes

jimka2001 force-pushed the 1518-articulations branch 3 times, most recently from 3cf7fa2 to 783c93f Compare May 10, 2015 16:11

lasconic reviewed May 11, 2015
View reviewed changes

jimka2001 added 11 commits May 16, 2015 10:46

added properties playArticulation and ornamentStyle

dde089e

impelemented playback of ornament articulations

4aa2e0d

updating to use writeProperty rather than direct call to xml.tag,

93c402b

hoping to solve an unreproducable casting problem reported by shoogle. updated midi test as per current state of mordent implementation

Updated articulations to work with accidentalscleaned up unused varia…

248b2af

…bles

added test of baroque ornaments

e108c17

implementation of glissando playback

782e592

added play button for glissando also updated test case updated glissando accidentals to work correctly based on end note, even if in different staff

updated glissando playback to understand accidentals in current chord.

f4c0685

fixed problem with ornaments/glissando with transposing instruments

ba72ff1

and added test cases for this situation

some changes for consistency, as requested by lasconic

98fb35b

jimka2001 force-pushed the 1518-articulations branch from 0f2cc11 to 98fb35b Compare May 16, 2015 08:57

lasconic reviewed May 18, 2015
View reviewed changes

lasconic added a commit that referenced this pull request May 18, 2015

Merge pull request #1946 from jimka2001/1518-articulations

ec9a4b8

1518 articulations

lasconic merged commit ec9a4b8 into musescore:master May 18, 2015

lasconic added a commit that referenced this pull request May 18, 2015

review indentation and code style for PR #1946

1dc19c4

1518 articulations #1946

1518 articulations #1946

Conversation

jimka2001 commented Apr 13, 2015

Jojo-Schmitz commented Apr 13, 2015

lasconic commented Apr 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimka2001 commented Apr 14, 2015

Jojo-Schmitz commented Apr 16, 2015

jimka2001 commented Apr 16, 2015

jimka2001 commented Apr 18, 2015

MarcSabatella commented Apr 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimka2001 commented May 6, 2015

lasconic commented May 6, 2015

jimka2001 commented May 6, 2015

Jojo-Schmitz commented May 6, 2015

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

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

lasconic commented May 18, 2015

jimka2001 commented May 18, 2015

lasconic commented May 18, 2015

jimka2001 commented May 18, 2015

lasconic commented May 18, 2015

jimka2001 commented May 18, 2015

lasconic commented May 18, 2015

jimka2001 commented May 18, 2015

lasconic commented May 18, 2015

jimka2001 commented May 19, 2015

lasconic commented May 19, 2015

jimka2001 commented May 19, 2015