Skip to content
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 Vertical Layout #432

Merged
merged 5 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions samples/DrawWithImageSharp/DrawWithImageSharp.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<DebugType>portable</DebugType>
Expand Down Expand Up @@ -46,7 +46,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="SixLabors.ImageSharp.Drawing" Version="2.1.3" />
<PackageReference Include="SixLabors.ImageSharp.Drawing" Version="2.1.4" />
<PackageReference Include="System.ValueTuple" Version="4.5.0" />
</ItemGroup>

Expand Down
4 changes: 4 additions & 0 deletions src/SixLabors.Fonts/FileFontMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ internal override bool TryGetGlyphId(
out bool skipNextCodePoint)
=> this.fontMetrics.Value.TryGetGlyphId(codePoint, nextCodePoint, out glyphId, out skipNextCodePoint);

/// <inheritdoc/>
internal override bool TryGetCodePoint(ushort glyphId, out CodePoint codePoint)
=> this.fontMetrics.Value.TryGetCodePoint(glyphId, out codePoint);

/// <inheritdoc/>
internal override bool TryGetGlyphClass(ushort glyphId, [NotNullWhen(true)] out GlyphClassDef? glyphClass)
=> this.fontMetrics.Value.TryGetGlyphClass(glyphId, out glyphClass);
Expand Down
13 changes: 13 additions & 0 deletions src/SixLabors.Fonts/FontMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,19 @@ internal FontMetrics()
/// </returns>
internal abstract bool TryGetGlyphId(CodePoint codePoint, CodePoint? nextCodePoint, out ushort glyphId, out bool skipNextCodePoint);

/// <summary>
/// Gets the specified glyph id matching the codepoint.
/// </summary>
/// <param name="glyphId">The glyph identifier.</param>
/// <param name="codePoint">
/// When this method returns, contains the codepoint associated with the specified glyph id,
/// if the glyph id is found; otherwise, <value>default</value>.
/// </param>
/// <returns>
/// <see langword="true"/> if the face contains a codepoint for the specified glyph id; otherwise, <see langword="false"/>.
/// </returns>
internal abstract bool TryGetCodePoint(ushort glyphId, out CodePoint codePoint);

/// <summary>
/// Tries to get the glyph class for a given glyph id.
/// The font needs to have a GDEF table defined.
Expand Down
61 changes: 55 additions & 6 deletions src/SixLabors.Fonts/GlyphPositioningCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,24 +79,48 @@ public void DisableShapingFeature(int index, Tag feature)
/// </summary>
/// <param name="offset">The zero-based index within the input codepoint collection.</param>
/// <param name="pointSize">The font size in PT units of the font containing this glyph.</param>
/// <param name="isSubstituted">Whether the glyph is the result of a substitution.</param>
/// <param name="isVerticalSubstitution">Whether the glyph is the result of a vertical substitution.</param>
/// <param name="isDecomposed">Whether the glyph is the result of a decomposition substitution.</param>
/// <param name="metrics">
/// When this method returns, contains the glyph metrics associated with the specified offset,
/// if the value is found; otherwise, the default value for the type of the metrics parameter.
/// This parameter is passed uninitialized.
/// </param>
/// <returns>The metrics.</returns>
public bool TryGetGlyphMetricsAtOffset(int offset, out float pointSize, out bool isDecomposed, [NotNullWhen(true)] out IReadOnlyList<GlyphMetrics>? metrics)
public bool TryGetGlyphMetricsAtOffset(
int offset,
out float pointSize,
out bool isSubstituted,
out bool isVerticalSubstitution,
out bool isDecomposed,
[NotNullWhen(true)] out IReadOnlyList<GlyphMetrics>? metrics)
{
List<GlyphMetrics> match = new();
pointSize = 0;
isSubstituted = false;
isVerticalSubstitution = false;
isDecomposed = false;

Tag vert = FeatureTags.VerticalAlternates;
Copy link
Preview

Copilot AI Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for checking vertical substitutions using FeatureTags.VerticalAlternates, FeatureTags.VerticalAlternatesAndRotation, and FeatureTags.VerticalAlternatesForRotation is duplicated in multiple places. This logic should be refactored into a separate method to avoid duplication and potential inconsistencies.

Suggested change
Tag vert = FeatureTags.VerticalAlternates;
isVerticalSubstitution = CheckVerticalSubstitution(glyph.Data.AppliedFeatures);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic is applied within a loop with other tests. Cannot abstract.

Tag vrt2 = FeatureTags.VerticalAlternatesAndRotation;
Tag vrtr = FeatureTags.VerticalAlternatesForRotation;

for (int i = 0; i < this.glyphs.Count; i++)
{
if (this.glyphs[i].Offset == offset)
{
GlyphPositioningData glyph = this.glyphs[i];
isSubstituted = glyph.Data.IsSubstituted;
isDecomposed = glyph.Data.IsDecomposed;

foreach (Tag feature in glyph.Data.AppliedFeatures)
{
isVerticalSubstitution |= feature == vert;
isVerticalSubstitution |= feature == vrt2;
isVerticalSubstitution |= feature == vrtr;
}

pointSize = glyph.PointSize;
match.AddRange(glyph.Metrics);
}
Expand Down Expand Up @@ -147,7 +171,7 @@ public bool TryUpdate(Font font, GlyphSubstitutionCollection collection)

// Perform a semi-deep clone (FontMetrics is not cloned) so we can continue to
// cache the original in the font metrics and only update our collection.
var metrics = new List<GlyphMetrics>(data.Count);
List<GlyphMetrics> metrics = new(data.Count);
TextAttributes textAttributes = shape.TextRun.TextAttributes;
TextDecorations textDecorations = shape.TextRun.TextDecorations;
foreach (GlyphMetrics gm in fontMetrics.GetGlyphMetrics(codePoint, id, textAttributes, textDecorations, layoutMode, colorFontSupport))
Expand Down Expand Up @@ -216,6 +240,10 @@ public bool TryAdd(Font font, GlyphSubstitutionCollection collection)
LayoutMode layoutMode = this.TextOptions.LayoutMode;
ColorFontSupport colorFontSupport = this.TextOptions.ColorFontSupport;

Tag vert = FeatureTags.VerticalAlternates;
Tag vrt2 = FeatureTags.VerticalAlternatesAndRotation;
Tag vrtr = FeatureTags.VerticalAlternatesForRotation;

for (int i = 0; i < collection.Count; i++)
{
GlyphShapingData data = collection.GetGlyphShapingData(i, out int offset);
Expand All @@ -227,7 +255,14 @@ public bool TryAdd(Font font, GlyphSubstitutionCollection collection)
// cache the original in the font metrics and only update our collection.
TextAttributes textAttributes = data.TextRun.TextAttributes;
TextDecorations textDecorations = data.TextRun.TextDecorations;
bool isVerticalLayout = AdvancedTypographicUtils.IsVerticalGlyph(codePoint, layoutMode);

bool isVertical = AdvancedTypographicUtils.IsVerticalGlyph(codePoint, layoutMode);
foreach (Tag feature in data.AppliedFeatures)
{
isVertical |= feature == vert;
isVertical |= feature == vrt2;
isVertical |= feature == vrtr;
}

foreach (GlyphMetrics gm in fontMetrics.GetGlyphMetrics(codePoint, id, textAttributes, textDecorations, layoutMode, colorFontSupport))
{
Expand All @@ -242,7 +277,7 @@ public bool TryAdd(Font font, GlyphSubstitutionCollection collection)
if (metrics.Count > 0)
{
GlyphMetrics[] gm = metrics.ToArray();
if (isVerticalLayout)
if (isVertical)
{
this.glyphs.Add(new(offset, new(data, true) { Bounds = new(0, 0, 0, gm[0].AdvanceHeight) }, font.Size, gm));
}
Expand Down Expand Up @@ -302,11 +337,25 @@ public void UpdatePosition(FontMetrics fontMetrics, int index)
public void Advance(FontMetrics fontMetrics, int index, ushort glyphId, short dx, short dy)
{
LayoutMode layoutMode = this.TextOptions.LayoutMode;
foreach (GlyphMetrics m in this.glyphs[index].Metrics)
Tag vert = FeatureTags.VerticalAlternates;
Tag vrt2 = FeatureTags.VerticalAlternatesAndRotation;
Tag vrtr = FeatureTags.VerticalAlternatesForRotation;

GlyphPositioningData glyph = this.glyphs[index];
foreach (GlyphMetrics m in glyph.Metrics)
{
if (m.GlyphId == glyphId && fontMetrics == m.FontMetrics)
{
m.ApplyAdvance(dx, AdvancedTypographicUtils.IsVerticalGlyph(m.CodePoint, layoutMode) ? dy : (short)0);
bool isVertical = AdvancedTypographicUtils.IsVerticalGlyph(m.CodePoint, layoutMode);

foreach (Tag feature in glyph.Data.AppliedFeatures)
{
isVertical |= feature == vert;
isVertical |= feature == vrt2;
isVertical |= feature == vrtr;
}

m.ApplyAdvance(dx, isVertical ? dy : (short)0);
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions src/SixLabors.Fonts/GlyphShapingData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public GlyphShapingData(GlyphShapingData data, bool clearFeatures = false)
this.LigatureComponent = data.LigatureComponent;
this.MarkAttachment = data.MarkAttachment;
this.CursiveAttachment = data.CursiveAttachment;
this.IsSubstituted = data.IsSubstituted;
this.IsDecomposed = data.IsDecomposed;
if (data.UniversalShapingEngineInfo != null)
{
Expand All @@ -57,9 +58,11 @@ public GlyphShapingData(GlyphShapingData data, bool clearFeatures = false)

if (!clearFeatures)
{
this.Features = new(data.Features);
this.Features.AddRange(data.Features);
}

this.AppliedFeatures.AddRange(data.AppliedFeatures);

this.Bounds = data.Bounds;
}

Expand Down Expand Up @@ -116,7 +119,12 @@ public GlyphShapingData(GlyphShapingData data, bool clearFeatures = false)
/// <summary>
/// Gets or sets the collection of features.
/// </summary>
public List<TagEntry> Features { get; set; } = new List<TagEntry>();
public List<TagEntry> Features { get; set; } = new();

/// <summary>
/// Gets or sets the collection of applied features.
/// </summary>
public List<Tag> AppliedFeatures { get; set; } = new();

/// <summary>
/// Gets or sets the shaping bounds.
Expand Down
19 changes: 14 additions & 5 deletions src/SixLabors.Fonts/GlyphSubstitutionCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,8 @@ public bool TryGetGlyphShapingDataAtOffset(int offset, [NotNullWhen(true)] out I
/// </summary>
/// <param name="index">The zero-based index of the element to replace.</param>
/// <param name="glyphId">The replacement glyph id.</param>
public void Replace(int index, ushort glyphId)
/// <param name="feature">The feature to apply to the glyph at the specified index.</param>
public void Replace(int index, ushort glyphId, Tag feature)
{
GlyphShapingData current = this.glyphs[index].Data;
current.GlyphId = glyphId;
Expand All @@ -234,6 +235,7 @@ public void Replace(int index, ushort glyphId)
current.MarkAttachment = -1;
current.CursiveAttachment = -1;
current.IsSubstituted = true;
current.AppliedFeatures.Add(feature);
}

/// <summary>
Expand All @@ -243,7 +245,8 @@ public void Replace(int index, ushort glyphId)
/// <param name="removalIndices">The indices at which to remove elements.</param>
/// <param name="glyphId">The replacement glyph id.</param>
/// <param name="ligatureId">The ligature id.</param>
public void Replace(int index, ReadOnlySpan<int> removalIndices, ushort glyphId, int ligatureId)
/// <param name="feature">The feature to apply to the glyph at the specified index.</param>
public void Replace(int index, ReadOnlySpan<int> removalIndices, ushort glyphId, int ligatureId, Tag feature)
{
// Remove the glyphs at each index.
int codePointCount = 0;
Expand Down Expand Up @@ -279,6 +282,7 @@ public void Replace(int index, ReadOnlySpan<int> removalIndices, ushort glyphId,
current.MarkAttachment = -1;
current.CursiveAttachment = -1;
current.IsSubstituted = true;
current.AppliedFeatures.Add(feature);
}

/// <summary>
Expand All @@ -287,7 +291,8 @@ public void Replace(int index, ReadOnlySpan<int> removalIndices, ushort glyphId,
/// <param name="index">The zero-based index of the element to replace.</param>
/// <param name="count">The number of glyphs to remove.</param>
/// <param name="glyphId">The replacement glyph id.</param>
public void Replace(int index, int count, ushort glyphId)
/// <param name="feature">The feature to apply to the glyph at the specified index.</param>
public void Replace(int index, int count, ushort glyphId, Tag feature)
{
// Remove the glyphs at each index.
int codePointCount = 0;
Expand Down Expand Up @@ -322,14 +327,16 @@ public void Replace(int index, int count, ushort glyphId)
current.MarkAttachment = -1;
current.CursiveAttachment = -1;
current.IsSubstituted = true;
current.AppliedFeatures.Add(feature);
}

/// <summary>
/// Replaces a single glyph id with a collection of glyph ids.
/// </summary>
/// <param name="index">The zero-based index of the element to replace.</param>
/// <param name="glyphIds">The collection of replacement glyph ids.</param>
public void Replace(int index, ReadOnlySpan<ushort> glyphIds)
/// <param name="feature">The feature to apply to the glyph at the specified index.</param>
public void Replace(int index, ReadOnlySpan<ushort> glyphIds, Tag feature)
{
if (glyphIds.Length > 0)
{
Expand All @@ -345,7 +352,7 @@ public void Replace(int index, ReadOnlySpan<ushort> glyphIds)
// Add additional glyphs from the rest of the sequence.
if (glyphIds.Length > 1)
{
glyphIds = glyphIds.Slice(1);
glyphIds = glyphIds[1..];
for (int i = 0; i < glyphIds.Length; i++)
{
GlyphShapingData data = new(current, false)
Expand All @@ -354,6 +361,8 @@ public void Replace(int index, ReadOnlySpan<ushort> glyphIds)
LigatureComponent = i + 1
};

data.AppliedFeatures.Add(feature);

this.glyphs.Insert(++index, new(pair.Offset, data));
}
}
Expand Down
23 changes: 23 additions & 0 deletions src/SixLabors.Fonts/StreamFontMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ internal partial class StreamFontMetrics : FontMetrics
private readonly ConcurrentDictionary<(int CodePoint, ushort Id, TextAttributes Attributes, bool IsVerticalLayout), GlyphMetrics[]> glyphCache;
private readonly ConcurrentDictionary<(int CodePoint, ushort Id, TextAttributes Attributes, bool IsVerticalLayout), GlyphMetrics[]>? colorGlyphCache;
private readonly ConcurrentDictionary<(int CodePoint, int NextCodePoint), (bool Success, ushort GlyphId, bool SkipNextCodePoint)> glyphIdCache;
private readonly ConcurrentDictionary<ushort, (bool Success, CodePoint CodePoint)> codePointCache;
private readonly FontDescription description;
private readonly HorizontalMetrics horizontalMetrics;
private readonly VerticalMetrics verticalMetrics;
Expand Down Expand Up @@ -61,6 +62,7 @@ internal StreamFontMetrics(TrueTypeFontTables tables)
this.outlineType = OutlineType.TrueType;
this.description = new FontDescription(tables.Name, tables.Os2, tables.Head);
this.glyphIdCache = new();
this.codePointCache = new();
this.glyphCache = new();
if (tables.Colr is not null)
{
Expand All @@ -82,6 +84,7 @@ internal StreamFontMetrics(CompactFontTables tables)
this.outlineType = OutlineType.CFF;
this.description = new FontDescription(tables.Name, tables.Os2, tables.Head);
this.glyphIdCache = new();
this.codePointCache = new();
this.glyphCache = new();
if (tables.Colr is not null)
{
Expand Down Expand Up @@ -174,6 +177,26 @@ internal override bool TryGetGlyphId(CodePoint codePoint, CodePoint? nextCodePoi
return success;
}

/// <inheritdoc/>
internal override bool TryGetCodePoint(ushort glyphId, out CodePoint codePoint)
{
CMapTable cmap = this.outlineType == OutlineType.TrueType
? this.trueTypeFontTables!.Cmap
: this.compactFontTables!.Cmap;

(bool success, CodePoint value) = this.codePointCache.GetOrAdd(
glyphId,
static (glyphId, arg) =>
{
bool success = arg.TryGetCodePoint(glyphId, out CodePoint codePoint);
return (success, codePoint);
},
cmap);

codePoint = value;
return success;
}

/// <inheritdoc/>
internal override bool TryGetGlyphClass(ushort glyphId, [NotNullWhen(true)] out GlyphClassDef? glyphClass)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public override bool TrySubstitution(

if (this.coverageTable.CoverageIndexOf(glyphId) > -1)
{
collection.Replace(index, (ushort)(glyphId + this.deltaGlyphId));
collection.Replace(index, (ushort)(glyphId + this.deltaGlyphId), feature);
return true;
}

Expand Down Expand Up @@ -135,7 +135,7 @@ public override bool TrySubstitution(

if (offset > -1)
{
collection.Replace(index, this.substituteGlyphs[offset]);
collection.Replace(index, this.substituteGlyphs[offset], feature);
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public override bool TrySubstitution(

if (offset > -1)
{
collection.Replace(index, this.sequenceTables[offset].SubstituteGlyphs);
collection.Replace(index, this.sequenceTables[offset].SubstituteGlyphs, feature);
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public override bool TrySubstitution(
// TODO: We're just choosing the first alternative here.
// It looks like the choice is arbitrary and should be determined by
// the client.
collection.Replace(index, this.alternateSetTables[offset].AlternateGlyphs[0]);
collection.Replace(index, this.alternateSetTables[offset].AlternateGlyphs[0], feature);
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ public override bool TrySubstitution(
}

// Delete the matched glyphs, and replace the current glyph with the ligature glyph
collection.Replace(index, matches, ligatureTable.GlyphId, ligatureId);
collection.Replace(index, matches, ligatureTable.GlyphId, ligatureId, feature);
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public override bool TrySubstitution(
bool hasChanged = false;
for (int i = 0; i < this.substituteGlyphIds.Length; i++)
{
collection.Replace(index + i, this.substituteGlyphIds[i]);
collection.Replace(index + i, this.substituteGlyphIds[i], feature);
hasChanged = true;
}

Expand Down
Loading
Loading