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

contour direction is not correct #10

Open
typemytype opened this issue Nov 13, 2017 · 34 comments
Open

contour direction is not correct #10

typemytype opened this issue Nov 13, 2017 · 34 comments
Assignees

Comments

@typemytype
Copy link

The first drawing the source, second the result through skia, the third with booleanOperations

screen shot 2017-11-13 at 13 16 00

<?xml version="1.0" encoding="UTF-8"?>
<glyph name="B" format="2">
  <outline>
    <contour>
      <point x="375" y="24" type="curve" smooth="yes"/>
      <point x="567" y="24"/>
      <point x="723" y="180"/>
      <point x="723" y="372" type="curve" smooth="yes"/>
      <point x="723" y="563"/>
      <point x="567" y="719"/>
      <point x="375" y="719" type="curve" smooth="yes"/>
      <point x="184" y="719"/>
      <point x="28" y="563"/>
      <point x="28" y="372" type="curve" smooth="yes"/>
      <point x="28" y="180"/>
      <point x="184" y="24"/>
    </contour>
    <contour>
      <point x="375" y="147" type="curve" smooth="yes"/>
      <point x="252" y="147"/>
      <point x="151" y="248"/>
      <point x="151" y="372" type="curve" smooth="yes"/>
      <point x="151" y="495"/>
      <point x="252" y="595"/>
      <point x="375" y="595" type="curve" smooth="yes"/>
      <point x="499" y="595"/>
      <point x="599" y="495"/>
      <point x="599" y="372" type="curve" smooth="yes"/>
      <point x="599" y="248"/>
      <point x="499" y="147"/>
    </contour>
    <contour>
      <point x="175" y="180" type="line"/>
      <point x="232" y="120" type="line"/>
      <point x="386" y="226" type="line"/>
      <point x="805" y="-96" type="line"/>
      <point x="918" y="-22" type="line"/>
      <point x="379" y="332" type="line"/>
    </contour>
  </outline>
</glyph>
@anthrotype
Copy link
Member

anthrotype commented Nov 13, 2017

yeah, the handling of contour direction in the resulting path sometimes is completely wrong, I still haven't figured out why. I have to say I've lost most of my initial enthusiasm.
I'm not looking forward to fix skia bugs.

@khaledhosny
Copy link
Collaborator

@anthrotype If you think Skia is not suitable for our needs, we better kill this experiment now and start over with something else.

@anthrotype
Copy link
Member

I don't know, Khaled. Maybe we are using the API incorrectly... I need help from you guys debugging this.
I can expose more SkPath methods to Python to help debugging, but I don't know if I have the ability/time to dig deeper in the C++ implementation.

@adrientetar
Copy link
Member

It's fine, we just need to shift the start point to the next on curve and reverse each contour in the resulting path.

@anthrotype
Copy link
Member

We can also try reproducing the issue using this interactive page https://fiddle.skia.org/c/@pathops
you can write some C++ code that uses skia and see the rendering live.
If we can repro then we can file a bug and hope they fix it upstream.

@adrientetar
Copy link
Member

It's not a bug: https://groups.google.com/d/msg/skia-discuss/9xieDIhMoxk/XbfbmHOTAAAJ

But the transform to what we want is (afaict) deterministic.

@anthrotype
Copy link
Member

so if skia cannot give us a result path with nonzero winding fill type, but always returns a path with even-odd fill type, then we would need a reliable way to change it back to non-zero winding...

@adrientetar
Copy link
Member

Actually yeah, with the example of the OP it doesn't work, one contour needs not be reverse.

@khaledhosny
Copy link
Collaborator

Can someone give me a python snippet that I can test with?

@adrientetar
Copy link
Member

Anyway yeah assuming it's a valid even odd contour (which I assume it is, for proper rendering) there must be an algorithm to change the contour according to the other fill rule.

@anthrotype
Copy link
Member

in TruFont, you can do

from pathops import *
glyph = CurrentGlyph()
contours = list(glyph)
glyph.clearContours()
union(contours, glyph.getPen())

@anthrotype
Copy link
Member

anthrotype commented Nov 13, 2017

note that the union function is internally calling the binary Op function with a UNION operator and the second operand being another empty Path.

You could alternatively try using a OpBuilder instance (wrapper for SkOpBuilder.cpp), call add methods on it, and then finally the resolve method, to see if anything changes.

And I noticed sometimes one gets it right, while the other doesn't. I still haven't understood the difference between the two though.

@adrientetar
Copy link
Member

Seems converting from even odd to winding might be quite complex. @behdad thoughts?

@anthrotype
Copy link
Member

also, in the functions defined in operations.py module, we are accumulating multiple contours in a single Path, and pass that to Op function as a whole.

We could alternatively try calling builder.add for as many contours in a glyph and see if that changes the result. In my tests, it didn't help much (some inner contours would even disappear altogether!)

@khaledhosny
Copy link
Collaborator

Interestingly, Skia’s canvas is able to draw the glyph correctly after the union operation https://fiddle.skia.org/c/4873ecb267c61ff616953a3b3887ad9e

@anthrotype
Copy link
Member

@khaledhosny yeah, because it's probably filling them with even-odd rule.

@behdad
Copy link
Member

behdad commented Nov 13, 2017

Seems converting from even odd to winding might be quite complex. @behdad thoughts?

Not more complex than doing it with outlines with overlaps! We should ask if Skia has that already. If not, something along https://github.com/behdad/glyphy/blob/master/src/glyphy-outline.cc#L268

@typemytype
Copy link
Author

its seems like fix_winding is solving lots of issues...

@typemytype
Copy link
Author

but still finding issues

screen shot 2017-11-14 at 10 15 54

(and some other I cannot share publicly without asking permission first)

@adrientetar
Copy link
Member

I think FixWinding is written for single contour paths (it's called by the builder on final paths being finally merging them), which is why it can have weird results for multiple contours.

@anthrotype
Copy link
Member

The core developer of Skia, Cary Clark is willing to help but he first want us to write down the requirements:
https://groups.google.com/d/msg/skia-discuss/9xieDIhMoxk/NlQq-AfQBAAJ

we need to reply to him

@typemytype
Copy link
Author

I can list where booleanOperations interferes with the contours and how it handles different situation. what is the structure of such a requirements? meaning how to write that down?

@anthrotype
Copy link
Member

anthrotype commented Nov 15, 2017

not sure. he mentions writing up some sample code using http://fiddle.skia.org/ (so it's easy to visualize and we can just share a link) with the desired outcome.
he also would like to see how we're using the API.
Maybe it'd be easier to just arrange a hangout with him? @behdad

@adrientetar
Copy link
Member

The requirements: to preserve contour direction and start points as much as possible (when the contours are modified and when they aren't).

Also we ought to provide sample results. @typemytype Do you have other boolean ops inputs you can share? The Q in the first post is a good one.

We should have inputs and the results of booleanOperations in SkPath form, i.e. make a path and then call path.dump(). That will print the c++ repr (like I did here).

@behdad
Copy link
Member

behdad commented Dec 10, 2017

Have we got back to Cary on this?

@adrientetar
Copy link
Member

No, someone ought to do it.

@adrientetar
Copy link
Member

As an update, I responded in the thread a while ago and bungeman@ pointed to this skia bug about having pathops output proper winding paths (assigned to Cary Clark).

@anthrotype anthrotype self-assigned this Aug 2, 2018
@anthrotype anthrotype reopened this Aug 8, 2018
@anthrotype
Copy link
Member

@typemytype @adrientetar @khaledhosny can you retry with the latest skia-pathops (v0.1.3) and see if it fixes the issue with winding direction?
There should be wheels available from PyPI shortly (if all the builds complete).

@adrientetar
Copy link
Member

Yes it seems correct now, nice work! I guess the only remaining issue now is the jumping start points.

@anthrotype
Copy link
Member

Yes it seems correct now, nice work!

Yay!

I guess the only remaining issue now is the jumping start points

I'll work on fixing #9 now

@anthrotype
Copy link
Member

https://bugs.chromium.org/p/skia/issues/detail?id=7682

they just added a new SkPathOps::AsWinding function to convert SkPath from even odd fill to non-zero winding. I'll see if it fixes this bug and if so I'll remove my own winding_from_even_odd function.

anthrotype added a commit that referenced this issue Aug 17, 2018
…dd to non-zero winding

was added in https://bugs.chromium.org/p/skia/issues/detail?id=7682

Should provide a better fix for #10

The old logic is still kept in case the new AsWinding function returns False (failure), but
probably should not be needed.
@anthrotype anthrotype reopened this Aug 17, 2018
@anthrotype
Copy link
Member

I just enabled the new AsWinding function, it seems to work however the feature seems unfinished. They seem to have forgotten a stray debug print statement so now we get loads of "incomplete!"...
I may revert for now.

https://bugs.chromium.org/p/skia/issues/detail?id=7682#c6

anthrotype added a commit that referenced this issue Aug 17, 2018
there's an annoying SkDebugf statement left that prints lots of incomplete!
warnings. Let's revert for now.
#10 (comment)
@typemytype
Copy link
Author

I dont know why, but pathops slowed down a lot... (more correct but a lot slower)

@anthrotype
Copy link
Member

I didn’t notice that. Since what version? How much slower? I’ll take a look after I’m back in September.

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

No branches or pull requests

5 participants