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

How to keep all control points when assigning layer to glyph? #5418

Closed
mf2vec-dev opened this issue May 10, 2024 · 8 comments
Closed

How to keep all control points when assigning layer to glyph? #5418

mf2vec-dev opened this issue May 10, 2024 · 8 comments

Comments

@mf2vec-dev
Copy link
Contributor

FontForge seems to remove control points between on-curve points that have the same coordinates when assigning a layer to a glyph.
Is there a way to disable the removal of these points?

Steps to reproduce

A simplified example showing this behavior:

import fontforge

c = fontforge.contour()
c.moveTo(0, 0)
c.cubicTo((0, 0), (0, 0), (0, 0))
c.closed = True

l = fontforge.layer()
l += c

print(l[0])

f = fontforge.font()
g = f.createChar(65)
g.layers[1] = l

print(g.layers[1][0])

Actual behavior

<Contour(cubic)
  (0,0) on
  (0,0) off
  (0,0) off
>
<Contour(cubic)
  (0,0) on
>

Expected behavior

<Contour(cubic)
  (0,0) on
  (0,0) off
  (0,0) off
>
<Contour(cubic)
  (0,0) on
  (0,0) off
  (0,0) off
>

FontForge version and OS

FontForge: 20230101 git:cce02b132c698b4568e7823abdd2f1b3daa32b5e
Ubuntu 22.04.4 LTS

Why this is a problem

I'm trying to create a variable font based on @ctrlcctrlv's tutorial. One of the properties that should be variable is the radius of the corners.

image

If the radius is greater than 0, the control points are obviously needed. If it is 0, there are two on-curve points at the corner (which is correct), but the two control points between them must be present for interpolation.

@mf2vec-dev
Copy link
Contributor Author

I was able to keep the control points by changing

sp->nonextcp = sp->noprevcp = true;

from true to false, and removing

from->nonextcp = from->nextcp.x==from->me.x && from->nextcp.y == from->me.y;
to->noprevcp = to->prevcp.x==to->me.x && to->prevcp.y == to->me.y;

@skef You last modified the two lines in splinerefigure.c, so you may have some background on this.
Is there anything against adding an option to disable control point removal? Is there any motivation (maybe problems in GUI, etc.?) to remove the control points other than they are not needed in most cases? The comment a few lines above motivates SplineRefigure3 because of small errors, but my understanding is that this is related to the RealNear checks a few lines below, not to the equality check in the lines in question.

@skef
Copy link
Contributor

skef commented May 18, 2024

The changes you're suggesting are large and fundamental and absolutely not motivated by your small use case, which I assume has to do with using FontForge for variable font development, something it's likely never going to be really designed for.

If you want to approach this problem in a way that has any likelihood of being accepted, you'll probably need to add a flag/mode to setLayer that means something like "trust me completely" and then implement the code to read in the Python layer object so that nothing changes. Given that you can't use the normal spline code such as SplineRemake3, that would be a not huge but still significant project.

@skef
Copy link
Contributor

skef commented May 18, 2024

In any case, the changes to management of nonextcp and noprevcp did not really have to do with RealNear, they addressed longstanding problems where one of the two would wind up being set to True and the other to False, and then you would have a Cubic spline with one control point, which is self-contradictory and would confuse all sorts of algorithms both inside and outside FontForge. The solution to that was to make the control point positions themselves the source of the values of those booleans. The direction you want to head would amount to removing support for lines from the cubic mode of FontForge. That's not going to happen. So the code would wind up needing to manage nonextcp and noprevcp separately again, and all those bugs that were resolved by that commit would come back.

It might simply be the case that FontForge isn't the right tool for the specific kind of font development you wish to pursue.

@skef
Copy link
Contributor

skef commented May 18, 2024

Let me approach this from a different perspective:

Why specifically are you trying to solve this problem, instead of dealing with the issue at a different point in your development "stack"? Over time I've looked at a number of variable font development platforms and what they generally worry about is whether there are the same number of splines on each corresponding contour, not the same number of points. Why can't the code that integrates your design instances (or "masters") say "oh, this is a line but this is a cubic, so I'll just convert the line into a cubic to match"?

If the answer is something like "then I can't use FontForge's interpolation code to preview a master", maybe that's what you should be looking to improve. One could, for example, either modify the interpolation code to be tolerant of the spline/line problem generally, or add a flag that makes it tolerant that way. And that could be beneficial to users in other situations.

@mf2vec-dev
Copy link
Contributor Author

First of all, thank you for taking the time to provide a thorough explanation and outline possible solution approaches.


The changes you're suggesting are large and fundamental[...]

OK, that's what I was afraid of.

[...] variable font development platforms [...] generally worry about [...] whether there are the same number of splines on each corresponding contour, not the same number of points.

I'm using fontmake as suggested in @ctrlcctrlv's tutorial. Unfortunately, it seems to be checking for the number of points.
(fontmake's compatibility check throws an error and complains Fonts had differing number of points in glyph ..., contour ...:)

"[...] so I'll just convert the line into a cubic to match"

While this would be possible, the control points of a cubic representing a line are only constrained to lie on the line segment. [p1, cp1, cp2, p2] = [(0, 0), (0, 0), (0, 0), (1, 0)], [(0, 0), (0, 0), (1, 0), (1, 0)], [(0, 0), (1/3, 0), (2/3, 0), (1, 0)], [(0, 0), (1/2, 0), (1/2, 0), (1, 0)] and [(0, 0), (1, 0), (1, 0), (1, 0)] are all valid cubic representations of the line [(0, 0), (1, 0)]. While the appearance of these line representations is the same, as soon as you interpolate, the positions of the control points become important. Therefore, always choosing one of these methods to place the control points is only a workaround, that the fontmake developers seem to have decided not to use.

"then I can't use FontForge's interpolation code[...]"

Since I'm utilizing fontmake, I'm not employing FontForge's interpolation functionalities.


a flag/mode to setLayer that means something like "trust me completely"

Of the different solutions you mentioned, implementing a separate mode seems to me to be the most promising. The routine should not make any changes to the geometry, it should just do some checks (e.g. if the number of control points is correct for the layer type).

Do you have a preference for the interface? Some ideas:

  • g.layers[1].set_without_cleanup(l)
  • fontforge.setPrefs('LayerCleanup', False) # default: True
    g.layers[1] = l

both instead of

g.layers[1] = l

whose behavior would not change.

@skef
Copy link
Contributor

skef commented May 19, 2024

While this would be possible, the control points of a cubic representing a line are only constrained to lie on the line segment. [p1, cp1, cp2, p2] = [(0, 0), (0, 0), (0, 0), (1, 0)], [(0, 0), (0, 0), (1, 0), (1, 0)], [(0, 0), (1/3, 0), (2/3, 0), (1, 0)], [(0, 0), (1/2, 0), (1/2, 0), (1, 0)] and [(0, 0), (1, 0), (1, 0), (1, 0)] are all valid cubic representations of the line [(0, 0), (1, 0)]. While the appearance of these line representations is the same, as soon as you interpolate, the positions of the control points become important. Therefore, always choosing one of these methods to place the control points is only a workaround, that the fontmake developers seem to have decided not to use.

That may be true but it's beside the point of our discussion, because (as far as I recall) splineRefigure3 and the other conversion code doesn't coerce cubic splines with merely colinear points into lines. The only condition FontForge is giving you trouble with is a spline where both control points are co-located with their associated on-curve points. And therefore if you have a line (or a degenerate "line" with both on-curve points co-located) in one case and a spline in another you know where the control points "are": they're co-located with their on-curve points. So there's no ambiguity in practice. The point isn't to special-case all cubic "lines", it's to special-case the case you're actually having trouble with.

I still think you would be better off putting some layer between FontForge and fontmake to do this for you rather than mucking around in FontForge code. And I'm not going to guarantee in advance that a PR along the lines you image will be accepted. But I won't stop you outright.

Anyway, stop uisng g.layers[?] = and start using g.setLayer(). The assignment interface is there for backward compatibility and doesn't give you any parametric control. And given that what you want is an internal representation of the spline that's exactly the same as the Python layer object representation, all you need is another mutually-exclusive flag name to represent what you want to add.

@skef
Copy link
Contributor

skef commented May 19, 2024

Looked into fontmake a bit. It seems like the relevant call is here in _build_interpolatable_masters():

    ):
        if ttf:
            ttf_curves = CurveConversion(ttf_curves)
            return ufo2ft.compileInterpolatableTTFsFromDS(
                designspace,
                useProductionNames=use_production_names,
                reverseDirection=reverse_direction,
                convertCubics=ttf_curves.convertCubics,
                allQuadratic=ttf_curves.allQuadratic,
                cubicConversionError=conversion_error,
                featureWriters=feature_writers,
                debugFeatureFile=debug_feature_file,
                feaIncludeDir=fea_include_dir,
                filters=filters,
                flattenComponents=flatten_components,
                autoUseMyMetrics=auto_use_my_metrics,
                inplace=True,
            )
        else:
            return ufo2ft.compileInterpolatableOTFsFromDS(
                designspace,
                useProductionNames=use_production_names,
                roundTolerance=cff_round_tolerance,
                featureWriters=feature_writers,
                debugFeatureFile=debug_feature_file,
                feaIncludeDir=fea_include_dir,
                filters=filters,
                inplace=True,
            )

So if you're building a CFF-based font it calls ufo2ft.compileInterpolatableOTFsFromDS. Note the filters argument. Those are defined in ufo2ft/filters, and the base.py method you overload in the BaseIFilter() class is:

    def filter(self, glyphName: str, glyphs: list) -> bool:
        """This is where the filter is applied to a set of interpolatable glyphs.

        Subclasses must override this method, and return True
        when the glyph was modified.
        """
        raise NotImplementedError

So that takes all the masters of a glyph and is intended to provide an opportunity to make changes. So all you would need here is something to read through the glyphs spline by spline (rather than point by point) and add the control points back if necessary (i.e. if some of the splines in other masters are cubics). You can limit this to when all the points on the spline are colocated if you like, or provide whatever limitation you find to be appropriate, so that other conditions will continue to error out on the compatibility check. Then you pass that filter as a parameter to fontmake and you've solved your problem.

@mf2vec-dev
Copy link
Contributor Author

I was able to implement a filter that does exactly what I want.

I was not aware of the possibility of using filters in fontmake. Thank you very much for pointing me in that direction.

If anyone comes across this issue in the future:
I'm using FontProject.run_from_designspace() which eventually passes the filters to ufo2ft.compileVariableTTFs() / ufo2ft.compileVariableCFF2s(). The filter property _pre must be set to True and the compatibility check must be disabled (run_from_designspace(..., check_compatibility=False)).

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

2 participants