Add support for chord diagrams in MusicXML #668
Conversation
Added support for frame elements to display chord diagrams. Also added some chord names to display for kind elements.
There was a problem hiding this comment.
Thanks for your contribution.
For importers it would be quite important to have good unit tests for all the added functionalities to ensure they really work as expected for the considered inputs. It would be great if you can take care of this.
Regarding the rendering I would say it is fine to always consider the chord.strings.length I think it does not make sense to have different combinations of strings in real-world files because one instrument can always have one set of strings and it does not make sense to have chord diagrams which do not reflect the instrument you're playing 😁 But it might happen that we have chord diagrams, without the staff defining strings (especially in MusicXML which is often more a graphical format than a semantic one). Also for the visual displays we would have a visual regression suite which would ensure nothing breaks in future, you could give it an attempt to extend it.
Beside that I added some remarks to the code itself, and as I approved the Actions you will notice that your changes break the non-web targets.
| case 'frame-strings': | ||
| let stringsCount:number = parseInt(frameChild.innerText); | ||
| for(let i = 0; i < stringsCount; i++){ // set strings unplayed as default | ||
| chord.strings[i] = -1; |
There was a problem hiding this comment.
AlphaTab is not only targeting JavaScript, this code will fail on some more strict platforms where the arrays are not dynamically growing as higher indexes are accessed. You should pre-allocate chord.strings before the loop with the right size or use .push here.
| for (let staff of track.staves) { | ||
| if(chord.strings.length > 0){ | ||
| for (let c of staff.chords.values()) { | ||
| if (String(c.strings) === String(chord.strings)) { |
There was a problem hiding this comment.
This will not work on any platforms beside web. Also the logic here seems quite performance intense, for each new chord we're looping again through all existing chords and comparing them on a string-by-string base (by utilizing some array-tostring magic here).
The more usual practice of alphaTab here is to keep some lookup map within the importer class as member and check through map functions if the chord already exists. It might be good to respect the id attribute of the harmony tag if it is available, and if not, try to generate a reasonable ID which allows deduplication based on the data we read. e.g. if strings are provided, we use the defined strings/frets, if they are not provided, we use the name.
|
Ping @KieranKS: Do you plan to still complete this PR? Otherwise I would close it for housekeeping purposes. |
Added support for frame elements to display chord diagrams. Also added some chord names to display for kind elements.
Issues
Issue #667 Add support for chord diagrams in MusicXML
Proposed changes
Add support for chord diagrams in MusicXML
Checklist
Further details
Issues displaying when discrepancy between staff.tuning and chord.strings.
This happens when using test file 71c-ChordsFrets.xml which has no staff-tuning element.
If staff-tuning elements are added for 6 strings the C chord (which in this file has 10 strings) overflows:
I don't know if there are any scores with varying numbers of strings in chord diagrams for the same part but quite few musicXML didn't have staff-tuning element.
Replacing _chord.staff.tuning.length with _chord.strings.length in ChordDiagramGlyph.ts fixed this but I haven't checked if this affects drawing chord diagrams from other file types so not committing.
Another option would be to not display chords with a different number of strings to staff.tuning or display them with the number of strings in staff.tuning.