Fix for #547 : Initial proposal for ArpeggioMark and ArpeggioMarkSpanner classes. #1337
Conversation
|
MusicXML parser/writer are next (of course). |
|
OK, support for ArpeggioMark (including m21ToXml.py and xmlToM21.py) is there. I have made no attempt to support ArpeggioMarkSpanner for cross-staff arpeggios, since I have no MusicXML examples to parse. Can I get this reviewed, please? Maybe we can merge it as is (minus the declaration of ArpeggioMarkSpanner) and write a new issue for ArpeggioMarkSpanner? |
|
OK, I think I've addressed all of @jacobtylerwalls' review. |
|
I'm still working on MusicXML parser/writer support for ArpeggioMarkSpanner (multi-chord/note arpeggios). I have the writer working (I'll push it for your perusal shortly), but I can't test it very well without the parser (well, I can, since I have a version of my Humdrum parser that can parse multi-chord arpeggios, but no-one else can, at least not very easily). On to the parser! |
|
Here is the parser support for multi-chord arpeggios. |
|
Hmmm. Time to write more tests. |
|
OK, this should be ready for final review. |
|
Once a week ping :-) |
|
Wow, flake8 got a lot pickier throughout music21. Hope that doesn't block me from getting this in. |
|
ArpeggioMark is totally approved. That can be merged. Thank you! I think from the discussion in #1142 (HammerOn, HammerOff) -- I'm still not willing to go with spanners being inside music21 objects because of the way that they're gathered -- what happens if we only show Staff 2 but the spanner is the .expressions on an object in Staff 1. If the spanner is in Staff 1, music21 is smart enough to do the right thing (or if it's not, there's one place we need to fix it). I'm also still scared by the circular reference -- note-holds-spanner-holds-note-holds-spanner...etc. And it's even harder for cases like chords (what if a chord has notes 0,1,2 in staff 1 and notes 3,4 in staff 2, but only notes 2-3 are arpeggiated?) Then we have chord holds spanner which holds notes which aren't in the Stream. I think that the general solution to the problem raised in #1142 needs to be solved before going with ArpeggioMarks inside of expressions. Maybe there is something like a spanner that references an ordered list of other m21 objects that could be made, since the entire infrastructure of spanner doesn't seem to be needed here. |
|
ArpeggioMarkSpanner is never put in obj.expressions, and shouldn't ever contain notes that are within chords. I have pushed another update to make the ArpeggioMarkSpanner docs more clear on this subject. |
Ah... I totally misread this. Let me check further. |
There was a problem hiding this comment.
Got it! This is great! A few small changes/refactors for readability, etc. then let's merge this today. (@jacobtylerwalls feel free also to merge if you see that the changes are made and I don't check in later today).
1. Sort chord in place at top of chordToXML, and get rid of all that index calculation. 2. Factor out a new method: arpeggioMarkToMxExpression.
|
I believe this is ready to go (I hope!) |
music21/musicxml/m21ToXml.py
Outdated
| mxArpeggio = Element('non-arpeggiate') | ||
| mxArpeggio.set('type', 'bottom') | ||
| elif topNoteIndex == noteIndexInChord: | ||
| elif noteIndexInChord == len(obj.notes): |
There was a problem hiding this comment.
this will never happen, off by one error. len(obj.notes) - 1 can we have a test to make sure these are generated correctly.
There was a problem hiding this comment.
Yeah, my multiStaffArpeggio test doesn't have any non-arpeggiates in it. I'll fix that (and then this bug).
|
Unfortunately, after adding some tests, the ArpeggioMarkSpanner for non-arpeggiate is not working, because the marks don't go on the lowest and highest notes of a single chord, but the lowest and highest notes of the entire ArpeggioMarkSpanner, so we'll need to extract that information instead. |
|
It's working now. Just need to lint |
Fixes #547