FretBend update #1580

adhooge · 2023-05-23T14:40:56Z

Hi! First PR so I expect a few things to be added/modified but here's a first draft.
I've updated the FretBend articulation so that it includes the musicXML properties bendAlter, preBend and release.

It goes round from musicXML to m21 back to mXML.

I've not written any tests yet because I'm not sure what to add. Let me know of what could be good to add!

as a PR newbie, I also have some irrelevant commits that I manually reverted. If that's a problem I'll try to clean that up.

mscuthbert

Well on its way! Ask around if you need help in writing tests, and for help on why music21's system of intervals is more complex than musicxml's.

(the changes to note.Note are rejected, so please remove them, thanks!)

mscuthbert · 2023-05-24T14:53:47Z

music21/articulations.py

@@ -84,6 +84,7 @@
 from music21.common.classTools import tempAttribute
 from music21 import environment
 from music21 import style
+from music21 import interval


alphabetical order (existing mistake with style before spanner can be fixed in this PR also)

and if this is added it needs to be removed from they TYPE_CHECKING import below (but I don't think it will need to be, see below)

Done with 3cf71e0

mscuthbert · 2023-05-24T15:02:13Z

music21/articulations.py

+    release: t.Optional[float]
+    withBar: t.Optional[bool]
+
+    def __init__(self, number=0, preBend=False, release=None, withBar=None, **keywords):


number is too generic a term -- and we usually reserve the term for identifying which of multiple simultaneous items (like slur number=1 vs slur number=2). Are fret-bends always chromatic intervals? it seems like you can also have a diatonic interval of a fretbend. hence why bendAlter was typed as IntervalBase not ChromaticInterval. It seems better to have bendAlter as described above interval.IntervalBase | None = None where None indicates unspecified. Having a default of 0, becoming ChromaticInterval(0) indicates that this fretBend does not alter the pitch at all, which would be a very strange fret bend. This would also make it so that interval.py does not need to be imported here.

The other keywords should be typed in the __init__ declaration in addition to the class.

We no longer use t.Optional[X] in the code base. Use X | None instead.

Changed with d63c78a

mscuthbert · 2023-05-24T15:04:38Z

music21/articulations.py

+        bend indication for fretted instruments
+
+        bend in musicxml
+
+        number is the interval of the bend in number of semitones, bend-alter in musicxml
+        preBend indicates wether the note is prebended or not
+        release is the offset value for releasing the bend, if Any
+        withBar indicates if the bend is done using a whammy bar movement


Capitalization and use asterisks and line breaks for documentation -- run documentation/build.py and look at the difference between the docs produced here and elsewhere in the system.

preBend indicates wether the note is prebended or not -- I don't know what prebended means.

what is release measured in? Is it an offset (which is relative to the start of the Measure/stream)? That doesn't sound right. Is it a quarterLength measured from the start of the note? or from the end of the note.

withBar doc is fine.

I've tried to improve it in 03e6117
Let me know if that seems good to you

tests are on their way

mscuthbert · 2023-05-24T15:08:53Z

music21/musicxml/m21ToXml.py

+            bendAlterSubElement = SubElement(mxTechnicalMark, 'bend-alter')
+            bendAlterSubElement.text = str(articulationMark.bendAlter.semitones)


this has enough ifs that it should be brought out as its own small method that allows for easier documentation and testing. articulationMark will probably need to be cast as a FretBend.

We can solve the case of GenericInterval later, which doesn't have .semitones. Perhaps it always needs to be a full Interval object or None.

New submethod added in 85cc6a8
Now I indeed have the problem that I need to retrieve the number of semitones. I could probably do something with GenericInterval.value but I don't know how to do the conversion properly

mscuthbert · 2023-05-24T15:10:01Z

music21/musicxml/m21ToXml.py

+                releaseSubElement = SubElement(mxTechnicalMark, 'release')
+                releaseSubElement.set('offset', str(articulationMark.release))


musicxml's definition of offset is very different from music21's and (despite me now being editor of the musicxml spec) we shouldn't use musicxml offsets anywhere in music21 -- these need to be converted according to the current divisionsPerQuarter setting.

should work with d93af6d though I need to write the tests

mscuthbert · 2023-05-24T15:12:41Z

music21/musicxml/xmlToM21.py

@@ -3881,11 +3881,18 @@ def xmlTechnicalToArticulation(self, mxObj):
            if tag in ('heel', 'toe'):
                if mxObj.get('substitution') is not None:
                    tech.substitution = xmlObjects.yesNoToBoolean(mxObj.get('substitution'))
+            if tag == 'bend':
+                tech.bendAlter = interval.Interval(int(mxObj.find('bend-alter').text))


this has too many potentials for crashing the processing. What if there is no bend-alter or no bend-alter.text? check that before typing to assign.

(Better to call out into a separate sub-method. This method was designed just for one-to-two lines per technical

sub-method and bend-alter check in 2e1ea43

mscuthbert · 2023-05-24T15:13:22Z

music21/musicxml/xmlToM21.py

+                if mxObj.find('pre-bend') is not None:
+                    tech.preBend = True
+                if mxObj.find('release') is not None:
+                        tech.release = int(mxObj.find('release').get('offset'))


as above -- offset needs to be converted to music21 quarterLengths.

except clause without Try.

Both fixed with 58eeb04

mscuthbert · 2023-05-24T15:13:33Z

music21/musicxml/xmlToM21.py

+                        tech.release = int(mxObj.find('release').get('offset'))
+                    except TypeError:
+                        # offset is not mandatory
+                        tech.release = 0


release is a float. needs to be 0.0

also fixed with 58eeb04

mscuthbert · 2023-05-24T15:14:38Z

music21/note.py

+from music21 import articulations

 if t.TYPE_CHECKING:
    from music21 import articulations


as above -- alphabetical and remove from type-checking if imported at top. (but won't be necessary).

mscuthbert · 2023-05-24T15:19:49Z

music21/note.py

+    @property
+    def string(self):


this new property and all properties below are rejected. Note is the most common object in music21 and users who are not working with guitar music will never use these routines but will need to figure out what they mean in the docs. (we don't have "accent" or "staccato" etc. either) -- and parallel routines would need to be added to Chord, etc.

In general for any mature open-source project, don't make additions or substantive changes to core parts of the system without a prior discussion.

oops sorry about that, they were just for my personal convenience. Removed with 0b2074f

mscuthbert · 2023-07-01T08:59:43Z

Hello -- just as a note (from the music21list Google Group) I'm taking a sabbatical from reviewing music21 issues and PRs until at least Jan 1, 2024, so this PR will be deferred until then.

adhooge · 2023-07-10T06:54:40Z

Hello Michael, sorry for not being able to finish this PR sooner. Enjoy your sabbatical! Hopefully my PR will be ready for approval when you come back 🤞

mscuthbert

Hi! It's again a Request Changes, but they're really minimal changes and I think this is very close to getting in. :-) CONGRATS on the amazing work so far and for putting up with a sabbatical that interrupted it!

mscuthbert · 2024-08-13T20:59:49Z

music21/articulations.py

+    >>> fb.release 
+    0.5
+    '''
+    bendAlter: interval.IntervalBase | None


since we need to have a number of semitones, maybe make this interval.Interval | interval.ChromaticInterval | None so that if it's not None it definitely has a number of semitones.

I'm not a guitar player so forgive me, but is there a concept of an indefinitely sized bend upwards or downwards? where the direction is known but not the size? (this could be something to put in later)

mscuthbert · 2024-08-13T21:02:34Z

music21/articulations.py

+
+    WithBar indicates if the bend is done using a whammy bar movement. Defaults to False.
+
+    >>> fb = articulations.FretBend(bendAlter=interval.ChromaticInterval(-2), release=0.5)


add number to this to show that it appears in the fb representation below.

mscuthbert · 2024-08-13T21:05:07Z

music21/articulations.py

+    release: float | None
+    withBar: bool 
+
+    def __init__(self, number=0, bendAlter=None, preBend=False, release=None, withBar=False, **keywords):


type needs to be repeated here. Also make all but number keyword only so people don't have the remember the order.

def __init__( self, number: int = 0, *, bendAlter: interval.Interval | interval.ChromaticInterval | None = None, preBend: bool = False, release: OffsetQL | None = None, withBar: bool = False, **keywords ):```

mscuthbert · 2024-08-13T21:05:24Z

music21/articulations.py

+    withBar: bool 
+
+    def __init__(self, number=0, bendAlter=None, preBend=False, release=None, withBar=False, **keywords):
+        super().__init__(**keywords)


bug. needs to be super().__init__(number=number, **keywords)

mscuthbert · 2024-08-13T21:06:21Z

music21/musicxml/m21ToXml.py

+            # musicxml expects a number of semitones but not sure how to get it
+            # from a GeneralInterval
+            pass


now with the change in typing above this can be possible.

mscuthbert · 2024-08-13T21:07:10Z

music21/musicxml/xmlObjects.py

                               # hammer-on and pull-off not implemented because handled
                               # in method objectAttachedSpannersToTechnicals of m21ToXml.py
                               # ('hammer-on', articulations.HammerOn),
                               # ('pull-off', articulations.PullOff),
-                               # bend not implemented because it needs many subcomponents
-                               # ('bend', articulations.FretBend),
+                               ('bend', articulations.FretBend),


this line appears to be added twice? (see line 70)

mscuthbert · 2024-08-13T21:08:28Z

music21/musicxml/xmlToM21.py

            self.setPlacement(mxObj, tech)
            return tech
        else:
            environLocal.printDebug(f'Cannot translate {tag} in {mxObj}.')
            return None

+    @staticmethod


Not going to be able to be a staticmethod -- see below.

mscuthbert · 2024-08-13T21:12:27Z

music21/articulations.py

+    '''
+    bendAlter: interval.IntervalBase | None
+    preBend: bool
+    release: float | None


instead of float, type as OffsetQL which is float|Fraction Music21 represents 1/3 as Fraction(1, 3) not 0.3333333428 as an inexact float.

mscuthbert · 2024-08-13T21:15:22Z

music21/musicxml/m21ToXml.py

+            # divisions relative to the current note.
+            releaseSubElement = SubElement(mxh, 'release')
+            quarterLengthValue = bend.release
+            divisionsValue = defaults.divisionsPerQuarter * quarterLengthValue


divisionsValue = int(defaults.divisionsPerQuarter * quarterLengthValue)

Musicxml "prefers" that offset be an integer:
https://www.w3.org/2021/06/musicxml40/musicxml-reference/data-types/divisions/

mscuthbert · 2024-08-13T21:16:46Z

music21/musicxml/xmlToM21.py

+                divisions = float(mxh.find('release').get('offset'))
+                bend.release = divisions / defaults.divisionsPerQuarter


In writing to musicxml, music21 uses defaults.divisionsPerQuarter. But in reading from musicxml, we need to use the divisions defined earlier in the score, which is in self.divisions on the measure parser.

Write

bend.release = opFrac(divisions / defaults.divisionsPerQuarter)

opFrac will convert it to Fraction(1, 3) if it's released within a triplet, etc. and leave it alone as 0.5 if it's a regular eighth note, etc.

adhooge and others added 17 commits November 21, 2022 17:00

⤴️ bend export

03fdb47

add some properties to note.Note

aca37e2

fix missing import

7118189

FretBend: update preBend type

766a113

FretBend: update Release type

17bc30d

noteToNotations: export string fret art in chord

4bcdffc

new changes

274c904

Merge remote-tracking branch 'upstream/master'

0fdac57

read bends

6d778f1

restore README

7e6ced8

single quotes

ef52fe0

typing withBar

e3f88ee

python3.9 compatibility

f1eae7d

remove useless files

22d07da

bendAlter typing

c157a53

rm useless file

67540ea

restore file

34ecc42

mscuthbert requested changes May 24, 2023

View reviewed changes

Alexandre D'Hooge and others added 10 commits March 18, 2024 12:20

fix some conflicts

13ac21f

add some properties to note.Note

9c3b1a3

fix missing import

0839230

finish rebase

cd534fe

FretBend: update Release type

b1c9277

noteToNotations: export string fret art in chord

caa8bd2

new changes

a21d3f6

read bends

7855e26

restore README

b95c2c0

single quotes

97297be

Alexandre D'Hooge and others added 29 commits August 13, 2024 22:53

new changes

bb8ab1d

read bends

4c5d96d

restore README

6646a04

single quotes

640f051

typing withBar

5ab284d

python3.9 compatibility

30c2a85

bendAlter typing

8262559

restore file

e36d0ff

add mute articulation

5cf8b7d

⤴️ bend export

63b9df5

FretBend: update preBend type

5dc55d9

FretBend: update Release type

11b6e7e

noteToNotations: export string fret art in chord

ba04559

single quotes

b2bd258

typing withBar

04d369e

python3.9 compatibility

8ef43f0

bendAlter typing

43e9277

restore file

38c0d61

alphabetical imports

adc65e3

rework types and default values

1420a42

FretBend: rework documentation

3ee1fba

separate method for FretBend

2773315

comment from musicXML doc

467317b

release to divisions

dae3be6

make setBend a sub-method

5c3b870

convert offset to quarter length

60fcfed

remove my properties

2c41586

FretBend: add tests

96782de

Merge branch 'pr-bend' of github.com:adhooge/music21 into pr-bend

e7e0db3

mscuthbert requested changes Aug 13, 2024

View reviewed changes

		bendAlterSubElement = SubElement(mxTechnicalMark, 'bend-alter')
		bendAlterSubElement.text = str(articulationMark.bendAlter.semitones)

		releaseSubElement = SubElement(mxTechnicalMark, 'release')
		releaseSubElement.set('offset', str(articulationMark.release))


		WithBar indicates if the bend is done using a whammy bar movement. Defaults to False.

		>>> fb = articulations.FretBend(bendAlter=interval.ChromaticInterval(-2), release=0.5)

		divisions = float(mxh.find('release').get('offset'))
		bend.release = divisions / defaults.divisionsPerQuarter

FretBend update #1580

Are you sure you want to change the base?

FretBend update #1580

Conversation

adhooge commented May 23, 2023

mscuthbert 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

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

Choose a reason for hiding this comment

mscuthbert commented Jul 1, 2023

adhooge commented Jul 10, 2023

mscuthbert 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment