Set noprevcp and nonextcp only in SplineRefigure* and SplinePointCreate

[skef]

The reasoning is in #4125. nonextcp and noprevcp are handy in a number of circumstances, mostly for special-casing lines, but it's bad to make them authoritative for cubic splines because you can really only have a full cublic spline or a line: There's no such thing as a cubic spline with one control point.

The solution is to set these flags based on the spline data in (both flavors of) SplineRefigure(). The remaining cases are isolated spline points and the ends of open contours, which are never passed to SplineRefigure(). It's OK for a control point to "dangle" in some cases (like when a following point is removed) but normally one should initialize ...cp location to me and set the no...cp flag to true. This is best achieved by allocating all spline points with SplinePointCreate() to set everything up well in the first place.

There's code in contrib/fonttools/acorn2sfd.c that doesn't allocate Spline or SplinePoint structs with chunkalloc(), so I just left that as-is. It may or may not cause problems.

The existing tests were helpful in turning up some problems, particularly with isolated TrueType points and point numbering. The fact that point numbers are used "substantially" strongly suggests that the TrueType paths come through this pretty well. The OpenType path issues are simpler but perhaps slightly less well tested, at least by those tests. I did some testing with SVGs and Glifs and such and that's all working now.

Closes #4125
Closes #2601
Closes #4327

[ctrlcctrlv]

Good news re: #4327. That was an important bug to fix. I can remove hacks from FRBAmericanCursive now~

[jtanx]

There are legitimate test failures in this by the way https://github.com/fontforge/fontforge/pull/4685/checks?check_run_id=2212087326

[skef]

OK, I'll take a look

[skef]

This was one of those "how was the test passing in the debug build?" type things. Just an allocation order problem.

[jtanx]

Is this meant to capture all references for where noprevcp/nonextcp is set outside these functions? Just doing a quick search, I see e.g. https://github.com/skef/fontforge/blob/f2ed275d9967ff37e51fd54bc6637edaa183606a/fontforge/scstyles.c#L5253

[skef]

My thought at the time was basically: this is already a huge update so don't clean up every single line.

Because these values are now conventionally set by SplineMake any instance where they're set and followed by a SplineMake call are harmless (as long as it's nonextcp of the first point and noprevcp of the second point).

If it's preferable to go through and eliminate all of them that's fine too.

[jtanx]

Great, well at least that aligns with my reading of what SplineMake/Refigure were doing.

I think it would be nice to eventually clean it up, as that makes it clearer that it's only canonically set in a few places, but I'm happy for it to either come in a later change or be bundled as part of this (at least given the size of this pr, it makes no difference to me whether or not it's added).

[skef]

I think for this we can put it off. There's so much potential cleanup in the source base and this is the kind of thing one can poke at when changing a file.

[jtanx]

For most of these, it's pretty clear to see that SplineMake/Refigure is called after, but for a few of these, I found it hard to prove that they were called (and more importantly called before noprevcp/nonextcp were used) - I've commented where I found this to be the case.

For at least a couple of these, I'm fairly sure that there are missing calls, in which case I've left further details.

[jtanx]

flag

[skef]

A general comment on these: If you allocate a new point and don't call SplineMake, or change an existing point and don't call SplineRefigure, and then put the point "back" into normal use, those are bugs. In the first case the points won't really be part of a common contour because its SplineMake that links them up. In the latter case you'll just get bad values because almost all calculations use the expanded coefficients.

For this one I don't see an issue. The case where SplineMake isn't called is when adding the first point to the SplineSet. Sure, the values are adjusted while nonextcp and noprevcp will retain the default false values from SplinePointCreate(), but the metaphysics of a dangling control point are unclear anyway. The worst that could happen is loosing the dangling coordinates on a round-trip, but I don't believe that's a problem with SFDs. (When round-tripping through external tools the result will be highly variable, again because of the unclear metaphysics.

[jtanx]

fixing a bug?

[skef]

Hmm. Doesn't seem like this should be right, though. I'll think about it.

[frank-trampe]

Either this is wrong or the untouched logic in the preceding psp logic is wrong.

[skef]

I think I just fat-fingered this. I don't remember making the change and analytically I don't see any reason for the change.

My guess is that at some point in editing I removed these lines (this and the one below) and then thought better of it, and when I put them back I got this one wrong.

Anyway, I put it back how it was.

[jtanx]

Changes look fine, just pending on that comment in splineoverlap.c

[jtanx]

lgtm but this needs a rebase

[skef]

rebased

[Finii]

This PR breaks @skef already when opening the font file

https://github.com/ryanoasis/nerd-fonts/blob/v2.2.2/src/unpatched-fonts/3270/Medium/3270Medium.otf

some outlines come out badly destroyed.

Bisected to commit 0f8fc11 (which is the first real change).
Opening with older FontForges is no problem.

The left side shows the same font as the right side, but the left side is after 0f8fc11, the right side before...

The commit is so distributed/unfocused touches many files, hard to see what goes wrong (for me).
Maybe you have an idea?

Note: The current 3270font release does not have this problem, maybe a peculiarity in the old font?

Edit: Correct font link, add bottom note

[skef]

Problem confirmed in current master. I'll try to take a look at this soon.

[skef]

The problem, at least, is showing up quite specifically: Each of the three affected glyphs has bad co-located control points on either side of the first point in a contour. Both sides should be lines (although in this font that doesn't prove much) and therefore both control points should (in FontForge) be co-located with the first point.

[Finii]

Thanks again for looking into it.
It seems the font itself is partially broken, although it renders ok usually. The current font version does not have that defect, so maybe the whole point is moot.

Maybe related, maybe not:

If you generate an otf from the opened font you get this warning:
MonotonicFindAlong: Ran out of intersections.

Iirc I get this error also in other instances, where I would not expect one, like 'removing a lot of glyphs from a font, but not changing anything else' or something. Nothing I can really pinpoint.

[skef]

It's not uncommon to run into fonts that have features or geometries that aren't technically wrong but that are "asking for it" from font tools or libraries. An angle that's off 90 degrees by a tiny amount can have unexpected results due to floating point calculation strangeness. It could be that the authors ran into some of these problems and made changes without anything being really "broken".

It's still good to fix the problems thrown up by these cases unless it's just too difficult.

[skef]

Although it has a .ttf extension the linked 3270Medium.ttf file contains a CFF table rather than a glyf table. However, Neither the .ttf nor the .otf current versions of the font (with glyf and CFF tables respectively) show the problem in FontForge.

The problem, when factoring out the subroutinization, reduces to this:

        <CharString name="m">
          70 -21 366 60 hstem
          57 60 121 61 121 60 vstem
          cntrmask 00111000
          592 165 rmoveto
          -293 208 rmoveto
          41 42 39 0 41 -41 0 -325 60 0 0 337 rlineto
          0 8 -3 7 -6 6 rrcurveto
          -59 59 rlineto
          -6 6 -7 3 -8 0 rrcurveto
          -64 hlineto
          -8 0 -7 -3 -6 -6 rrcurveto
          -38 -38 -36 39 rlineto
          -6 6 -7 3 -9 0 rrcurveto
          -62 hlineto
          -8 0 -7 -3 -6 -6 rrcurveto
          -9 -9 0 17 -61 0 0 -426 60 0 0 323 44 43 36 0 41 -42 0 -324 61 0 rlineto
          endchar
        </CharString>
      </CharStrings>

So, basically, there are two rmovetos in a row at the start of the path.

A lone moveto in a CFF CharString can have various roles during development, depending on the tool you're using, but they're not supposed to wind up in final output. So this is a bit more towards straightforward "bad output" than "asking for it".

What FontForge should do opening this file is an interesting question. There are basically two options: leave a lone "dot" at the first location or just pretend the first moveto isn't there (while still honoring the math of the "r" prefixes). It looks like the intention of the code is the latter, given that there's no dot with pre-20220308 releases.

I suspect what's happening here is that the code that deals with double-movetos is moving the location of the point but not updating the initial control point, because it didn't used to matter. I'll try to dig that up.

[Finii]

Thank you, great digging that out 👍