Issues with RendererOptions

[antonfirsov]

• The type name is confusing in ImageSharp.Drawing context. There are many many *Options types across our libraries, most of them indicating what is the type about. Here it's unclear at the first glance what are we rendering.
• Some properties are get only and initialized from the constructor while others are settable.
• We have a TextOptions type there that duplicates a subset of the functionality which is confusing. Upstream issue: Issues with TextOptions and Fonts.RendererOptions ImageSharp.Drawing#174

[JimBobSquarePants]

YES!!!!

We need to have a good look at this. They're tightly coupled in a way that makes updates breaking and was a nightmare during development of the Bidi code. I'd like a single type that comes from Fonts.

[antonfirsov]

From what I see, RenderOptions is a superset, and additional values are passed as parameters of DrawTextProcessor:

Fonts.RendererOptions	TextOptions
Font
TabWidth	TabWidth
ApplyKerning	ApplyKerning
DpiX	DpiX
DpiY	DpiY
FallbackFontFamilies	FallbackFonts
WrappingWidth	WrapTextWidth
WordBreaking	WordBreaking
LineSpacing	LineSpacing
TextDirection
TextAlignment
HorizontalAlignment	HorizontalAlignment
VerticalAlignment	VerticalAlignment
LayoutMode
Origin
ColorFontSupport	RenderColorFonts

Not sure about what is the right design to consolidate them. @tocsoft thoughts?

[JimBobSquarePants]

Some properties are get only and initialized from the constructor while others are settable.

I think this is due to design constraints caused by nullable reference types. We want to enforce that the Font property is not null when leaving the constructor. I can't think of a better way to workaround that but I'd only pass that single property.

Regarding consolidation. The best answer IMO is to use a single type TextOptions which replaces RendererOptions in the Fonts library. Drawing can use that as it's merely a subset with no extra properties.

For our processing overloads we simply create an instance from the passed parameters.

[antonfirsov]

The best answer IMO is to use a single type

The main challenge with this is that would be confusing if we pass a Font both in DrawText(...) parameters and in the options object, we should avoid that redundancy.

One solution would be to remove Font from the options object, and pass it around separately in Fonts methods just like we do in Drawing. I'm not sure though if it's only Font or there are other properties? What about TextDirection, TextAlignment, LayoutMode and Origin?

[JimBobSquarePants]

We currently pass Font, Dpi(XY), and Origin via various constructor overloads.

Origin can be passed separately via a TextOptions instance. It'll be Vector2.Zero 99% of the time. I'm playing about with tis just now.

Ripping Font out could complicate some of the existing APIs both public and internal e.g. TextMeasurer, TextLayout.

I'm not 100% convinced TextOptions should be stored in DrawingOptions. I get that we want a set of global properties we can share but we're muddying the waters somewhat by passing around a set of default fallback fonts in that type. Text isn't something you generally fire and forget a bunch of setting for either - If you're rendering multiple text blocks you generally have different settings.

We only use two other properties from DrawingOptions. Transform and GraphicsOptions.

A prototype replacement would look something like this. Since the constructor allows simple default assignment plus all the properties are mutable people could maintain their own default instances and clone via the constructor updating only the properties they require.

/// <summary>
/// Provides configuration options for rendering text.
/// </summary>
public sealed class TextOptions
{
    private const float DefaultDpi = 72F;
    private float tabWidth = 4F;
    private float dpi = DefaultDpi;
    private float lineSpacing = 1F;

    /// <summary>
    /// Initializes a new instance of the <see cref="TextOptions"/> class.
    /// </summary>
    /// <param name="font">The font.</param>
    public TextOptions(Font font)
        : this(font, DefaultDpi)
    {
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="TextOptions"/> class.
    /// </summary>
    /// <param name="font">The font to render.</param>
    /// <param name="dpi">The DPI (Dots Per Inch) to render/measure the text at.</param>
    public TextOptions(Font font, float dpi)
    {
        this.Font = font;
        this.Dpi = dpi;
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="TextOptions"/> class from properties
    /// copied from the given instance.
    /// </summary>
    /// <param name="options">The options whose properties are copied into this instance.</param>
    public TextOptions(TextOptions options)
    {
        this.Font = options.Font;
        foreach (FontFamily family in options.FallbackFontFamilies)
        {
            this.FallbackFontFamilies.Add(family);
        }

        this.TabWidth = options.TabWidth;
        this.ApplyHinting = options.ApplyHinting;
        this.Dpi = options.Dpi;
        this.LineSpacing = options.LineSpacing;
        this.Origin = options.Origin;
        this.WrappingWidth = options.WrappingWidth;
        this.WordBreaking = options.WordBreaking;
        this.TextDirection = options.TextDirection;
        this.TextAlignment = options.TextAlignment;
        this.HorizontalAlignment = options.HorizontalAlignment;
        this.VerticalAlignment = options.VerticalAlignment;
        this.LayoutMode = options.LayoutMode;
        this.KerningMode = options.KerningMode;
        this.ColorFontSupport = options.ColorFontSupport;
    }

    /// <summary>
    /// Gets or sets the font.
    /// </summary>
    public Font Font { get; set; }

    /// <summary>
    /// Gets or sets the collection of fallback font families to use when
    /// a specific glyph is missing from <see cref="Font"/>.
    /// </summary>
    public ICollection<FontFamily> FallbackFontFamilies { get; set; } = new HashSet<FontFamily>();

    /// <summary>
    /// Gets or sets the DPI (Dots Per Inch) to render/measure the text at.
    /// <para/>
    /// Defaults to 72.
    /// </summary>
    public float Dpi
    {
        get => this.dpi;

        set
        {
            Guard.MustBeGreaterThanOrEqualTo(value, 0, nameof(this.dpi));
            this.dpi = value;
        }
    }

    /// <summary>
    /// Gets or sets the width of the tab. Measured as the distance in spaces.
    /// <para/>
    /// Defaults to 4.
    /// </summary>
    public float TabWidth
    {
        get => this.tabWidth;

        set
        {
            Guard.MustBeGreaterThanOrEqualTo(value, 0, nameof(this.TabWidth));
            this.tabWidth = value;
        }
    }

    /// <summary>
    /// Gets or sets a value indicating whether to apply hinting - The use of mathematical instructions
    /// to adjust the display of an outline font so that it lines up with a rasterized grid.
    /// </summary>
    public bool ApplyHinting { get; set; }

    /// <summary>
    /// Gets or sets the line spacing. Applied as a multiple of the line height.
    /// <para/>
    /// Defaults to 1.
    /// </summary>
    public float LineSpacing
    {
        get => this.lineSpacing;

        set
        {
            Guard.IsTrue(value != 0, nameof(this.LineSpacing), "Value must not be equal to 0.");
            this.lineSpacing = value;
        }
    }

    /// <summary>
    /// Gets or sets the rendering origin.
    /// </summary>
    public Vector2 Origin { get; set; } = Vector2.Zero;

    /// <summary>
    /// Gets or sets the width relative to the current DPI at which text will automatically wrap onto a newline.
    /// </summary>
    /// <remarks>
    /// If value is -1 then wrapping is disabled.
    /// </remarks>
    public float WrappingWidth { get; set; } = -1F;

    /// <summary>
    /// Gets or sets the word breaking mode to use when wrapping text.
    /// </summary>
    public WordBreaking WordBreaking { get; set; }

    /// <summary>
    /// Gets or sets the text direction.
    /// </summary>
    public TextDirection TextDirection { get; set; } = TextDirection.Auto;

    /// <summary>
    /// Gets or sets the text alignment of the text within the box.
    /// </summary>
    public TextAlignment TextAlignment { get; set; }

    /// <summary>
    /// Gets or sets the horizontal alignment of the text box.
    /// </summary>
    public HorizontalAlignment HorizontalAlignment { get; set; }

    /// <summary>
    /// Gets or sets the vertical alignment of the text box.
    /// </summary>
    public VerticalAlignment VerticalAlignment { get; set; }

    /// <summary>
    /// Gets or sets the layout mode for the text lines.
    /// </summary>
    public LayoutMode LayoutMode { get; set; }

    /// <summary>
    /// Gets or sets the kerning mode indicating whether to apply kerning (character spacing adjustments)
    /// to the glyph positions from information found within the font.
    /// </summary>
    public KerningMode KerningMode { get; set; }

    /// <summary>
    /// Gets or sets a value indicating whether to enable various color font formats.
    /// </summary>
    public ColorFontSupport ColorFontSupport { get; set; } = ColorFontSupport.MicrosoftColrFormat;
}

[antonfirsov]

I'm not 100% convinced TextOptions should be stored in DrawingOptions. I get that we want a set of global properties we can share but we're muddying the waters somewhat by passing around a set of default fallback fonts in that type. Text isn't something you generally fire and forget a bunch of setting for either - If you're rendering multiple text blocks you generally have different settings.

It depends on the application. Users rendering complex drawings are used to canvas-style API-s, where they can save as much information as possible into canvas "globals" and shorten the local calls. TextOptions is not different from this perspective. For example you may want to set a global font, just like you set a global foreground color. The problem is that our drawing API-s are halfway between being canvas-style and "pass-everything as args".

[JimBobSquarePants]

TextOptions is not different from this perspective

Here I disagree. There's a fundamental different in the options classes carried in DrawingOptions.

GraphicsOptions and ShapeOptions contain properties that affect all drawing operations. (Though ShapeOptions only contains a single property - IntersectionRule, perhaps we should hoist it? )

TextOptions affects text drawing only with all the rules directly responsible for controlling the renderer and nothing else.

EDIT. I have an experimental build running locally and it's much nicer to use. No more complex nested settings.