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

iup.c behaves differently from iup.py #3282

Closed
behdad opened this issue Sep 28, 2023 · 4 comments
Closed

iup.c behaves differently from iup.py #3282

behdad opened this issue Sep 28, 2023 · 4 comments

Comments

@behdad
Copy link
Member

behdad commented Sep 28, 2023

Just building Oswald font:

fontmake -o variable --output-dir Oswald.glyphs --drop-implied-oncurves --no-production-names --keep-direction OswaldFont/sources/Oswald.glyphs

produces different gvar table if varLib.iup module is Cython-compiled vs if it's not. We think the Python version is correct as it matches the Rust port.

@behdad
Copy link
Member Author

behdad commented Sep 28, 2023

If I comment out the following line (tolerance annotation in can_iup_in_between), then the .c behavior matches .py:

diff --git a/Lib/fontTools/varLib/iup.py b/Lib/fontTools/varLib/iup.py
index 0b7dcb7e9..4673a155e 100644
--- a/Lib/fontTools/varLib/iup.py
+++ b/Lib/fontTools/varLib/iup.py
@@ -167,7 +167,7 @@ def iup_delta(
 @cython.locals(
     i=cython.int,
     j=cython.int,
-    tolerance=cython.double,
+    # tolerance=cython.double,
     x=cython.double,
     y=cython.double,
     p=cython.double,

@behdad
Copy link
Member Author

behdad commented Sep 28, 2023

If I rewrite the generator as a for loop, that also makes the difference go away:

-    return all(
-        abs(complex(x - p, y - q)) <= tolerance
-        for (x, y), (p, q) in zip(deltas, interp)
-    )
+    for (x, y), (p, q) in zip(deltas, interp):
+        if not (abs(complex(x - p, y - q)) <= tolerance):
+            return False
+    return True

The .c code for the generator is suspect...

@behdad
Copy link
Member Author

behdad commented Sep 28, 2023

So does this:

diff --git a/Lib/fontTools/varLib/iup.py b/Lib/fontTools/varLib/iup.py
index 0b7dcb7e9..a393319f2 100644
--- a/Lib/fontTools/varLib/iup.py
+++ b/Lib/fontTools/varLib/iup.py
@@ -189,8 +189,9 @@ def can_iup_in_between(
     interp = iup_segment(coords[i + 1 : j], coords[i], deltas[i], coords[j], deltas[j])
     deltas = deltas[i + 1 : j]
 
+    tolerance2 = tolerance
     return all(
-        abs(complex(x - p, y - q)) <= tolerance
+        abs(complex(x - p, y - q)) <= tolerance2
         for (x, y), (p, q) in zip(deltas, interp)
     )
 

@behdad
Copy link
Member Author

behdad commented Sep 28, 2023

So does using a list comprehension:

diff --git a/Lib/fontTools/varLib/iup.py b/Lib/fontTools/varLib/iup.py
index 0b7dcb7e9..a0344bf83 100644
--- a/Lib/fontTools/varLib/iup.py
+++ b/Lib/fontTools/varLib/iup.py
@@ -189,10 +189,10 @@ def can_iup_in_between(
     interp = iup_segment(coords[i + 1 : j], coords[i], deltas[i], coords[j], deltas[j])
     deltas = deltas[i + 1 : j]
 
-    return all(
+    return all([
         abs(complex(x - p, y - q)) <= tolerance
         for (x, y), (p, q) in zip(deltas, interp)
-    )
+    ])
 
 
 @cython.locals(

Not the same, but maybe related: cython/cython#3928

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

1 participant