Open win98se opened 2 years ago
You posted two backtraces, one with math.ceil and the other not; which one is right?
In any case, the fix looks like it would be to wrap a call to int
around the division; would you submit a PR?
You posted two backtraces, one with math.ceil and the other not; which one is right?
The one without is the right one; The one with is from #30, which doesn't work here as well...
In any case, the fix looks like it would be to wrap a call to
int
around the division; would you submit a PR?
In my understanding, should it look like glyph.left_side_bearing += int(delta / 2)
? Even if so, it doesn't work too.
Even if so, it doesn't work too.
Same error?
Yes, same error, unfortunately.
Sorry to hear this. Unfortunately, I'm not going to have time to debug this in the short term. I hope someone else can chime in.
It seems like a problem with fontforge itself since setting glyph.left_side_bearing
to any float
raises TypeError
despite it being valid. Heck, since it's a float
when loaded from a font face, trying to set it as itself (glyph.left_side_bearing = glyph.left_side_bearing
) also raises TypeError
.
Can we report a bug to the Fontforge folks?
I'm still not sure why, but this fixed it for me:
glyph.left_side_bearing = int(glyph.left_side_bearing + delta / 2)
glyph.right_side_bearing = int(glyph.right_side_bearing + delta - glyph.left_side_bearing)
@PatienceAllergy that makes sense to me, though I suspect you'd want int(math.round(…))
instead. I was surprised that @win98se said it didn't work, but looking at the code again it makes sense that it did not work: glyph.left_side_bearing += int(delta / 2)
is the same as glyph.left_side_bearing = glyph.left_side_bearing + int(delta / 2)
, which is not right (it still stored a float). Your version casts to int after computing the new value, which is good.
Would you be OK with preparing a pull request, with the rounding added?
Also, it would be great if someone could make a bug report in the fontforge tracker.
Would you be OK with preparing a pull request, with the rounding added?
I'll give it a go. I've never done one before. So, as long as you're not in a hurry, I'll look at it tomorrow arvo (about 18hrs from now).
Would you be OK with preparing a pull request, with the rounding added?
Done! Well now I know how to do that. I also explicitly did a test font conversion to be 100% sure.
Also, it would be great if someone could make a bug report in the fontforge tracker.
I'm not sure I understand the problem well enough, was my explanation on the pull request right?
I was surprised that @win98se said it didn't work, but looking at the code again it makes sense that it did not work:
glyph.left_side_bearing += int(delta / 2)
is the same asglyph.left_side_bearing = glyph.left_side_bearing + int(delta / 2)
, which is not right (it still stored a float).
This is indeed my oversight, my apologies.
@PatienceAllergy Thank you for making a pull request at #34, your logics solved it; Also @cpitclaudel meant that we should create an issue at https://github.com/fontforge/fontforge/issues, but my wording is specifically bad - I hope you may help in creating one.
I'm not sure I understand the problem well enough, was my explanation on the pull request right?
In my opinion, yes.
Is there some reason why this hasnt been merged to master? It appears the pull request is fully functional and ready, no?
Command line:
./monospacifier.py --references ./sources/references/* --inputs ./sources/inputs/* --save-to ./fonts --copy-metrics --merge
Error log:
Seems to be related with #30.