fonttools / skia-pathops

Python bindings for the Skia library's Path Ops
https://skia.org/docs/dev/present/pathops/
BSD 3-Clause "New" or "Revised" License
47 stars 14 forks source link

contour direction is not correct #10

Open typemytype opened 6 years ago

typemytype commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

anthrotype commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

khaledhosny commented 6 years ago

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

adrientetar commented 6 years ago

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 commented 6 years ago

in TruFont, you can do

from pathops import *
glyph = CurrentGlyph()
contours = list(glyph)
glyph.clearContours()
union(contours, glyph.getPen())
anthrotype commented 6 years ago

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 commented 6 years ago

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

anthrotype commented 6 years ago

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 commented 6 years ago

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

anthrotype commented 6 years ago

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

behdad commented 6 years ago

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 commented 6 years ago

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

typemytype commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Have we got back to Cary on this?

adrientetar commented 6 years ago

No, someone ought to do it.

adrientetar commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

anthrotype commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

typemytype commented 6 years ago

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

anthrotype commented 6 years ago

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