fix #1265: Fix issues with vertical placement of annotations

[AaronDavidNewman]

fix #1265

Fix some issues with vertical placement and different modifiers, this time focusing on annotation. Also fixed some issues found
in my last PR with articulation placement. Because of an error in sign, a code leg would never have been traversed. I added a test case for that and fixed the error.

before:

after:

Note there are some tiny changes in other test cases, due to the fact we are calculating text height rather than hard-coded estimate. The text here is slightly closer to the note/stave in the new version, for instance.
before:

after:

[rvilarl]

@AaronDavidNewman there changes specially wit Tabs that I woul not expect. 1 Diff 2 Current 3 Reference(master)
Below all the visual differences (Bravura only)

Annotation TabNote_Annotations

Annotation Bottom_Annotations_with_Beams

Annotation Standard_Notation_Annotation

Annotation Test_Justification_Annotation_Stem_Down

Annotation Test_Justification_Annotation_Stem_Up

Articulation Articulation___Vertical_Placement

Rhythm Rhythm_Draw___beamed_slash_notes__some_rests

StaveNote Center_Aligned_Note_with_Annotation

Style Basic_Style

Annotation Harmonics

Annotation Fingerpicking

Annotation Bottom_Annotation

Annotation Simple_Annotation

Style TabNote_modifiers_Style

TabTie Tapping

[rvilarl]

@AaronDavidNewman do you know how to get the image differences above? I documented it in the WIKI
https://github.com/0xfe/vexflow/wiki/Visual-Regression-Tests

"
Alternatively, you can test against a particular commit (i.e.: master or any other commit). Run the following commands to generate the reference:

$ git checkout master
$ npm run reference

Then run the following command in your branch:

$ npm run test:reference

"

One other thing, to get the issue linked you have to place the fix #1265 in the first comment. For example one line at the beginning befor "Fix some issues with vertical placement and different modifiers..." If it is correctly linked you will get it in the Linked issues list to the right.

[AaronDavidNewman]

@AaronDavidNewman do you know how to get the image differences above

Does this work on a (windows) PC? I'm thinking probably not. Really excited about the headless chrome rendering.

[rvilarl]

@AaronDavidNewman do you know how to get the image differences above

Does this work on a (windows) PC? I'm thinking probably not. Really excited about the headless chrome rendering.

What error do you get following the procedure described in the wiki?

[0xfe]

Thanks folks -- @rvilarl from the diffs you pasted in, there's two things that stand out:

• Some of the annotations are too far from the top of the stave lines
• Vertically centered annotations are broken

Overall this is a good change, and I wonder if we should fix those issues inside font-specific metrics (maybe by adding some new shifts just for those cases.)

The alternative is to dive right into the whacky world of text font metrics, but I'd really like to avoid that. Instead if we can say some thing like "Vexflow works best with text fonts X, Y, and Z", and optimize for those cases, that would make our lives a lot simpler. Thoughts?

[AaronDavidNewman]

Some of the annotations are too far from the top of the stave

Mohit are you confusing before and after? The current annotations actually cling a little tighter to the top of the staff than they previously did. EDIT: maybe you are talking about the last tab test case in annotations_tests. I am still looking at that one.

Vertically centered annotations are broken

Indeed, but I didn't change those :) Those should be pretty much the same.

The only thing this PR broke (that I've seen so far) is the tab annotations. Tab notes seem to have a stem, but the stem is never rendered. I have a fix for that. Also annotations over rests were taking the stem seriously. So I just ignore stem in those 2 cases (stem.isRendered only tells you if it's already been rendered, doesn't help for formatting).

What error do you get following the procedure described in the wiki?

I am stuck on the dependencies (RSVG and ImageMagick for windows 10). I'm sure it's possible but it seems hard. I'll try to figure something out, maybe I can borrow a mac from somebody.

[rvilarl]

I will give it a try on Windows later today🙂 Master is broken, I cannot try now :(
I use Linux by the way
@AaronDavidNewman you are right. I tried with Windows and I did not succeed, it is as a minimum complex. :)

[h-sug1no]

I'm running vrtest on a w10 desktop PC using (vmware (workstation player)+ubuntu).

Building an environment for vrtest on windows (MinGW or cygwin) is complicated.

It would be easier to use linux in a virtual environment like hyper-v(wsl2), virtualbox, or vmware...
(it requires sufficient memory, cpu, and storage)

[sschmidTU]

When I need a Linux environment on Windows, I use VirtualBox (which is free compared to VMWare) + Linux Mint XFCE, which is supposed to be slim and fast.
WSL2 works, but is a bit annoying to set up, and WSL2 broke my rsync compared to WSL1 (while WSL1 can't do the visual regression bash script).
My VirtualBox Linux is quite slow compared to native, not sure why with virtualization support, but it works. (edit: setting processors to 4 on Settings -> System seemed to speed it up)

[AaronDavidNewman]

I think I have all the tab cases worked out now, except for Bend. Here, I think the issue is with Bend. Modifiers treat top_text_line or text_line inconsistently. Usually it is treated with regard to the note, but for modifiers that are always on top/bottom, the Bend.format method assumes the text line is relative to the staff. This causes issues when modifiers are mixed.

I think the solution is to implement the same logic in bend - always make the staff line relative to the highest/lowest TabNote depending on whether the placement is on top or bottom.

[rvilarl]

@AaronDavidNewman in #409 there is some discussion about top_text_line and text_line

[AaronDavidNewman]

I have the bend now considering other modifiers. There is one test case that is still different, but I think the change is correct:. This is how it used to look

The annotation and bend text are at the same level. If the annotation were longer, they would collide:

I'm not quite sure why you'd want to have a separate annotation instead of just adding the 'T" to the bend text. The bend text is now stacked on top of the bend (and any other modifiers):

[rvilarl]

@AaronDavidNewman these two test have still to be looked at (first seems to be ok, just preesenting the complete stave):

Annotation Fingerpicking

Current

Reference

Articulation Articulation___Vertical_Placement

Current

Reference

[AaronDavidNewman]

these two test have still to be looked at

Right, these are OK. In both cases I changed the test case to test more things, or fix the broken width. It is kind of annoying that the 'p' is a little below the line, though.

In the end I standarised in #1262 measureText

Is this for a future change? b/c the current svgcontext is still using svg.getBbox() to get the dimensions, rather than the text formatter.

There will be other PRs for the vertical formatting change, so we can change this code in one of those to use SVGContext if that is the preferred way to get text dimensions.

[rvilarl]

In the end I standarised in #1262 measureText

Is this for a future change? b/c the current svgcontext is still using svg.getBbox() to get the dimensions, rather than the text formatter.

Sorry you are right #1262 is not merged, I saw the congratulations from Simon and I assumed it was merged. @0xfe are you happy with the approach in #1262 for unifying the measurement of texts in CanvasContext and SvgContext. If #1262 is going to be merged I would recommend the change described above.

[AaronDavidNewman]

This is ready for review

[AaronDavidNewman]

This is merged and up to date. Diffs look basically the same.

[0xfe]

Awesome stuff Aaron -- made a minor edit to fix a lint error. Merging!

[0xfe]

Just looking at guitar bend thing again, it seems like it is inconsistent with how tabs are generally rendered. The T (tap) and Full (bend) should really be on the same Y if there's no collision.

Can we add some logic not to shift the Y if there's no collision? (or to override the shifting in some situations?)

[AaronDavidNewman]

Yeah, bend definitely came out a little worse :(

I can't think of an easy way to check for collisions. Ideally, the modifier context 'state' would be an array of bounding boxes. I think this is what @mscuthbert was alluding to above about Finale. That is a lot of rewriting, including things that currently work pretty well like accidentals. So that's the dream.

I can think of 2 possible short-term solutions. One is to simply switch the order of annotation and bend/vibrato in modifiercontext. That fixes it, and I don't think it's so awful because many modifiers don't really apply to tab notes, especially if there is a Bend.

The second is to allow bend to 'opt out' of vertical formatting, or allow annotation to 'opt out' of reporting its vertical placement. In this case the application would be responsible for avoiding collisions by moving the bounding box of the annotation. We have similar logic with ChordSymbol, because sometimes it's OK for a chord change to overlap other notes.

Edit: a third option is to just make the Tap be part of the bend. I think this is best, then you don't need an Annotation at all.