-
Notifications
You must be signed in to change notification settings - Fork 662
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
fix #1265: Fix issues with vertical placement of annotations #1270
Conversation
@AaronDavidNewman there changes specially wit Tabs that I woul not expect. 1 Diff 2 Current 3 Reference(master) 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 |
@AaronDavidNewman do you know how to get the image differences above? I documented it in the WIKI "
Then run the following command in your branch:
" One other thing, to get the issue linked you have to place the |
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? |
Thanks folks -- @rvilarl from the diffs you pasted in, there's two things that stand out:
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? |
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.
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).
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. |
I will give it a try on Windows later today🙂 Master is broken, I cannot try now :( |
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... |
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. |
I think I have all the tab cases worked out now, except for I think the solution is to implement the same logic in bend - always make the staff line relative to the highest/lowest |
@AaronDavidNewman in #409 there is some discussion about |
Make Y position of Bend dependent on top string, like other modifiers
@AaronDavidNewman these two test have still to be looked at (first seems to be ok, just preesenting the complete stave): Annotation Fingerpicking Articulation Articulation___Vertical_Placement |
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.
Is this for a future change? b/c the current svgcontext is still using 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. |
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. |
This is ready for review |
This is merged and up to date. Diffs look basically the same. |
Awesome stuff Aaron -- made a minor edit to fix a lint error. Merging! |
Just looking at guitar bend thing again, it seems like it is inconsistent with how tabs are generally rendered. The Can we add some logic not to shift the Y if there's no collision? (or to override the shifting in some situations?) |
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 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. |
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: