-
Notifications
You must be signed in to change notification settings - Fork 406
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
FretBend update #1580
base: master
Are you sure you want to change the base?
FretBend update #1580
Changes from 17 commits
03fdb47
aca37e2
7118189
766a113
17bc30d
4bcdffc
274c904
0fdac57
6d778f1
7e6ced8
ef52fe0
e3f88ee
f1eae7d
22d07da
c157a53
67540ea
34ecc42
13ac21f
9c3b1a3
0839230
cd534fe
b1c9277
caa8bd2
a21d3f6
7855e26
b95c2c0
97297be
3d5092a
e45f7b8
4f07cbb
97e33a0
edf7934
27974be
3cf71e0
d63c78a
03e6117
85cc6a8
c11ce14
d93af6d
2e1ea43
58eeb04
0b2074f
f215c8c
e4499dc
0ba3f33
32d5135
030935a
a40c9d6
4938f1e
ded5058
bb8ab1d
4c5d96d
6646a04
640f051
5ab284d
30c2a85
8262559
e36d0ff
5cf8b7d
63b9df5
5dc55d9
11b6e7e
ba04559
b2bd258
04d369e
8ef43f0
43e9277
38c0d61
adc65e3
1420a42
3ee1fba
2773315
467317b
dae3be6
5c3b870
60fcfed
2c41586
96782de
e7e0db3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,7 @@ | |
from music21.common.classTools import tempAttribute | ||
from music21 import environment | ||
from music21 import style | ||
from music21 import interval | ||
from music21 import spanner | ||
|
||
if t.TYPE_CHECKING: | ||
|
@@ -582,10 +583,27 @@ class PullOff(spanner.Spanner, TechnicalIndication): | |
pass | ||
|
||
class FretBend(FretIndication): | ||
bendAlter: interval.IntervalBase | None = None | ||
preBend: t.Any = None | ||
release: t.Any = None | ||
withBar: t.Any = None | ||
bendAlter: interval.IntervalBase | ||
preBend: bool = False | ||
release: t.Optional[float] | ||
withBar: t.Optional[bool] | ||
|
||
def __init__(self, number=0, preBend=False, release=None, withBar=None, **keywords): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The other keywords should be typed in the We no longer use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed with d63c78a |
||
''' | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tried to improve it in 03e6117 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests are on their way |
||
''' | ||
super().__init__(**keywords) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bug. needs to be |
||
self.bendAlter = interval.ChromaticInterval(number) | ||
self.preBend = preBend | ||
self.release = release | ||
self.withBar = withBar | ||
|
||
class FretTap(FretIndication): | ||
pass | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,7 +63,7 @@ | |
from music21.musicxml import helpers | ||
from music21.musicxml.partStaffExporter import PartStaffExporterMixin | ||
from music21.musicxml import xmlObjects | ||
from music21.musicxml.xmlObjects import MusicXMLExportException | ||
from music21.musicxml.xmlObjects import MusicXMLExportException, booleanToYesNo | ||
from music21.musicxml.xmlObjects import MusicXMLWarning | ||
|
||
environLocal = environment.Environment('musicxml.m21ToXml') | ||
|
@@ -5486,7 +5486,6 @@ def articulationToXmlTechnical(self, articulationMark): | |
# these technical have extra information | ||
# TODO: hammer-on | ||
# TODO: pull-off | ||
# TODO: bend | ||
# TODO: hole | ||
# TODO: arrow | ||
musicXMLTechnicalName = None | ||
|
@@ -5498,7 +5497,7 @@ def articulationToXmlTechnical(self, articulationMark): | |
musicXMLTechnicalName = 'other-technical' | ||
|
||
# TODO: support additional technical marks listed above | ||
if musicXMLTechnicalName in ('bend', 'hole', 'arrow'): | ||
if musicXMLTechnicalName in ('hole', 'arrow'): | ||
musicXMLTechnicalName = 'other-technical' | ||
|
||
mxTechnicalMark = Element(musicXMLTechnicalName) | ||
|
@@ -5523,7 +5522,17 @@ def articulationToXmlTechnical(self, articulationMark): | |
mxTechnicalMark.text = str(articulationMark.number) | ||
if musicXMLTechnicalName == 'fret': | ||
mxTechnicalMark.text = str(articulationMark.number) | ||
|
||
if musicXMLTechnicalName == 'bend': | ||
bendAlterSubElement = SubElement(mxTechnicalMark, 'bend-alter') | ||
bendAlterSubElement.text = str(articulationMark.bendAlter.semitones) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New submethod added in 85cc6a8 |
||
if articulationMark.preBend: | ||
preBendSubElement = SubElement(mxTechnicalMark, 'pre-bend') | ||
if articulationMark.release is not None: | ||
releaseSubElement = SubElement(mxTechnicalMark, 'release') | ||
releaseSubElement.set('offset', str(articulationMark.release)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should work with d93af6d though I need to write the tests |
||
if articulationMark.withBar is not None: | ||
withBarSubElement = SubElement(mxTechnicalMark, 'with-bar') | ||
withBarSubElement.text = str(articulationMark.withBar) | ||
# harmonic needs to check for whether it is artificial or natural, and | ||
# whether it is base-pitch, sounding-pitch, or touching-pitch | ||
if musicXMLTechnicalName == 'harmonic': | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sub-method and bend-alter check in 2e1ea43 |
||
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')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above -- offset needs to be converted to music21 quarterLengths. except clause without Try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both fixed with 58eeb04 |
||
except TypeError: | ||
# offset is not mandatory | ||
tech.release = 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. release is a float. needs to be 0.0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also fixed with 58eeb04 |
||
# TODO: <bend> attr: accelerate, beats, first-beat, last-beat, shape (4.0) | ||
# TODO: <bent> sub-elements: bend-alter, pre-bend, with-bar, release | ||
# TODO: musicxml 4: release sub-element as offset attribute | ||
|
||
|
||
self.setPlacement(mxObj, tech) | ||
return tech | ||
else: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
from music21 import style | ||
from music21 import tie | ||
from music21 import volume | ||
from music21 import articulations | ||
|
||
if t.TYPE_CHECKING: | ||
from music21 import articulations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above -- alphabetical and remove from type-checking if imported at top. (but won't be necessary). |
||
|
@@ -1784,6 +1785,49 @@ def pitchChanged(self): | |
if self._chordAttached is not None: | ||
self._chordAttached.clearCache() | ||
|
||
@property | ||
def string(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops sorry about that, they were just for my personal convenience. Removed with 0b2074f |
||
''' | ||
Returns articulations.StringIndication if object has one. | ||
''' | ||
for art in self.articulations: | ||
if isinstance(art, articulations.StringIndication): | ||
return art | ||
return None | ||
|
||
|
||
@property | ||
def fret(self): | ||
''' | ||
Returns articulations.FretIndication if object has one. | ||
''' | ||
for art in self.articulations: | ||
if isinstance(art, articulations.FretIndication): | ||
return art | ||
return None | ||
|
||
@property | ||
def isBended(self): | ||
''' | ||
Returns True if the Note has a FretBend articulation. False otherwise. | ||
''' | ||
for art in self.articulations: | ||
if isinstance(art, articulations.FretBend): | ||
return True | ||
return False | ||
|
||
@property | ||
def bend(self): | ||
''' | ||
Returns the articulations.FretBend object if there is one in self.articulations. | ||
Returns None otherwise. | ||
''' | ||
if not self.isBended: | ||
return None | ||
for art in self.articulations: | ||
if isinstance(art, articulations.FretBend): | ||
return art | ||
|
||
# ------------------------------------------------------------------------------ | ||
# convenience classes | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alphabetical order (existing mistake with style before spanner can be fixed in this PR also)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 3cf71e0