music-encoding / sibmei

Sibelius MEI Plugin
MIT License
39 stars 16 forks source link

Introduce line handlers #214

Closed th-we closed 11 months ago

th-we commented 1 year ago

Use the same mechanism for handling lines as we use for text and symbols.

There is one thing I'm not entirely sure about. Should @endid always point to the note the line ends at? For example, in this case of a vibrato line:

grafik

The endpoint is attached to the second note (because it lasts until that very point), but the vibrato line does not apply to the second note.

Should @endid simply be omitted for some line types? They will still have @tstamp2.

ahankinson commented 1 year ago

Yes, @endid should be omitted if the line does not cover the ending.

th-we commented 1 year ago

O.K., I've changed the behavior to not include @endid for all line styles. However, I'm not 100% content yet. Specifically, @endid can currently only be added if the line end is perfectly attached to the end position of a NoteRest. However, Sibelius does not always create the lines like that and the user can not easily see it.

I could imagine these options to choose from different @endid behaviors for different line styles:

A suggestion for how this could be achieved: With the declarative line handling, we could introduce something like this, e.g. for:

'line.staff.vibrato', CreateSparseArray('Line', CreateDictionary(
  'form', 'wavy',
  'type', 'vibrato',
  'endid', ClosestPrecedingNoteRest
))

...where ClosestPrecedingNoteRest is an object that tells the line handler what to do. For extensions, it can be made accessible as api.ClosestPrecedingNoteRest.

Better naming ideas for ClosestPrecedingNoteRest/ClosestFollowingNoteRest are welcome as these names don't really transport that a NoteRest that precisely matches the endpoint of the line will be chosen as @endid.

As extending and maintaining the existing test files became really confusing, I tried something akin to Schematron rules in 6bdccbd7ab7f2215cc63bfdc8a3f74d044cd5769 which hopefully makes creating, understanding and maintaining tests much easier. Here is a screenshot, which should be pretty self explanatory: grafik The test expression is defined in the Sibelius file and will be exported to a special annotation. The JavaScript test code is reduced to a boilerplate, which could eventually be removed as we could also iterate over all test files and evaluate all the exported XPath test annotations. Some things might still be more sensible to test with JavaScript.

If this approach seems sensible to you, I would probably write any new tests in this fashion. It makes it much clearer to see what object a test applies to, and it does not require to keep two test files (JavaScript and Sibelius) in sync. Also, in the output MEI, the test and the exported object are very close together for checking. In the case of the screenshot above:

<octave xml:id="m-74" dis="8" endid="#m-69" layer="1" place="above" staff="1" startid="#m-68" tstamp="1" tstamp2="0m+3" vo="10.5mm"/>
<anchoredText xml:id="m-75">line.staff.octava.plus8</anchoredText>
<annot xml:id="m-76" layer="1" staff="1" startid="#m-69" tstamp="1.2734" type="xpath-test" vo="1.75mm">octave[@endid][@tstamp=1][@tstamp2=&apos;0m+3&apos;]</annot>
th-we commented 11 months ago

O.K., from my side, this is ready to merge now. Could anyone run the tests on their setup?

th-we commented 11 months ago

I addressed the different @endid handling for different line styles using placeholder values that are resolved when then ending bar for the line is reached. The way it works is documented for the function HandleLineTemplate() over here.

th-we commented 11 months ago

I noted that many of the line-like objects you've converted are now set to PreciseMatch which is a pretty big change in behaviour, I think? If I recall, for example, slurs were set to Closest before

You're right! I guess it would be good if you (@ahankinson, @rettinghaus, @annplaksin) could have a look at my choices for PreciseMatch, Previous, Next and Closest here (without wanting to distract from the rest of the code).

Thanks for the quick reaction!

ahankinson commented 11 months ago

Honestly I would just change them all to Closest, and then only use PreciseMatch in places where you know that's what you want / need.

(I know that the "Closest" @endid resolver has led to some annoyed people in the past, but it's better than the alternative which is a line that never resolves....)

th-we commented 11 months ago

I don't think it's that dramatic if a line does not resolve to an @endid, it will still have @tstamp2. Just like for trill lines:

grafik

The end of the trill line precisely matches the following note, but as we agreed, that should not be the @endid as the trill line does not apply to this note any more. So currently, I do not set an @endid and only rely on @tstamp2.

For easier review, here's a list of search strategies and the styles I currently use them for:

I don't think that Closest would be the correct strategy for the ones that are listed as Previous because they are all bracket types. A bracket will not apply to a following event, even if it is closer to the bracket end than the previous event.

Concerning the ones where I used Next, I'd be O.K. with converting them to PreciseMatch(but not closest, as they all point to something "later" in time).

rettinghaus commented 11 months ago

I agree with @th-we that this is probably the best approach.

@th-we PR still in conflict with target, please resolve.

rettinghaus commented 11 months ago

@ahankinson may I proceed?

ahankinson commented 11 months ago

Sure! Thanks.

th-we commented 11 months ago

Did anyone run the tests?

ahankinson commented 11 months ago

Oh, wait! I just spotted something -- I left a review comment.

ahankinson commented 11 months ago

I didn't run the tests

annplaksin commented 11 months ago

I currently cannot run any tests because I have no running Sibelius available. Sorry!

th-we commented 11 months ago

O.K., I took the chance to further simplify how line templates can be registered, especially in extension plugins. From my side, this is ready to merge. At least for me, tests all pass.

rettinghaus commented 11 months ago

I made some more tests and everything looks good to me. Thank you!