Add Points Of Inflection / Balance / Harmonize #4900
Conversation
|
Awesome. FTR, Here are binary artifacts for this pull already: |
|
Fantastic, excellent work. Finally my decision to make MFEK's point type FontForge-similar (with Thanks a lot also for using |
|
Oh, and by the way, some code review as far as this project goes— ① Why did you add the options to the MetricsView? Seems to clutter menus and not make much sense, can you rationalize it? https://github.com/fontforge/fontforge/pull/4900/files#diff-7dcb402a6783e5b71a16f0e5edf1387845b40574aaeea9faa90c172ee7832c05 |
|
@ctrlcctrlv I will implement your suggestions 1,3,4,5 as soon as I have an answer to your question 2. |
|
I think there's an argument to be made that Add Points of Inflection should appear everywhere that Add Extrema appears, in contrast with Harmonize and Balance. The former are similar in that they're more about normalization/avoiding potential tool problems than about output geometry. So I would recommend leaving Add Points of Inflection in those places even if/when the other two are removed. I'm not aware of any guidelines against adding hotkeys to new "core" menu items. User-chosen hotkeys override core hotkeys (I believe), so people using them for something else shouldn't be affected. |
JFTR, Is there a way to fully customize FontForge hotkeys? |
|
@Symbian9 Yes, this should work: https://fontforge.org/docs/ui/misc/HotKeys.html#hotkeys-hotkeyassign |
…thods, restructured the corresponding doc
|
@ctrlcctrlv |
|
I'm gonna wait to review this until #4789 is in and this branch is rebased. |
|
@linusromer Now that #4789 is merged can you rebase this off the current master. I think there are a bunch of common commits and doing that will make reviewing easier. |
|
I've started reviewing this and so far the new code looks good, however, when reviewing with all commits it looks like some of #4789 was reverted. Could you take a look and either figure out how I or github is confused or fix the issue? |
…c cases are marked with comments
…aks between '}' and 'else', added better behaviour for conversion from tangent to smooth
…r behaviour for conversion from hvcurve to curve
|
@skef Github is confused because of me. I think I have somehow messed up the commits of my "master" and "inflections" branches. But now I think I have solved the issue by cherry-picking the corresponding commits of my master branch. (In retrospective I probably should have done a rebase first to make things clearer.) |
fontforge/fontviewbase.c
Outdated
| @@ -929,6 +929,108 @@ void FVAddExtrema(FontViewBase *fv, int force_adding ) { | |||
| ff_progress_end_indicator(); | |||
| } | |||
|
|
|||
| void FVAddInflections(FontViewBase *fv, int anysel) { | |||
There was a problem hiding this comment.
I diffed these and they're identical except for the (reused) strings and the function call. The code base has that sort of thing but in this case I think its worth adding the string and function pointer to the signature of one of these and having the others be one-liners.
There was a problem hiding this comment.
Your suggested generalized solution is surely preferable. I will implement it as soon as I have a little bit more time. This affects FVHarmonize and FVBalance as well.
There was a problem hiding this comment.
@skef I am a little stuck here. I have tried your suggestion with a generalized function
void _FVElementAction(FontViewBase *fv, int anysel, void (*func)(SplineChar*, SplineSet*, enum ae_type, int), const char* progressmsg) that calls func(sc, sc->layers[layer].splines, anysel, emsize). However my three functions SplineCharAddInflections(), SplineCharBalance() and SplineCharHarmonize() only take 3 parameters, whereas SplineCharAddExtrema() takes one additional parameter emsize. I think there is no elegant solution of optional parameters in C. There is of course a cheap solution: void _FVElementAction(FontViewBase *fv, int anysel, const char* functionname) that contains a switch (functionname). Is there something I am obviously missing?
There was a problem hiding this comment.
I was just looking at the three added functions. Don't worry about the more ambitious project of including SplineCharAddExtrema().What makes these worth doing is the identical signature of the inner function.
fontforge/python.c
Outdated
| @@ -3468,6 +3468,51 @@ return( NULL ); | |||
| Py_RETURN( self ); | |||
| } | |||
|
|
|||
| static PyObject *PyFFContour_AddInflections(PyFF_Contour *self) { | |||
fontforge/python.c
Outdated
| @@ -4424,6 +4475,51 @@ return( NULL ); | |||
| Py_RETURN( self ); | |||
| } | |||
|
|
|||
| static PyObject *PyFFLayer_AddInflections(PyFF_Layer *self) { | |||
fontforge/python.c
Outdated
| @@ -9395,6 +9497,30 @@ return( NULL ); | |||
| Py_RETURN( self ); | |||
| } | |||
|
|
|||
| static PyObject *PyFFGlyph_AddInflections(PyFF_Glyph *self) { | |||
There was a problem hiding this comment.
These are short so ambivalent here.
|
|
||
| /* Make the line between the control points parallel to the chord */ | ||
| /* such that the area is preserved (kind of an improved "tunnify") */ | ||
| Spline *SplineBalance(Spline *s) { |
There was a problem hiding this comment.
You're (obviously -- this is the point) modifying SplinePoint values in this function but I don't see calls to SplineRefigure() anywhere in the path. (Above that's taken care of by SplineSplit().)
There was a problem hiding this comment.
Indeed, SplineRefigure() is not called ( I just have checked it) although it probably should be called. I HAVE tested my PR and I did not run into problems so far. But my tests were not more systematic than testing some problematic cases and run the functions on some huge fonts. I did not implement a separate torture test for the UI functions. Is there an existing torture test that I have missed (like trip and trap by Donald E. Knuth)?
Anyway, I will add SplineRefigure() calls as soon as I have a little bit more time.
There was a problem hiding this comment.
There is absolutely no good systematic method of UI testing, unfortunately. When one is adding or making significant modifications to UI-tied functionality it's best to just play around with it in the UI to see what happens. Making real-time changes in a CharView is probably the best way as its most likely to turn up updating problems (such as those caused by a lack of SplineRefigure().
|
|
||
| /* Make the splines adjacent to the SplinePoint sp G2-continuous */ | ||
| /* by moving sp between its adjacent control points */ | ||
| void SplinePointHarmonize(SplinePoint *sp) { |
There was a problem hiding this comment.
Same here, no SplineRefigure()s to be seen.
…functions, added SplineRefigure() to the base functions of Balance and Harmonize
|
@skef I have abstracted all occuring AddInflections, Balance, Harmonize in python.c and in the FontView and CharView (the python font method for AddInflections excluded). And I have added |
|
This pull request introduces 9 alerts when merging 9b9f21b into dca27e2 - view on LGTM.com new alerts:
|
|
@skef After implementing the asked changes I have now tested it more extensively (all Python functions, FontView, CharView, MetricsView) with different fonts (also with quadratic layers). I think it should work now. |
|
|
||
| #, c-format | ||
| msgid "Include filename too long on line %d of %s" | ||
| msgstr "Include-Dateiname zu lang in Zeile %d in %s" | ||
| msgstr "Name der einzufügenden Datei zu lang in Zeile %d in %s" |
There was a problem hiding this comment.
This actually needs to be updated in crowdin, I'll find a way of getting it there
|
Are you talking about the "Edit tunni lines" feature of Fontlab 8? While I
probably could implement this feature as a raw algorithm to change a curve,
I see no easy way to implement a user interface to it.
Am Fr., 7. Apr. 2023 um 18:46 Uhr schrieb Sergej Lebedev <
***@***.***>:
… @linusromer <https://github.com/linusromer> I think "Add Points Of
Inflection / Balance / Harmonize" are very good functions. But it could be
extended by allowing you to control the curvature of a curve by entering
numbers. For example, you could make an arc in all glyphs, which can be
precisely defined with tunni lines. So one could get much more control over
the shapes. Like in Fontlab 8, you can define the curvature very precisely
by seeing numbers and entering them. Can you please programme this?
—
Reply to this email directly, view it on GitHub
<#4900 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACVRWCUPV7LPRKSJOW7BCQLXABAF3ANCNFSM5NEQ6JYA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@linusromer In FontLab, the tunni lines are given numbers. For example, for an arc shape 60 or 50. This allows you to determine how much an arc is curved. Whether it is flatter or rounder. This way you can create exactly the same round shapes for all letters. At the moment, FontForge does not have an exact way to easily control how much an arc is curved. |
|
@linusromer Smart Corners - It might be possible to implement a similar function in FontForge. |
|
The things you wish are clearly a matter of user interface. I am the wrong
person to implement this or other things that have to be displayed on the
canvas. Someone else might be the right person...
Sergej Lebedev ***@***.***> schrieb am Mo., 10. Apr. 2023,
10:34:
… @linusromer <https://github.com/linusromer> Smart Corners - It might be
possible to implement a similar function in FontForge.
https://help.fontlab.com/fontlab-vi/Using-Smart-Corners/
—
Reply to this email directly, view it on GitHub
<#4900 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACVRWCTCZEARTCQBLQBA7JTXAPAX7ANCNFSM5NEQ6JYA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@linusromer Maybe as a plugin? |
|
Horizontal and vertical alignment could be solved by a plugin similar to
github.com/linusromer/curvatura. However, I will not work on it (I am not
part of the FontForge team). You could write the plugin on your own. Before
programming you should think of the interface: Should only complete
contours be aligned or only selected nodes? If only complete contours
should be aligned how would the selection work? To which place should it be
aligned? What are the menu entries ("align horizontally" and "align
vertically" ?).
Am Mo., 10. Apr. 2023 um 13:46 Uhr schrieb Sergej Lebedev <
***@***.***>:
… @linusromer <https://github.com/linusromer> Maybe as a plugin?
It would also be nice to have a function in FontForge that allows you to
centre elements horizontally and vertically. Such alignment functions are a
matter of course in any graphics programme such as Illustrator or Affinity
Designer.
—
Reply to this email directly, view it on GitHub
<#4900 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACVRWCXRU3U76YW5ZQOUYELXAPXINANCNFSM5NEQ6JYA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I am a type designer, not a programmer, so I cannot program a plug-in. |
As intended I have implemented entries in the "Element Menu" named "Add Points Of Inflection", "Balance" and "Harmonize" for the font view, the char view and the metrics view. Corresponding scripting functions are included as well.
The translations to German are included. However, the translations to other languages will have to be done by others.
This makes my plug-in https://github.com/linusromer/curvatura more or less useless (because the "Harmonize handles" of curvature is probably not used by anybody).
Type of change