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

[Discussion in progress] Round all the calculations to whole pixels #52

Closed
wants to merge 2 commits into from

Conversation

ForNeVeR
Copy link
Owner

@ForNeVeR ForNeVeR commented Feb 25, 2017

While investigating #50 I've found that we have small rounding issues everywhere. To fix them, I had to introduce rounding in every Draw method.

Obviously we could add a smarter solution, but let's discuss that in principle at first.

The outcome is that the formulae are not so blurry. Check out these screenshots:
Before: image

After: image

Unfortunately, now we have small troubles (on some of the scale levels) when connecting part of square root tail with the rest of the radical line. Honestly said, earlier we had small issues there, but now we have huge noticeable gap. The following examples were taken with Scale = 21.0:

Before: image

After: image

That last issue is cleanly a regression, so I'm not sure if we want to merge this PR as-is. Let's discuss! @torfranz, I welcome you to take a look, too.

@ForNeVeR ForNeVeR self-assigned this Feb 25, 2017
@alexreg
Copy link
Collaborator

alexreg commented Feb 26, 2017

Hmm. I can see there was already a minute issue before these modifications, like you say. Did you figure out the cause of this (roughly speaking, even)?

@alexreg
Copy link
Collaborator

alexreg commented Feb 26, 2017

I have no idea how to solve this problem elegantly right now (perhaps the hour is to blame!)... but I'm not a great fan of the current solution, to be honest. Did you consider any other approaches?

@ForNeVeR
Copy link
Owner Author

@alexreg thanks, that was a good question. I've struggled a bit around SVG rendering code, and found that the SVG image rendered by our code had no flaws. So, I've tried to replace the code in VerticalBox.Draw with a call to RenderGeometry:

    internal class VerticalBox : Box
    {
        // ...
        public override void Draw(DrawingContext drawingContext, double scale, double x, double y)
        {
            base.Draw(drawingContext, scale, x, y);

            var g = new GeometryGroup();
            RenderGeometry(g, scale, x, y);
            drawingContext.DrawGeometry(Brushes.Black, new Pen(), g);
        }
        // ...
    }

And here's what I got:
image

The radical line is now absolutely straight in any scale, but

  1. the vertical line in the radical sign have noticeable gaps between its composable parts
  2. the font antialiasing was completely disabled, because WPF now renders actual geometry instead of font characters (although I'm not sure that's a bad thing; the result without ClearType actually looks a bit clearer to me)

I think that's a viable solution, but we need more work on it:

  1. fix these gaps on the vertical line
  2. try to scope the change only to the radical sign and surrounding lines (everything else should stay as-is)

We need also think more about switching the whole render to DrawGeometry instead of DrawGlyphRun. Probably that will be the best solution.

@alexreg
Copy link
Collaborator

alexreg commented Feb 26, 2017

Ahh, those damned gaps in the line — I actually remember them from my testing 6 years ago!

I think that turning off ClearType would perhaps be a good idea, since TeX is not designer to work with it.

@alexreg
Copy link
Collaborator

alexreg commented Feb 26, 2017

Just had a little play around, and no idea what's causing those gaps! If I couldn't solve it back then... do have a go, and let me know how you get on though.

@torfranz
Copy link

Hi,
I tried the 2 branches 50-blur and 50-blur-alternate against the master and for my particular case 50-blur was the best option so far. My formulas are rather simple compared to the example formula so I can't really tell what will be best going forward. Maybe some RenderPath property as long as there is no best universal solution? Thanks for all your effort so far, I see this will probably not be easily solved.

@ForNeVeR ForNeVeR mentioned this pull request Mar 3, 2017
@ForNeVeR
Copy link
Owner Author

ForNeVeR commented Mar 3, 2017

Idea from #66: compare our fonts with the ones from LaTeXMath — probably the guys there have already solved the issue.

@alexreg
Copy link
Collaborator

alexreg commented Mar 4, 2017

Yep. If using their fonts doesn't work, and there are no obvious tricks in their code, perhaps we can even point them to our issue, and see if they recognise the problem from their past work.

@gsomix
Copy link
Collaborator

gsomix commented Mar 4, 2017

Latest 50-blur-alternate branch.

big

small

@ForNeVeR
Copy link
Owner Author

ForNeVeR commented Mar 4, 2017

As we ascertained with @gsomix, now that branch looks nice, but still blurred at some of the scale levels. Probably we'll need to merge it with (at least some of) the changes in bugfix/50-blur.

@alexreg
Copy link
Collaborator

alexreg commented Mar 5, 2017

Very good. Yeah, I'll let you test that out. If we could be clear about what changes we're making to reduce blurriness (and make the minimal number possible), then that would be great. Would be happy to review a PR then.

@ForNeVeR
Copy link
Owner Author

ForNeVeR commented Mar 5, 2017

Yep. We're slowly making progress in this issue, it's not yet ready for review and merging.

@ForNeVeR
Copy link
Owner Author

I'm closing the PR and deleting the branches bugfix/50-blur and bugfix/50-blur-alternate, because issue #50 was solved by #71.

@ForNeVeR ForNeVeR closed this Mar 12, 2017
@ForNeVeR ForNeVeR deleted the bugfix/50-blur branch March 12, 2017 03:51
@alexreg
Copy link
Collaborator

alexreg commented Mar 12, 2017

Small issue, can probably be committed directly: change default formula in example to:

L = \int_a^b \sqrt[4]{ \left| \sum_{i,j=1}^ng_{ij}\left(\gamma(t)\right) \left[\frac{d}{dt}x^i\circ\gamma(t) \right] \left{\frac{d}{dt}x^j\circ\gamma(t) \right} \right|}dt

@ForNeVeR
Copy link
Owner Author

ForNeVeR commented Mar 12, 2017

@alexreg, done that!

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

Successfully merging this pull request may close these issues.

4 participants