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

Overhaul Text Arrangement and Trimming #1556

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

VisualMelon
Copy link
Contributor

@VisualMelon VisualMelon commented May 10, 2020

Fixes #1531, #1559, and #1554

Checklist

  • I have included examples or tests
  • I have updated the change log (will do so if/when this is striped back and becomes a set of meaningful commits)
  • I am listed in the CONTRIBUTORS file
  • I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

Big Issues

  • API for ITextTrimmer is weak: the render context determines the type of trimming that is done, which may not be ideal

@oxyplot/admins

@VisualMelon
Copy link
Contributor Author

MaxSize support is probably pretty slow, and will be caught out by strings with combining characters (need to do some research on that), but gives pretty reasonable results. It could be performed more quickly by render contexts with access to glyph information (e.g. SkiaSharp), where cropping the line would be logarithmic (i.e. irrelevant) rather that log-linear.

WPF (the bottom-most line is drawn clipped)

image

Skia on WPF (the bottom-most line is skipped completely)

image

@VisualMelon
Copy link
Contributor Author

VisualMelon commented May 12, 2020

@Jonarw would you mind taking a gander at the changes in Skia, and let me know if this looks too abhorrent to you to be an acceptable change?

Having done the provisional implementation of maxSize, I think putting all this stuff in one place is a good idea, but it may be inappropriate to clip by text rather than pixels for e.g. SkiaSharp if it can do clipping like occurs with WPF. That might motivate adding additional functionality to ArrangeText so that it can arrange according to maxSize but not perform the clipping itself.

@Jonarw
Copy link
Member

Jonarw commented May 12, 2020

Generally I think the ArrangeText() function is a very good idea to make text rendering more consistent between the platforms. However I think we should think a bit more about what exactly this should do.

I am a bit sceptical if clipping on character level is really the way to go for implementing maxSize. I think this could be unreliable for certain character combinations, and performance will probably fairly bad (though this should not be our main concern).

I am more concerned that we might be trying to do something very complex that we don't actually really need (see also my comment in #1559) - after all, I haven't missed this feature on SkiaSharp so far.

@Jonarw
Copy link
Member

Jonarw commented May 12, 2020

I am also not too sure how about I feel about guessing the font metrics (ascent, descent, leading) by doing multiple text measurements. At the current state, the exact algorithm for text height measurement should be an implementation detail of the render context, which we shouldn't rely on. If we effectively move the responsibility for vertical text arrangement out of the render context and into the ArrangeText() function, we should also adapt the API accordingly to make the responsibilities clear. This could mean:

  • IRenderContext.MeasureText() explicitly only works for single lines of text and only measures the width
  • IRenderContext.DrawText() explicitly only works for single lines of text
  • IRenderContext offers some way to get the relevant font metrics (ascent, descent, leading) the ArrageText() function needs to do its job properly

@VisualMelon
Copy link
Contributor Author

VisualMelon commented May 12, 2020

@Jonarw yeah, it's pretty horrendous, and I think what you describe is actually close to how OxyPlot was designed: DrawText is even documented as not working with multi-line text.

To that end, I would suggest keeping the current API as it is (i.e. MeasureText measures whole texts, DrawText should support multi-line text), but add additional methods which provide what you describe (i.e. measure width of text, retrieve metrics for a given font/size/weight combination). It could be that these are not made available through IRenderContext - because axes and series and what-not shouldn't need them - but through another ITextMeasurer interface. That would avoid any breaking changes, and make it easy to push all the text-arranging stuff into it's own class. (This new class could provide methods for computing the multi-text widths/heights, so all the render context has to do is implement the basic features you describe).

@Jonarw
Copy link
Member

Jonarw commented May 12, 2020

You are right of course, from the perspective of Axes, Series etc. the IRenderContext interface should not change.
Or maybe it could change, and we replace the removed/changed APIs by extensions, similar to the RenderingExtensions.DrawClippedXXX() stuff?

I haven't completely wrapped my mind around this, I need to think about it some more...

@VisualMelon
Copy link
Contributor Author

Thanks for the comments on what must look a complete mess at the moment. I'll probably next implement your suggestions about abstracting out the 'basic' measurement stuff (so we don't have to infer metrics), and put the whole thing in its own class. Rough plan:

  • add ITextMeasurer interface with methods
    • GetFontMetrics(family, size, weight) returning ascender, descender, and leading (probably in a struct if I can be bothered to add another new file)
    • Measure(text, family, size, weight) returning the width of the text
  • add TextArranger class with members
    • ITextMeasurer Measurer (or would we rather pass it as a parameter as I have been doing so far? All DPI awareness is in the ITextMeasurer: should the RenderContext have to know about this?)
    • MeasureText(text, family, size, weight) retuning the size of the (potentially multiline) text as an OxySize according to jonarw's proposal in What is MeasureText #1551
    • ArrangeText(...) doing what it does now, but leaning on the ITextMeasurer to arrange (potentiall multiline) text however it is asked to do so
  • patch ImageSharp, SkiaSharp, and managed-only SVG RenderContexts as necessary

The additional interface will duly provide the information the managed only SvgRenderContext needs to do probably text aligning because it won't have to guess the descent anymore.

@VisualMelon VisualMelon changed the title Svg Text Alignment Overhaul Text Arrangement and Trimming May 13, 2020
@VisualMelon
Copy link
Contributor Author

VisualMelon commented May 13, 2020

Recent changes:

  • Moved ArrangeText into it's own class
  • Moved text-trimming into its own class which implements ellipsises
  • Broke Winforms somewhat (can't seem to get GetFontMetrics right, any input welcome)
  • Moved Baseline offset into ArrangeText, so the DrawText methods are even tidier than ever
  • Squashed everything to give the illusion that I know what I'm doing

Next tasks:

  • Fix winforms
  • Refit RenderContexts so that they lean on TextArranger.MeasureText and move the measuring logic into MeasureTextWidth (so they don't get into an infinite loop)

Any feedback (esspecially negative) welcome as always.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented May 14, 2020

Refitting done, so ImageRenderContext, SvgRenderContext, and SkiaRenderContext shouldn't change any more. @Jonarw all else aside, does the interface to TextArranger look half-sane?
We could go further and extract ITextWriter from the render contexts: this would make sense for the various SvgExporter overrides, as currently there is unpleasantness associated with using render contexts that don't do any rendering, and the TextArranger has for each render context realistically has to be created in the constructor.

Also refitted the TextTrimmer so it supports Unicode and adding additional tests. This of course has made it slower... but I don't think that should be a concern: it is rarely used, and would be providing a capability that is presently unavailable almost everywhere. I doubt it is perfect (i.e. there will be strings that it gives nonsense results on), but it should be fairly easy to extend by making the central regexes more complicated. Annoyingly we can't compile the regexes because we started .NET Standard 1.0; however, the architecture means that if someone really cared they can swap out the text trimmer for their own; whether this is a good architecture is another issue.

WinForms is still broken: either I'm having difficultly extracting the font metrics (i.e. Graphics.RenderContext.GetFontMetrics is broken), or DrawString is doing something unexpected.

@VisualMelon
Copy link
Contributor Author

Fixed WinForms.

I think that means everything is working as I understand it should. I've not StyleCop'd this, but I'm going to open for review because there is nothing else I'm intending to do at the moment.

@VisualMelon VisualMelon marked this pull request as ready for review May 14, 2020 10:13
@VisualMelon VisualMelon force-pushed the SvgVAlignAgain branch 2 times, most recently from 8a10280 to 1927d3d Compare May 14, 2020 15:09
@Jonarw
Copy link
Member

Jonarw commented May 14, 2020

Looks very promising!
I did not get into this too deep, but the general concept looks sensible to me. Over the weekend I will have a closer look.

Just for my understanding: This adapts ImageSharp, SkiaSharp, WinForms and Pdf, at the moment, right? So WPF would still have to be adjusted for this (but could also be its own issue and PR).

Also we should then add more sophisticated ITextTrimmers where possible (i.e. for SkiaSharp) - but again, this does not have to be part of this PR in my opinion.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented May 14, 2020

@Jonarw thanks. I'll eagerly await your feedback, and perhaps put my efforts into #1516

It also adapts the built-in SVG renderer, which now depends on an ITextMeasurer rather than a whole IRenderTarget, which is something that could be exploited in the future.

I've not refitted WPF yet because frankly I'm frightened of it, but I can do that work for this PR or it can be done later if @objorke approves. For the purposes of consistent vertical alignment it is probably a good idea, but it may still be that we leave the trimming to it (though as I've discussed elsewhere, trimming by character seems a much better strategy to me) which could complicate matters with multi-line text

Regards advanced trimmers: yep, that was pretty much what I had in mind. The performance so far is perfectly good in my opinion, but there is plenty of scope for improvement. I won't claim the current trimmer behaviour is perfect, but it should be fairly easy to adapt if we find difficulties, and the architecture means it should be easy to support users without regular releases.

@VisualMelon
Copy link
Contributor Author

Rebase against develop.
I think I probably broke the ImageSharp implementation in the rebase, so will try to test and fix that before going any further.
Changed #if NETSTANDARD2_0_OR_GREATER to #if NETSTANDARD2_1_OR_GREATER in SkiaRenderContext because I couldn't get it to build otherwise; not sure what is up with that.

@VisualMelon
Copy link
Contributor Author

Should idealy review https://www.unicode.org/reports/tr14/tr14-40.html and http://www.unicode.org/reports/tr14/tr14-22.html to try to conform to unicode standards, but I don't think this is essential for this PR given there was no support for anything prior and it will probably be very difficult to write something standard complaint (if anyone knows better, please do speak up), so we should take our time over it.

@VisualMelon
Copy link
Contributor Author

Not sure why the test is failing

@VisualMelon
Copy link
Contributor Author

Failing on the XKCD examples, something to do with font.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Aug 10, 2023

New code is crashing because it insists on measuring the text when rendering, which the old code didn't. Not immediately sure how to deal with this. Should we be throwing if the font isn't available, or should the drawing routines trap the exception and use a default? Certainly need to be able to detect when the font is missing so we can use a consistent default if we go that way.

Example stack trace:

  Failed ExportPngAndCompareWithBaseline(Test #1) [53 ms]
  Error Message:
   System.InvalidOperationException : No glyph typeface found
  Stack Trace:
     at OxyPlot.Wpf.CanvasRenderContext.GetFontMetrics(String fontFamily, Double fontSize, Double fontWeight) in /_/Source/OxyPlot.Wpf/CanvasRenderContext.cs:line 463
   at OxyPlot.Rendering.TextArranger.MeasureText(String text, String fontFamily, Double fontSize, Double fontWeight) in /_/Source/OxyPlot/Rendering/TextArranger.cs:line 197
   at OxyPlot.Rendering.TextArranger.ArrangeText(ScreenPoint p, String text, String fontFamily, Double fontSize, Double fontWeight, Double rotation, HorizontalAlignment horizontalAlignment, VerticalAlignment verticalAlignment, Nullable`1 maxSize, HorizontalAlignment targetHorizontalAlignment, TextVerticalAlignment targetVerticalAlignment, String[]& lines, ScreenPoint[]& linePositions) in /_/Source/OxyPlot/Rendering/TextArranger.cs:line 78
   at OxyPlot.Wpf.CanvasRenderContext.DrawText(ScreenPoint p, String text, OxyColor fill, String fontFamily, Double fontSize, Double fontWeight, Double rotation, HorizontalAlignment horizontalAlignment, VerticalAlignment verticalAlignment, Nullable`1 maxSize) in /_/Source/OxyPlot.Wpf/CanvasRenderContext.cs:line 344
   at OxyPlot.XkcdRenderingDecorator.DrawText(ScreenPoint p, String text, OxyColor fill, String fontFamily, Double fontSize, Double fontWeight, Double rotate, HorizontalAlignment halign, VerticalAlignment valign, Nullable`1 maxSize) in /_/Source/OxyPlot/Rendering/RenderContext/XkcdRenderingDecorator.cs:line 133
   at OxyPlot.PlotModel.RenderErrorMessage(IRenderContext rc, String title, String errorMessage, Double fontSize) in /_/Source/OxyPlot/PlotModel/PlotModel.Rendering.cs:line 155
   at OxyPlot.PlotModel.RenderOverride(IRenderContext rc, OxyRect rect) in /_/Source/OxyPlot/PlotModel/PlotModel.Rendering.cs:line 134
   at OxyPlot.PlotModel.OxyPlot.IPlotModel.Render(IRenderContext rc, OxyRect rect) in /_/Source/OxyPlot/PlotModel/PlotModel.Rendering.cs:line 34
   at OxyPlot.Wpf.PngExporter.ExportToBitmap(IPlotModel model) in /_/Source/OxyPlot.Wpf/PngExporter.cs:line 99
   at OxyPlot.Wpf.PngExporter.Export(IPlotModel model, Stream stream) in /_/Source/OxyPlot.Wpf/PngExporter.cs:line 73
   at OxyPlot.Wpf.PngExporter.Export(IPlotModel model, String fileName, Int32 width, Int32 height, Double resolution) in /_/Source/OxyPlot.Wpf/PngExporter.cs:line 62
   at OxyPlot.Wpf.Tests.ExampleLibraryTests.<ExportPngAndCompareWithBaseline>g__ExportAndCompareToBaseline|4_0(PlotModel model, String fileName, <>c__DisplayClass4_0& ) in /_/Source/OxyPlot.Wpf.Tests/ExampleLibraryTests.cs:line 52
   at OxyPlot.Wpf.Tests.ExampleLibraryTests.ExportPngAndCompareWithBaseline(ExampleInfo example) in /_/Source/OxyPlot.Wpf.Tests/ExampleLibraryTests.cs:line 64

@JimBobSquarePants
Copy link

Text layout always involves measuring so that line breaks can be handled.

@VisualMelon
Copy link
Contributor Author

Aye, I think it can't be avoided, but we need to decide how to handle a missing font when rendering: previously it would just be passed through to WPF and it would default to a different font. My preference at the moment would probably be to implement that behaviour explicitly, so we handle falling back to a different font, maybe allowing the user to configure if they want it to throw in this case or not.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Aug 11, 2023

Pushed a change so that CanvasRenderContext uses the default font if it can't get font metrics for the time being.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Aug 16, 2023

Rebased. @Jonarw do you have any opinion on the issue of default fonts? (see recent comments above and most recent commit)

@Jonarw
Copy link
Member

Jonarw commented Aug 16, 2023

I think falling back to the default font seems sensible. How do other renderers handle this? As there are no crashes in the examples I assume they also just fall back to the default?

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Feb 13, 2024

Rebased against master: @Jonarw given how slowly things are moving at the moment, I'd suggest we (squash) and merge this, and give ourselves time to find any remaining issues with the understanding that this isn't necessary a completely fixed API yet.

If this massive PR gets out of the way, I might get up the courage to start re-writing the Axis abstractions per #1640.

Copy link
Member

@Jonarw Jonarw left a comment

Choose a reason for hiding this comment

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

I am sorry for my prolonged absence, this came during a quite busy time for me, and then I just forgot about it.
I agree with you and would be happy to have this merged! I just found one property that I think is a probably a remnant of an earlier version of CanvasRenderContext?

/// </summary>
/// <value><c>true</c> if stream geometry should be used; otherwise, <c>false</c> .</value>
/// <remarks>The XamlWriter does not serialize StreamGeometry, so set this to <c>false</c> if you want to export to XAML. Using stream geometry seems to be slightly faster than using path geometry.</remarks>
public bool UseStreamGeometry { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is used anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve vertical text alignment of OxyPlot.SvgExporter
4 participants