vexflow / vexflow

TypeScript library for rendering music notation & guitar tablature.
Other
54 stars 10 forks source link

SVG: For text elements, hit-testing is performed on a character cell basis #155

Closed AaronDavidNewman closed 2 months ago

AaronDavidNewman commented 10 months ago

return VF.Element.measureWidth(smoGlyph.vexGlyph);

returns about 86 px for a 'gClef', but the actual width is about 23 px. All the glyphs have this issue, so the layout ends using too much space.

Was: image

Is: image

Also, the height of the glyphs' bounding box is somehow wrong. The bounding box returns height of 160 even though the notehead that is being measures is about 14 px.

image

image

image

rvilarl commented 10 months ago

@AaronDavidNewman what font size are you using? how doyou measure the height? Would it be possible to get the image above in SVG format?

AaronDavidNewman commented 10 months ago

I think these issues are unrelated. The issue with the height bounding box is something you can also see on the vexflow test page, if you just hover over any SVG glyph in the element inspector. It is strange, I am not sure of the source. Possibly it is returning the max height of the font, and not the height of the glyph in question. This is different behavior than you get for the bounding box of the path.

For the width, I think I see the issue. Passing the glyph 'gClef' into Element.measure will measure the width of the string 'gClef', not the actual glyph. If I get the width of VexFlow.glyphs[glyph], it is correct. So that one is easily worked around.

rvilarl commented 10 months ago

@AaronDavidNewman if you look at the initialisation of TExtMetrics in Element, you will see

#textMetrics: TextMetrics = {
    fontBoundingBoxAscent: 0,
    fontBoundingBoxDescent: 0,
    actualBoundingBoxAscent: 0,
    actualBoundingBoxDescent: 0,
    actualBoundingBoxLeft: 0,
    actualBoundingBoxRight: 0,
    width: 0,
  };

with TextMetrics you get both the font height and the actual text height.

AaronDavidNewman commented 10 months ago

@rvilarl, I get that, but getting the metrics of a glyph and getting the bounding box of an element are different things. When planning the layout, you need the size of the glyphs. But after the music is drawn, you need to bounding boxes for the UI. Otherwise, Smoosic has no way to know where the individual note heads are, so notes can be selected by the UI. Also, I usually want the bounding box of the note head + stem for the UI, which is the parent StaveNote element.

rvilarl commented 10 months ago

Does not Element.getBoundingBox support what you describe? If not, we should be able to fix it.

rvilarl commented 10 months ago

The topic pointer events with text seems to be an issue that might be a show stopper with interactions.

This event fires on the text cell and not on the glyph. See interactive mouseovers in https://vexflow.github.io/vexflow-examples/tests/flow.html

@ronyeh @jaredjj3 any idea how to solve this?

rvilarl commented 10 months ago

As per https://www.w3.org/TR/SVG/interact.html

" For text elements, hit-testing is performed on a character cell basis:

@ronyeh @jaredjj3 @0xfe @mscuthbert @schmidTU @AaronDavidNewman any idea how to get around this? This breaks the interaction with the glyphs.

rvilarl commented 10 months ago

I believe that I found a solution. Setting a transparent rect to capture the pointer events. I will raise a PR now. @AaronDavidNewman the PR considers StaveNotes, what other elements would you like to get pointer events?

sschmidTU commented 10 months ago

offtopic note: Looks like a great find, thanks, Aaron! I've noticed Vexflow 4/5 uses much more space than Vexflow 1, making scores much bigger, so if this can reduce that, that would be fantastic. Unfortunately I don't have much else to contribute right now, I would have to do a deep dive on SVG text measuring, etc.

AaronDavidNewman commented 10 months ago

I don't know about the pointers - is that related to the bounding box issue? I don't use them - I create my own events based on the note IDs. Pointers are only good for pointing events but I need to know where the music is before someone clicks around it, so the keyboard can navigate through the music, playback, scrolling and etc.

For this reason, I guess I don't understand - what does having them around a text element break?

rvilarl commented 10 months ago

@AaronDavidNewman @sschmidTU thanks for your feedback.

@AaronDavidNewman your example above with:

<g class="vf-notehead" id=... pointer-events="bounding-box"> 

made me think that the bounding box error would also be present in the StaveNote tests with interactive mouseover. And that was the case, I saw that moving the cursors up and down the note, quite far away, the note was changing the color. Please have a look at the PR to see the change. Still one question, then you do not use the VexFlow SVG infrastructure to capture the pointer over elements, do you?

rvilarl commented 10 months ago

I think these issues are unrelated. The issue with the height bounding box is something you can also see on the vexflow test page, if you just hover over any SVG glyph in the element inspector. It is strange, I am not sure of the source. Possibly it is returning the max height of the font, and not the height of the glyph in question. This is different behavior than you get for the bounding box of the path.

@AaronDavidNewman #156 also resolves this issue.

AaronDavidNewman commented 10 months ago

Resolves it how? Does it fix the bounding box of the element just to remove the pointer event? Or do I have to get the bounding box from the transparent element? I know about the element id that contains the text, because I opened the svg group when I added the note to the stave, but I won't know about the transparent element unless I hunt for it.

I know there are people with smaller apps that use the pointer stuff, since it's an easy way to get the mouse events, so I'm fine with a solution just for that. Although it would be nice if we could opt out of the pointer events (and extra DOM element that it now entails), maybe when we create the renderer.

rvilarl commented 10 months ago

@AaronDavidNewman I do not know how you are handling the events and why the bounding box of the text does not fit. Could you share some code in a jsfiddle or as text. Perhaps I can figure something else out.

AaronDavidNewman commented 10 months ago

@rvilarl I am not using DOM events, I am using the browser svg method getBBox() to get the constraints of the rendered note. Here is a fiddle.

https://jsfiddle.net/AaronDavidNewman/nc0q43wf/

Edit: And I don't think there is any way to get the correct height in an SVG bounding box, in anything actually rendered using one of the music fonts in javascript. The issue is the height of the font in the font file. This will be returned as the max height for all glyphs in this font, regardless of the actual height. I think this is just the price we pay for using text font glyphs vs. extracting the paths and rendering those.

See this SO:

This is using getBBox()

In Bravura, for instance, the max height of the font is ~4x the size of the base unit. So any glyph this font will return a height of 160 px given a 30pt size of the font (30 4 1.3333).

I'm not sure if this is a show-stopper for Smoosic or not. I only need the height to create the boxes for the interactive UI, and it doesn't have to be exact. I could possibly get around this by just using a reasonable value for the height (maybe divide by 2). For height estimation, we have the number of leger lines and we know how big those are.

rvilarl commented 10 months ago

@AaronDavidNewman why don´t you use the library function?

https://jsfiddle.net/m7nqL8co/

AaronDavidNewman commented 10 months ago

@rvilarl I may be able to do that if all I need is the stave note. I really need the bounding box of an the element containing the note (including modifiers). Not everything in vexflow has a boundingBox method. It won't work for any element that contains a stave note and something else, or doesn't contain a stave note at all. getBBox works for every element.

It may be more performant to get the box from the objects, where possible. If I could get the modifier extent with the note, it might be worth switching different bounding box methods.

btw it looks like the getBoundingBox method in 4.x did include the width of dots, but the 5.0 one doesn't. So that may be a new bug. See the second fiddle for an example in 4.x.

e.g:

https://jsfiddle.net/AaronDavidNewman/tag31eLj/

vs. https://jsfiddle.net/AaronDavidNewman/pmzbyqoe/

rvilarl commented 10 months ago

@AaronDavidNewman if you list what elements you need, I will be happy to fix the bounding boxes. And yes there are bugs :)

ronyeh commented 10 months ago

Thanks for this discussion.

From what I can tell, original VexFlow wasn't designed with post-render user interaction as a priority.

The interactivity test case basically directs the developer to attach event handlers directly to the SVG paths (e.g., note heads).

I think to support interactivity, we need to figure out: 1) what is the ideal shape of the target for mouse pointer events for a particular item? Is it a union of boxes that include all note heads, the stem, the dot, and the flat/sharp symbols? What about any other modifiers like staccato, or is that separate from the main item/element? 2) what happens when an "item" is moved or adjusted. How fast can we reformat the score? Can we just reformat and render one measure at a time?

I think for 1. above, the best API would be to make it easy for the developer to attach/detach mouse event handlers for that "item". The default "item" might include all note heads and stem and modifiers for that particular beat in that particular voice. However, it is possible the developer might want more fine grained picking (like allowing their user to click on a single note head, to drag it into a different position of the staff). In that case, the API should be flexible to allow the selecting of parts of an item (individual note heads or accidental symbols).

But yes, I think the problem with using the SMuFL glyphs directly in the SVG is that the glyph may have a very large bounding box (with mostly empty space). This causes the line height of that <text> element to be really tall, so a simple implementation of mouse hover / click will fail due to the very large default bounding box.

Summary: we need to figure out how to calculate tight bounding polygons around staff items and then use those polygons as the targets for mouse events.

rvilarl commented 10 months ago

Hi Ron, thanks for your input!

I think that the number one priority is to decide which elements should be grouped to calculate their bounding box. Ideally we should group them also this way into SVG. Moving from bounding box to polygons would be an improvement afterwards.

I believe that the interactions in VexFlow were just thought to do hover or change colours (I am planning to use it with pianoplay)

Repositioning of elements is quite complex. These editing functions are provided by smoosic. However in regards to simply graphic movement I was a while ago looking at grouping of svg elements with relative positioning. There are two ways to do that:

see https://stackoverflow.com/questions/8028746/embedding-svg-in-svg But this comes with other complexities: where should be located the origin of the group....

Summary: we have to decide first which elements should be grouped

ronyeh commented 10 months ago

I agree with your summary. Maybe we need to print out some representative scores and use a red marker to hand draw bounding boxes around groups. That will give us an idea of what needs to be grouped together.

Another solution to get tight bounding boxes is to fall back to the old way.... If a developer needs interactivity with the SVG shapes and paths, they need to set a flag to render paths as we did in V4. Then instead of using smufl text characters, vexflow will render the paths at runtime using opentype.js. So we would be parsing the glyphs at runtime and converting them to paths.

rvilarl commented 10 months ago

Another solution to get tight bounding boxes is to fall back to the old way.... If a developer needs interactivity with the SVG shapes and paths, they need to set a flag to render paths as we did in V4. Then instead of using smufl text characters, vexflow will render the paths at runtime using opentype.js. So we would be parsing the glyphs at runtime and converting them to paths.

I moved to the usage of fonts for two reasons:

My first proposal was to use opentype but Mohit said that he would prefer not to have external dependencies. Then one day you commented on font-face and using fonts.

I would not fall back to the old hardcoded way.

rvilarl commented 10 months ago

I agree with your summary. Maybe we need to print out some representative scores and use a red marker to hand draw bounding boxes around groups. That will give us an idea of what needs to be grouped together.

I would suggest to also look into the groups done by smoosic.

rvilarl commented 10 months ago

@rvilarl I may be able to do that if all I need is the stave note. I really need the bounding box of an the element containing the note (including modifiers). Not everything in vexflow has a boundingBox method. It won't work for any element that contains a stave note and something else, or doesn't contain a stave note at all. getBBox works for every element.

Hi @AaronDavidNewman I have been using your demo this morning. Thanks for mentioning us on the README (there is a typo in my nich) As you mention above I will fix the stave note bounding box to include the modifiers.

ronyeh commented 10 months ago

I would not fall back to the old hardcoded way.

Nope. No more hard coding the paths for each glyph!

I would support using opentype.js as an optional runtime dependency to convert font glyphs to paths for the purposes of perfect bounding boxes (to enable mouse interactions).

We've been using opentype.js to extract font paths for a very long time. It was a dev time dependency. But here it would be optional, and only used by developers who want the v4/v3 style SVG paths again. Another benefit is that SVG that contain paths are portable to any system, even if they don't have smufl fonts installed.

ronyeh commented 8 months ago

I just merged #156. We should check if this issue is still a problem.

rvilarl commented 3 months ago

Hi @AaronDavidNewman is this still an issue? Have you found other bugs on bounding boxes? @jaredjj3 is now using the StaveNote bounding boxes

rvilarl commented 2 months ago

Should we close this issue?

AaronDavidNewman commented 2 months ago

If you can get the bounding box of a note that includes accidentals and dots, I think that would get at what this issue is about. I don't know when I will have a chance to test this in Smoosic again, if other issues are found we can open them at that time.

rvilarl commented 2 months ago

image

@AaronDavidNewman this is already in and available in alpha.4