0xfe / vexflow

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

chord symbol metrics don't have 'scale' #1563

Closed AaronDavidNewman closed 1 year ago

AaronDavidNewman commented 1 year ago

Glyph chord symbol metrics in the font file used to have a metric called 'scale' for all the chord glyphs. This represented the default scale vs. the natural font scale of the glyph. That is missing now. This breaks smoosic b/c it uses this scale in the chord symbol editor UI. And I think this would affect the chord symbols as well.

I think the solution is just to add it back. For some glyphs, though, the scale was different for different fonts. Where do custom metrics go now? I only see 'common' metrics.

rvilarl commented 1 year ago

Glyph chord symbol metrics in the font file used to have a metric called 'scale' for all the chord glyphs. This represented the default scale vs. the natural font scale of the glyph. That is missing now. This breaks smoosic b/c it uses this scale in the chord symbol editor UI. And I think this would affect the chord symbols as well.

The scale was unified to 0.8 and following the metrics convention, the scale has now a default value one level above.

    chordSymbol: {
      csymDiminished: {
        scale: 0.8,
      },
      csymHalfDiminished: {
        scale: 0.8,
      },
      csymAugmented: {
        scale: 0.8,
      },
      csymParensLeftTall: {
        scale: 0.8,
      },
      csymParensRightTall: {
        scale: 0.8,
      },
      csymBracketLeftTall: {
        scale: 0.8,
      },
      csymBracketRightTall: {
        scale: 0.8,
      },
      csymParensLeftVeryTall: {
        scale: 0.8,
      },
      csymParensRightVeryTall: {
        scale: 0.8,
      },
      csymDiagonalArrangementSlash: {
        scale: 0.8,
      },
      csymMinor: {
        scale: 0.8,
      },
      csymMajorSeventh: {
        scale: 0.8,
      },
      accidentalSharp: {
        scale: 0.8,
      },
      accidentalFlat: {
        scale: 0.8,
      },
 }

is equivalent to

    chordSymbol: {
      scale: 0.8,
 }

I think the solution is just to add it back. For some glyphs, though, the scale was different for different fonts. Where do custom metrics go now? I only see 'common' metrics.

I have been working to get a common metric in order to allow reading additional fonts (usage directly of OTFs) without needing to hard code metrics for each font.

AaronDavidNewman commented 1 year ago

I thought we could introduce font-specific metrics where needed, and it would fall back to the common metric if there were none. Is that not possible?

rvilarl commented 1 year ago

We can introduce specific metrics calling Font.load an using a modified copy of the metrics:

Font.load('Bravura', BravuraFont, CommonMetrics);

Font.load('Bravura', BravuraFont, ModifiedMetrics);

ronyeh commented 1 year ago

Do we have an easy fix for this? Ideally, upgrading to 4.2 should not break existing apps like smoosic.

rvilarl commented 1 year ago

Undo the unification/simplification described above,

ronyeh commented 1 year ago

Even if it was unified to 0.8... can we add them all back (set to 0.8) for this 4.2 release? Then in 5.0 we can discuss this again.

rvilarl commented 1 year ago

Of course, yes

AaronDavidNewman commented 1 year ago

If you're interested, I solved this problem on my branch like so, I could push it. It uses only the custom metrics required for this font and this feature.

In load_petaluma.ts:


const petalumaChordMetrics: Record<string, ChordSymbolGlyphMetrics> = {
  csymDiminished: {
    scale: 0.8,
// .... custom metrics
  };
export function loadPetaluma() {
  const metrics: FontMetrics = JSON.parse(JSON.stringify(CommonMetrics));
  const chordMetrics = metrics.chordSymbol;
  if (chordMetrics) {
    chordMetrics.glyphs = petalumaChordMetrics;
  }
  Font.load('Petaluma', PetalumaFont, metrics);
rvilarl commented 1 year ago

@AaronDavidNewman Thank you! Please push it.

ronyeh commented 1 year ago

Can we submit it as a PR?

We can add a comment on top of that block that this is one way to customize metrics for a font.

On Sat, May 13, 2023, 11:49 AM Rodrigo Vilar @.***> wrote:

@AaronDavidNewman https://github.com/AaronDavidNewman Thank you! Please push it.

— Reply to this email directly, view it on GitHub https://github.com/0xfe/vexflow/issues/1563#issuecomment-1546729508, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2MCMSJYWKKNK3A44G7FLXF7JSRANCNFSM6AAAAAAXRB6BLQ . You are receiving this because you commented.Message ID: @.***>

AaronDavidNewman commented 1 year ago

Which branch is this? When I add my change to the main branch, I get this error:

ERROR in ./src/fonts/load_petaluma.ts:90:5
TS2322: Type '{ scale: number; leftSideBearing: number; advanceWidth: number; yOffset: number; }' is not assignable to type 'ChordSymbolGlyphMetrics'.
  Object literal may only specify known properties, and 'scale' does not exist in type 'ChordSymbolGlyphMetrics'.
    88 |   },
    89 |   accidentalFlat: {
  > 90 |     scale: 0.8,
       |     ^^^^^^^^^^
    91 |     leftSideBearing: -10,
    92 |     advanceWidth: 228,
    93 |     yOffset: -284,
rvilarl commented 1 year ago

@AaronDavidNewman what is the branch and repository that you are trying to merge? your fork of vexflow or vexflow-smoosic? Which branch?

ronyeh commented 1 year ago

@rvilarl Can we re-introduce the scale in such a way as it doesn't break smoosic? Let's add it back to the metrics file so that we can proceed with testing the 4.2 release.

rvilarl commented 1 year ago

@rvilarl Can we re-introduce the scale in such a way as it doesn't break smoosic? Let's add it back to the metrics file so that we can proceed with testing the 4.2 release.

I was asking @AaronDavidNewman about the branch he wants to merge to fix it.

AaronDavidNewman commented 1 year ago

I did a clone of the master vexflow branch. There is no scale for ChordSymbol metrics. I just checked on the main page, it's not there. I thought there was such an attribute. There is on my branch an I know I merged from the main at some point.

export interface ChordSymbolGlyphMetrics {
  leftSideBearing: number;
  advanceWidth: number;
  yOffset: number;
}
rvilarl commented 1 year ago

@AaronDavidNewman when you say your branch, which of your repos you refer to? vexflow or vexflow-smoosic? In master? This will help me figure out when you merged.

rvilarl commented 1 year ago

Hi @AaronDavidNewman I have looked into this. I defined ChordSymbolGlyphMetrics on the 08.12.2022 to avoid using any. Commit chordsymbol metrics. All you have to do is to add the definition of scale: number into ChordSymbolGlyphMetrics in your PR.

rvilarl commented 1 year ago

Hi @AaronDavidNewman I was trying to make a PR to put it back in and it is more complex. metrics.scale are since long ago no longer used. See:

   // Note: symbol widths are resolution and font-independent.
    // We convert to pixel values when we know what the font is.
    if (symbolType === SymbolTypes.GLYPH && typeof params.glyph === 'string') {
      const glyphArgs = ChordSymbol.glyphs[params.glyph];
      const glyphPoints = 20;
      symbolBlock.glyph = new Glyph(glyphArgs.code, glyphPoints, { category: 'chordSymbol' });
      // Beware: glyph.metrics is not the same as glyph.getMetrics() !
      // rv.glyph.point = rv.glyph.point * rv.glyph.metrics.scale;
      // rv.width = rv.glyph.getMetrics().width;
      // don't set yShift here, b/c we need to do it at formatting time after the font is set.
    } else if (symbolType === SymbolTypes.TEXT) {
      symbolBlock.width = this.textFormatter.getWidthForTextInEm(symbolBlock.text);
    } else if (symbolType === SymbolTypes.LINE) {
      symbolBlock.width = params.width;
    }

The scale should be defined under glyph.chordsymbol as I described in my first comment above.

rvilarl commented 1 year ago

If we want the scale in ChordSymbolGlyphMetrics the code above has to be changed to:

- const glyphPoints = 20;
+ const glyphPoints = 20 * metrics.scale;
- symbolBlock.glyph = new Glyph(glyphArgs.code, glyphPoints, { category: 'chordSymbol' });
+ symbolBlock.glyph = new Glyph(glyphArgs.code, glyphPoints,);

@AaronDavidNewman how do you want me to proceed?

AaronDavidNewman commented 1 year ago

Sorry, I am out of practice. I figured it out and created a pull request just now.