Skip to content

Conversation

Alatius
Copy link
Contributor

@Alatius Alatius commented Aug 30, 2022

Bug fix for problems discussed in #5093. My changes are described and explained there.

@ctrlcctrlv
Copy link
Member

@linusromer: Thoughts? :)

@linusromer
Copy link
Contributor

I have tested the new "merge" and "simplify" with some fonts and some special situations: I believe that 2a0b4a6 is a good idea - it may even speed up things slightly.
I am not that sure about 15d5ca9 but for me it seems to work and fix the bug. This may need a bit more testing with expanding strokes.

@Alatius
Copy link
Contributor Author

Alatius commented Sep 7, 2022

I am not that sure about [15d5ca9]

Is there anything in particular that makes you hesitate? The change to use to->prev when tlen==0 seems to me to be quite straight-forward. To the extent that I myself have doubts, it boils down to me not quite understanding under what circumstances the code block starting at line 752 is even supposed to be executed, i.e., I don't know how it can occur that from->next == NULL (or to->prev == NULL). Lacking input data to test on where this is the case, I have never seen this code being run, and thus I have been unable to test the effect. But in any case, I don't see how it would have any adverse effects (supposing that it is ever executed), compared to the result of the code before this commit.

@linusromer
Copy link
Contributor

Is there anything in particular that makes you hesitate?

No. Many things in FontForge depend on splinefit.c and sometimes it is hard to predict how it will affect things in FontForge (e.g. rounding problems of near zero values seem to have been quite a struggle). I would not have commented on this particular commit if not ctrlcctrlv had asked me about my thoughts. So I am just saying: I am not sure about this commit.

@ctrlcctrlv ctrlcctrlv merged commit f152f12 into fontforge:master Sep 13, 2022
@ctrlcctrlv
Copy link
Member

I've merged it as it also looks fine to me, but you are indeed right that it's hard to predict. I have a ton of fonts that use Expand Stroke though and none broke, sometimes the only way we can get feedback on it is to merge. Wouldn't be upset if it's reverted of course.

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

Successfully merging this pull request may close these issues.

3 participants