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

Glissando as a multi-segment line. #1536

Merged
merged 11 commits into from Feb 18, 2015
Merged

Glissando as a multi-segment line. #1536

merged 11 commits into from Feb 18, 2015

Conversation

mgavioli
Copy link
Contributor

  • Implemented as a sub-class of SLine.
  • Anchor type changed from CHORD to NOTE: allows to attach glissando start and end points to individual notes, rather than generically to chords (with note within the chord chosen by the program).
  • The Glissando element is now stored in the Note::_spannerFor list.
  • Chord::_glissando has been removed and replaced by a bool _endsGlissando, recording whether the chord is at the end of glissando (as gliss.-end chords require more space if mid-measure or system-initial).
  • Debugger UI for Chord updated accordingly.
  • Glissando is now saved into score file as a spanner, within the initial note, and with appropriate <endSpanner> tag in the Glissando ending note.
  • Existing scores with the old Glissando file format are correctly read back.

Notes:

  • This version can read scores from older versions, but older versions cannot read scores from this version (they do not expect a <Glissando> tag within a Note). This probably requires a new file version number; as the file version number increased fairly recently, it might make sense to piggy-back this change, if merged soon.
  • This implementation would rather easily allow to move the start and end anchors around (as for slurs) to override the note/chord chosen by the program when the glissando is initially created; but the UI for this is not implemented yet.

The latest commits should make the PR fully functional.

@MarcSabatella
Copy link
Contributor

Wow, anchor to notes too? That's going to make some people very happy!

I don't particularly care about the ability to change anchors, but if this new implementation - being Sline-based - also comes with the ability to adjust line length, that's going to make even more people happy!

@Jojo-Schmitz
Copy link
Contributor

Yes, I think it needs a new file version, and if only to make Beta 1 users aware that it won't be able to read files (using glissandi) created with this version.
I see some traces of melisma, is it implemented too already?
I guess the failing mtests need to get adjusted?

@MarcSabatella
Copy link
Contributor

If it helps you decide, I also considered raising version number when I added the slash notation support. Scores created using the new slash facility won't open (and will likely crash) older versions. I mentioned something about it at the time but never followed through. Between that and this, and perhaps also changes made recently for custom key signatures, I'm thinking we should.

@mgavioli
Copy link
Contributor Author

@Jojo-Schmitz and @MarcSabatella : about the version number, I agree but I think I'll leave to @lasconic e/o @wschweer to decide.

About anchors: I believe that the ability to change the anchor points is not marginal; when the ending point is a multi-note chord, the algorithm defaults to the chord top note (as the old algorithm did; also for backward compatibility). This might or might not be what the user intended.

Also, the glissando is dropped to the starting note and defaults to end at the (top note of the) next chord of that part; I can think of cases where this is not necessary the 'right thing' (for instance, in keyboard parts, in orchestral divisi or in general in multi-part staves): the intended end of the glissando might be several chords beyond or in the 'other staff'. I believe the implementation to be easy enough to be worth pursuing.

But I also wanted to make the PR available ASAP, also to profit from for discussions like this.

About dragging grips: the ability to drag ending grips of individual segments comes 'for free' with SLine: entering edit mode on the new glissando should already display and manage draggable grips; I did not test it, though, and I not sure those user editings be correctly saved. This point too should be rather easy to implement / correct, though.

About melisma: no, it is not implemented, I'm working on it now. Simply, as I was adding new types to enum's or switch's, I though it convenient to already add the corresponding melisma item.

About the failing test: I'll have a look; perhaps the test needs updating, but it is well possible I did not take all the side effects into account (this selection stuff is new to me) and then my code is to be corrected.

@chenlung
Copy link
Contributor

http://musescore.org/en/node/19155
http://musescore.org/en/node/23100

I don't understand the mentions of melisma - could someone explain?

@Jojo-Schmitz
Copy link
Contributor

@chenlung: @mgavioli plans to extend this fix to also allow cross system melismas.

@MarcSabatella
Copy link
Contributor

Btw, would also be worth taking another look at http://musescore.org/en/node/21725. I'd guess this change invalidates the existing PR for that. Maybe it ends up being fixed automatically, maybe it requires a different fix.

@mgavioli
Copy link
Contributor Author

Trying to fix the failing tests; one was just a matter of re-aligning the test ref. score, but:

  • MXML in/out was not trivial and required rewriting the relevant MXML functions: @lvinken comments would be welcome!.
  • GuitarPro import remains mostly undecipherable: @jpirie help would be welcome...

@MarcSabatella: as glissando is now note-to-note, it should draw correctly from a grace note (assuming enough space is created and the final note is correctly 'guessed'). I'll have a look, but I would like to have the tests fixed first.

@mgavioli
Copy link
Contributor Author

Added an intermediate fix (for MusicXML and Selection filter) to keep the ball rolling.

Guitar Pro import still needs updating, though: I hope @jpirie can have a look at it or at least explain to me how to go on...

@jpirie
Copy link
Contributor

jpirie commented Dec 21, 2014

Sorry haven't replied earlier, it's been crazy in the run up to Christmas for me! :-)

I have taken a look at this today and I've been able to modify the Guitar Pro import implementation in order to work with the new glissando implementation, but for some reason I only see the glissando on the score, and not when viewing the part. I'm not sure if that's something that I need to solve at my import time or whether something needs to be modified in your change. I'll have a think about that anyway - if you have any additional information on that then do let me know.

As I was testing this out I discovered some kind of bug that causes MuseScore to segfault. I can reproduce with the following steps, can you see if you can reproduce on your end?

-> Open Musescore
-> Create a new score for trombone
-> Place two notes on the first bar
-> Attempt to drag a glissando from the master palette
-> Crash.

Stack trace from GDB given below:

Program received signal SIGSEGV, Segmentation fault.
0x00000000009d79d8 in Ms::Element::parent (this=0x0) at /home/jpirie/repos/musescore-mgavioli/libmscore/element.h:329
329 Element* parent() const { return _parent; }
(gdb) bt
#0 0x00000000009d79d8 in Ms::Element::parent (this=0x0)
at /home/jpirie/repos/musescore-mgavioli/libmscore/element.h:329
#1 0x00000000009d7d0a in Ms::Note::chord (this=0x0) at /home/jpirie/repos/musescore-mgavioli/libmscore/note.h:345
#2 0x0000000001056243 in Ms::Glissando::layout (this=0x5d24c90)
at /home/jpirie/repos/musescore-mgavioli/libmscore/glissando.cpp:181
#3 0x0000000000d3bf8e in Ms::ScoreView::dragEnterEvent (this=0x583b560, event=0x7fffffffb540)
at /home/jpirie/repos/musescore-mgavioli/mscore/dragdrop.cpp:246
#4 0x00007ffff4a4be68 in QWidget::event(QEvent*) () from /usr/lib/libQt5Widgets.so.5
#5 0x00000000009cd4b5 in Ms::ScoreView::event (this=0x583b560, event=0x7fffffffb540)
at /home/jpirie/repos/musescore-mgavioli/mscore/scoreview.cpp:3992

@lvinken
Copy link
Contributor

lvinken commented Dec 21, 2014

Hi Maurizio,

sorry for late reply, been very busy on other subjects. I did a quick check
on your MXML changes, but did not see any obvious issues.

Do the test cases (testArpGliss1..3.xml) still pass ? Is the result of
importing them still accurately reflected by the corresponding PDF files ?
Is test coverage sufficient ?

Regards, Leon.

On Sun, Dec 21, 2014 at 9:38 AM, Maurizio M. Gavioli <
notifications@github.com> wrote:

Added an intermediate fix (for MusicXML and Selection filter) to keep the
ball rolling.

Guitar Pro import still needs updating, though: I hope @jpirie
https://github.com/jpirie can have a look at it or at least explain to
me how to go on...


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

@mgavioli
Copy link
Contributor Author

@jpirie : Hi, John; I know, this is not the most serendipitous moment in the year... Thanks for looking into the matter!

About the gliss. in part issue: as GP import does not really work here, I don't actually know, I should have a look at the 'working' code. Could you send it to me in some way?

In the meantime I'll look at the crash while dragging.


@lvinken : Hi Leon, thanks for looking into the matter. 'Yes' to all your three questions:

Yes, all MXML import/export tests do pass now.

Yes, the imported XML scores match their PDF's.

Yes, I assume test coverage to be sufficient; unless the MXML format allows variations of the glissando stuff which were not supported by MuseScore before and are now; basically, if MXML supports glissando starting from and/or ending into multi-note chords and/or between non-adjacent chords, it might be worth adding more tests for that. I would say this is somewhat lower priority than having the fix working, though.

@jpirie
Copy link
Contributor

jpirie commented Dec 23, 2014

@mgavioli Sure thing. I've committed to the glissando_v2 branch of my fork of Musescore a patch which makes the gliss display (in the main score only) with your new implementation in the shift slide tests (e.g. mtest/guitarpro/shift-slide.gp5). I hope this is useful to you.

I'll have a glance at it over Christmas also and see if I can think of a reason why the slide would only show up in the main score. I think I might have solved a similar problem in the past but I cannot remember how..

@mgavioli
Copy link
Contributor Author

@jpirie Got it and working on it.

@mgavioli
Copy link
Contributor Author

  • Fixed cloning of glissando elements into parts and into linked staves (actually of all spanners attached to Notes)
  • Fixed a bug when reading scores with the old format glissando which also contained parts
  • Updated MXML I/O according to @lvinken suggestions
  • Updated import GTP according to @jpirie suggestions and fixed its tests accordingly.

@lasconic
Copy link
Contributor

Once merged, we need to bump up the file revision in mscore.h

@lasconic
Copy link
Contributor

Can you rebase this PR and maybe bump up the file revision number then?

- Implemented as a sub-class of `SLine`.
- Anchor type changed from CHORD to NOTE: allows to attach glissando start and end points to individual notes, rather than generically to chords (with note within the chord chosen by the program).
- The Glissando element is now stored in the `Note::_spannerFor` list.
- `Chord::_glissando` has been removed and replaced by a `bool _endsGlissando`, recording whether the chord is at the end of glissando (as gliss.-end chords require more space if mid-measure or system-initial).
- Debugger UI for `Chord` updated accordingly.
- Glissando in now save into score file as a spanner, within the initial note, and with appropriate `&lt;endSpanner&gt;` tag in the Glissando ending note.
- Existing scores with the old Glissando file format are correctly read back.

Notes:

- MusicXML import/export of the new Glissando implementation NOT IMPLEMENTED.
- This version can read scores from older versions, but older versions **cannot read** scored from this version (they do not expect a &lt;Glissando&gt; tag within a Note). Does this require a NEW FILE VERSION NUMBER?
- This implementation would allow rather easily to move the start and end anchors around (as for slurs) to override the note/chord chosen by the program when the glissando is initially created; but the UI for this is not implemented yet.
- MXML I/O code has been adapted to the new glissando code.
- Ref. scores for Selection filter tests have been updated.

Guitar Pro: import code needs to be adapted to the new glissando code and I have no idea how to do that.
For both parts (file `excerpt.cpp`, func. `cloneStaves()`) and linked staves (func. `cloneStaff()`).
Also, some code clean-up (mainly deleting old code commented out)
@mgavioli
Copy link
Contributor Author

@lasconic : rebased, aligned with recent changes and increased file version number; Travis is having its moods again, but sooner or later...

@mgavioli
Copy link
Contributor Author

@lasconic : Travis octroya Sa grace!

@lasconic
Copy link
Contributor

Let's merge it. This is hopefully the last big change we merge before MuseScore 2.0 RC.

lasconic added a commit that referenced this pull request Feb 18, 2015
Glissando as a multi-segment line.
@lasconic lasconic merged commit fadb223 into musescore:master Feb 18, 2015
@mgavioli mgavioli deleted the glissando_v2 branch February 21, 2015 21:58
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.

None yet

7 participants