skia-pathops icon indicating copy to clipboard operation
skia-pathops copied to clipboard

contour direction is not correct

Open typemytype opened this issue 7 years ago • 34 comments

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>

typemytype avatar Nov 13 '17 12:11 typemytype

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.

anthrotype avatar Nov 13 '17 12:11 anthrotype

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

khaledhosny avatar Nov 13 '17 16:11 khaledhosny

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.

anthrotype avatar Nov 13 '17 17:11 anthrotype

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

adrientetar avatar Nov 13 '17 17:11 adrientetar

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.

anthrotype avatar Nov 13 '17 17:11 anthrotype

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.

adrientetar avatar Nov 13 '17 17:11 adrientetar

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...

anthrotype avatar Nov 13 '17 17:11 anthrotype

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

adrientetar avatar Nov 13 '17 17:11 adrientetar

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

khaledhosny avatar Nov 13 '17 17:11 khaledhosny

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.

adrientetar avatar Nov 13 '17 17:11 adrientetar

in TruFont, you can do

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

anthrotype avatar Nov 13 '17 17:11 anthrotype

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.

anthrotype avatar Nov 13 '17 17:11 anthrotype

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

adrientetar avatar Nov 13 '17 17:11 adrientetar

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!)

anthrotype avatar Nov 13 '17 17:11 anthrotype

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

khaledhosny avatar Nov 13 '17 18:11 khaledhosny

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

anthrotype avatar Nov 13 '17 18:11 anthrotype

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

behdad avatar Nov 13 '17 19:11 behdad

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

typemytype avatar Nov 14 '17 08:11 typemytype

but still finding issues

screen shot 2017-11-14 at 10 15 54

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

typemytype avatar Nov 14 '17 09:11 typemytype

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.

adrientetar avatar Nov 14 '17 10:11 adrientetar

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

anthrotype avatar Nov 15 '17 11:11 anthrotype

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?

typemytype avatar Nov 15 '17 13:11 typemytype

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

anthrotype avatar Nov 15 '17 13:11 anthrotype

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).

adrientetar avatar Nov 16 '17 13:11 adrientetar

Have we got back to Cary on this?

behdad avatar Dec 10 '17 22:12 behdad

No, someone ought to do it.

adrientetar avatar Dec 10 '17 23:12 adrientetar

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).

adrientetar avatar May 01 '18 18:05 adrientetar

@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).

anthrotype avatar Aug 08 '18 19:08 anthrotype

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

adrientetar avatar Aug 08 '18 23:08 adrientetar

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 avatar Aug 09 '18 10:08 anthrotype