0xfe / vexflow

A JavaScript library for rendering music notation and guitar tablature.
http://www.vexflow.com
Other
3.88k stars 661 forks source link

Fix stems + noteheads #1415

Closed ronyeh closed 1 year ago

ronyeh commented 2 years ago

https://github.com/0xfe/vexflow/pull/1401#issuecomment-1163651219

Recent PRs have caused some stems to be detached from the noteheads. We need to update the metrics files (esp for Leland) so that the stems and noteheads connect smoothly.

ronyeh commented 2 years ago

@rvilarl if you have time can you look into this? I noticed that in the new test cases from recent merges, some note heads don't connect well to their stems. I think we need to adjust the font metrics files (especially in Leland) to make the note heads match up again. For example, a couple of note heads in Gonville changed recently. I think that may have resulted in the note head not aligning with the stems anymore.

I think this issue should be resolved before the 4.1 release, since it is a regression, in some ways.

rvilarl commented 2 years ago

@ronyeh this is only adjustment of metric files, I think that it can be done after 4.1, I would not wait for that. My problem is that I doubt on how some noteheads should be connected to the stem. I will look into it. @AaronDavidNewman could you help here? @ronyeh by the way have you looked into the open PRs?

rvilarl commented 2 years ago

These are the results @ronyeh @AaronDavidNewman what are the must fix according to you?

Leland NoteHead Various_Heads pptr-NoteHead Various_Heads Leland svg NoteHead Various_Heads_1 pptr-NoteHead Various_Note_Heads_1 Leland NoteHead Various_Heads_2 pptr-NoteHead Various_Note_Heads_2 Leland

Bravura NoteHead Various_Heads pptr-NoteHead Various_Heads Bravura svg NoteHead Various_Heads_1 pptr-NoteHead Various_Note_Heads_1 Bravura NoteHead Various_Heads_2 pptr-NoteHead Various_Note_Heads_2 Bravura

Gonville NoteHead Various_Heads pptr-NoteHead Various_Heads Gonville svg NoteHead Various_Heads_1 pptr-NoteHead Various_Note_Heads_1 Gonville NoteHead Various_Heads_2 pptr-NoteHead Various_Note_Heads_2 Gonville

Petaluma NoteHead Various_Heads pptr-NoteHead Various_Heads Petaluma svg NoteHead Various_Heads_1 pptr-NoteHead Various_Note_Heads_1 Petaluma NoteHead Various_Heads_2 pptr-NoteHead Various_Note_Heads_2 Petaluma

rvilarl commented 2 years ago

These are the results @ronyeh @AaronDavidNewman what are the must fix according to you?

I would say: 1) the stem up in Various Heads 2 2) the lack of space between Notes in Various Heads 1

rvilarl commented 2 years ago

@ronyeh this topic is not so easy to make it properly. I am trying to understand how to align the headnotes properly and the logic is distributed in several places: in the xxx_metrics.ts files in noteHead and stem and also in the tables.ts file in glyphProps. In tables we also still use any making it difficult to understand where the different values are used. My plan is:

AaronDavidNewman commented 2 years ago

Sorry, I'm just now catching up. So this is kind of a long response :)

I don't have a priority preference but X notehead is pretty common in jazz, slash and triangle note head is pretty common for percussion. So we could fix up the metrics for those symbols easy enough.

Also, I notice that we have don't have any slash note head cases in the note head test set (they are in the easy score set).

Finally, it would be nice if we could decouple the note head from the duration and key somehow. This may be outside the scope of this issue, and it's a legacy problem. For instance, currently there is a 'type' in the NoteStruct, a note type that comes from the duration string from 'Tables.getGlyphProps', and a note type in the key string from StaveNote.calculateKeyProps. It's not clear what the precedence is. You should be able to specific a note head and override any default that is assigned from key or duration. StaveNote has 'custom' but there is no way to set that in the constructor. This would be required for some of the Smufl note heads we don't support.

btw I like the idea of renaming this.glyph in StaveNote and NoteHead to this.glyphProps. That is a good first step.

rvilarl commented 1 year ago

I have been looking into this topic and it seems that we are using the metrics to resolve another problem. The width of the custom noteheads are not calculated properly. I have added the following lines to notehead.draw:

      Glyph.renderGlyph(ctx, head_x, y, glyph_font_scale, this.glyph_code, {
        category: this.custom_glyph ? `noteHead.custom.${categorySuffix}` : `noteHead.standard.${categorySuffix}`,
      });
+      ctx.setFillStyle('#3a2');
+      ctx.fillRect(head_x, y, this.width, 2);
+      ctx.setFillStyle('black');
    }

and I get the following

This one is fine NoteHead Basic Bravura Here the Xs are wrong NoteHead Various_Heads Bravura Here a lot of them are wrong NoteHead Various_Note_Heads_1 Bravura

@AaronDavidNewman any idea?

AaronDavidNewman commented 1 year ago

Short answer, no. I know there was a problem where the note was using the default notehead width for formatting, instead of the actually chosen one. But I thought that was fixed.

How come (for the incorrect 'X' ones) the green line doesn't start at the start of the glyph, X-wise? The notehead glyphs should all start at x=0.

rvilarl commented 1 year ago

Thanks for the tip, that was exactly the problem.