Fix moving Spiros between layers #3680
Conversation
This closes fontforge#3668. With this patch, Spiros can be cleanly copied between layers of different types («Q»uadratic vs «C»ubic).
|
When I was looking at this stuff earlier, I concluded that the Spiro layer was maintaining "Spiro data" and also converting that data to a cubic spline on each edit. That was presumably so that the UI could actually draw the curve, because the graphics layer doesn't speak Spiro. So am I correct that what this patch does is just suppress any conversion when in Spiro mode, so that both the Spiro data and the temporary cubic spline gets pasted? If that's correct, then that does seem like most of what is needed, but I'm still worried about the behavior in different cases. A font can have Quadratic or Cubic formatted splines. A character can be in Bezier or Spiro "mode", with associated data. Ideally all of these should work:
(These being all the combos except B -> B.) So, questions:
On the second question, I personally feel that when you copy data created in Spiro mode and paste it into a window in Bezier mode, it should switch the target window into Spiro mode. That doesn't seem to be what happens in the current code. Maybe that's because no one has thought about it, and maybe it's because other people didn't think it should work that way. However: if that change were made, it seems that this patch would address all the cases. Spiro data and the generated cubic spline would get copied, the window would wind up in Spiro mode, and then changing out of Spiro mode (if that was desired) would do the appropriate conversion. So those are my off-the-cuff thoughts on this. To summarize: we should test the various copy/paste combinations, and decide what relationship, if any, copying has to spiro/bezier mode. |
|
@skef I don't think you really understand the status quo ante. Copying a Spiro into a Bézier mode CharView shouldn't switch the mode...splines have Spiros associated with them when copied and pasted. Those associated Spiros are lost when the spline is edited as a Bézier, but you can do the following:
The bug was that the Spiro data was being lost. This prevents that from happening. |
|
By the way, I'm not saying it's good practice to mix Béziers and Spiros on the same layer. To the contrary, it's dumb and will likely lead to problems. But FontForge lets you do it. |
OK -- my intuition about what's more intuitive is different, but I don't put much weight on that. Anyway, let me boil down my more complex and nebulous question to a simpler one: What happens with your patch if you copy from a cubic font/character in Spiro mode to a quadratic font/character in Bezier mode? Does that work well? |
|
@skef It might very well be intuitive, but Spiros are destructive. In fact, FontForge is the only software I'm aware of that even tries to automatically convert Bézier curves to Spiros, and it often does so poorly, its Spiros are usually not even the same curve as the Bézier was. So because switching the mode might destroy the original curve, I understand why George makes you switch it manually, to say "I want this layer on this char to be Spiro mode", so you don't accidentally destroy the Bézier curve data.
It does indeed. I tested that before opening this PR. 👍 |
|
(To be clear, we're currently chatting about an issue not directly related to this push.) I don't understand how the destructiveness argues against my semantic. It seems like if you were working in Spiros in window A and copy the data into window B, wouldn't you usually still want to be working in Sprios in B? Unless there were already contents in B, I guess; then it isn't as clear. But in any event I don't think it's a big deal either way. |
|
@skef Right I'm talking about the case where there are already contents on that layer in B, it wouldn't be clear that Spiro mode should be enabled automatically. Maybe if there are no contents it should be…but it's never worked that way, and this is a bug fix, not really adding a new feature like that. |
| sc->layers[layer].splines = new; | ||
| if ( sc->layers[layer].splines != NULL && | ||
| sc->layers[layer].splines->spiro_cnt <= 0 ) { | ||
| new = SplineSetsTTFApprox(sc->layers[layer].splines); |
There was a problem hiding this comment.
I guess this isn't a very fair question, but have you dug around enough to identify why this works? It seems like this change amounts to "if there are Spiro points, don't convert". So when copying to a quadratic layer not in Spiro mode, what code winds up doing the "translation"?
There was a problem hiding this comment.
(I'm asking because ideally a reviewer should understand specifically why the change helps, so it would be faster for me to review if I had some pointers to the other relevant code.)
There was a problem hiding this comment.
It works because the translation isn't actually necessary for a Spiro spline; the spline is only necessary for display to the user, so it doesn't matter if FontForge internally keeps a cubic spline on a quadratic layer. Indeed, unless #3679 is merged, Spiro splines are saved as cubic, which is another bug entirely.
There was a problem hiding this comment.
When it's actually time to export the font, FontForge will throw away the Spiros and do the proper conversion anyway. So Spiro is a special case; and a bit of a buggy one.
There was a problem hiding this comment.
But my question relates to the case we've been discussing in the general comment thread: A user has two fonts open, and a CharView open for a glyph in each font. One window has a Cubic foreground but is being edited in Spiro mode. A user copies content in that window. The other window has a Quadratic foreground and is not being edited in Spiro mode. The user pastes into that window. For that case to work correctly, if the latter window remains in "Bezier mode", something needs to convert the spline to quadratic format. The code in this push request just prevents conversion in certain cases. So the case in question works, something else must do the conversion to quadratic/order2. I'm wondering what that is.
There was a problem hiding this comment.
I just tried it; it works. I wasn't doubting that, I was asking how it worked. This isn't really code I've looked at closely so I'll leave the review to others.
There was a problem hiding this comment.
@skef Because CVMouseDownPoint calls SplineSetSpirosClear. FontForge is good about clearing the Spiros on edits, I'm sure other places also call it. Just look for calls of SplineSetSpirosClear
There was a problem hiding this comment.
After clearing the Spiros, CVMouseDownPoint calls SplineMake, which takes order2 which comes from the layer info.
There was a problem hiding this comment.
@skef Sorry if I'm just not understanding you, sorry you no longer want to review this :(
There was a problem hiding this comment.
(If no one else has yet) I'll take a look at these pointers later and see if I can situate myself in the code enough to confidently review it.
|
Note to maintainers: Don't merge this yet, it causes a regression! This solves one bug yes, but by doing so creates another: it makes it so drawing a shape on a «Q»uadratic layer with a Spiro and Spiro mode enabled corrupts the Spiro data. I'm not sure why yet, but it seems to be because Don't merge this please until I figure out how to either make this not affect shape drawing, or patch shape drawing as well. However, #3679 is fine and as far as I know regression free. |
|
I finally got around to taking another look at this, but unfortunately I still can't review it properly. However, I may have found a problematic test case. I've been away from FontForge for a bit, but I have double-checked that this is with the program compiled from the current master plus @ctrlcctrlv's spiroQlayer branch: I opened two "New" fonts Untitled1 and Untitled2, and a CharView in each of those fonts I'll call C1 and C2. In C1 I switched into Spiro mode (still with Cubic). In C2 I switched into Quadratic mode (still with Bezier). Then I drew a shape in each: Then I did a So now I have mixed quadratic and cubic content in the same layer. If I move a point in the pasted contour I wind up with mixed quadratic and cubic content in the same contour: So if I have the relevant code it seems that this fix is at least incomplete as a fix of the "general problem area" of #3668. |
|
Wow, that's cool, I broke FontForge in a way I didn't even think possible! Mixed quadratic and cubic points in the same contour‽‽ What is the world coming to‽ Unfortunately I don't quite know how to fix it, code review is of course welcome. This shouldn't affect the validity of my other patch, #3679, desperately needed IMHO… |
|
#3679 seems well on track for the next release. There's just a review lull right now. As for this problem, it doesn't seem hard to address. Assume we're restricting the question of how to copy from a Spiro layer to some layer. (That is, we're ignoring the problem of copying from a Bezier layer into a Spiro layer.) We know that Spiro maintains parallel cubic Bezier content to draw the curve, so just copying both structures works when the target is Spiro, and also when the target is Cubic (because the Spiro content will wind up being thrown away). This is the "premise" of the code currently in your pull request. So all that needs to be added is 1) a check to see if the target is Quadratic and 2) the (by now familiar) code to translate the temporary cubic curve to a quadratic one in that case. Basically, make sure the curves are of the right type for the target at the time of copying. |
|
Just wanted to note that FreeType happily accepts both second-order and third-order contours in a single outline... |
|
Hey guys, voluntarily closing this as even though it fixes the issue it creates new issues. I'm not sure how to add those new checks @skef talked about, and am not really that interested in figuring it out at this time to be honest. Of course, anyone is free to use my patch or build on it and open a new PR. And perhaps some day I may reopen this, but don't count on it as I'm busy with other FF work. |
This closes #3668.
With this patch, Spiros can be cleanly copied between layers of
different types («Q»uadratic vs «C»ubic).