ApproximateSplineFromPointsSlopesAccurate #4567
Conversation
Added an additional function ApproximateSplineFromPointsSlopesAccurate() in splinefit.c which has a boolean at the end. When the boolean is false, it behaves as before but if the boolean is true, it supplies a more accurate (but slower) approximation. The "Merge" function which lives inside splineutil2.c now uses the more accurate version. "Expand stroke" and "Simplify" do still use the original function.
|
If the big block gets restored to its old indentation I will approve this. I'll leave other stylistic choices to the discretion of the submitter and other reviewers (if any). Someone might quibble with adding |
|
@skef, I actually think in this case the spacing might not need to be reverted. Clearly this function was entirely changed by the commit, and it is unlikely that any bugs that arise from this would not be blamed on this commit. Furthermore, if it is found that the bugs go older than that, I don't think it's that big a deal to go back one commit because of the already mentioned fact that this commit changes entirely the behavior of the function. I don't know if that's enough to sway you, but I would be happy to merge this now if it is. It is an obviously useful feature to have a better merge. |
|
I would also say that because the entire function was reviewed by @linusromer and significantly changed, that it would be worse to have some of it spaced one way and some of it spaced another. If it was not such a large overhaul to the function I would have a different opinion. |
|
@ctrlcctrlv I don't agree that the function is "entirely changed" by the commit. There's an added section that corresponds to the new boolean. The only change to the rest of the function (as far as I can tell from verifying a fraction of them) is the indentation. That indentation change obscures that the rest of the function was syntactically unmodified by the commit. |
|
Yeah but how could adding extra indentation be avoided? There's a new else block so some kind of new indentation is needed isn't it? And indentation that sticks the way we usually do things would still create changed lines wouldn't it? |
|
Although it would be difficult to pick out specific PRs at this point, I take myself to be following a review convention I and other contributors have been informed of in similar cases. In general modifying very large blocks of existing code only for indentation has been discouraged. Maybe @frank-trampe can weigh in on whether this is still relevant.
It's C -- you avoid the extra indentation by not adding it. The project certainly doesn't use a style checker. The |
|
I don't know Skef. I don't think that we have ever required someone to not add indentation that they otherwise should have. I agree, @frank-trampe or @jtanx should weigh in. I think not adding the indentation would harm the readability of the function and that that matters more than the blame. |
the generalized function is now named with a underscore prefix. the indention is as required by skef. the spaces of "( is_accurate )" are now gone.
|
@linusromer GW's indentation style (still used in most of the files) is idiosyncratic and a pain to reproduce. I followed through on your intention in a different way and added a commit to your repository. Make sure you pull it down before editing any further (if you do). (If this is surprising, there's a little checkbox at submit time that gives project reviewers write access to the source branch of a PR and it's checked by default. Just a handy github thing.) Since the latest commit is mine I'll let you sign off on it before approving but the current code looks good to me. (The windows build failure looks unrelated but I'll follow up on that.) |
|
@jtanx Looks like the windows build is unhappy -- the last few commits on this have failed to build for windows in a way that doesn't seem tied to the source changes. |
|
Sorry for my silence on the indentation issue. @skef was on point, and I missed the subsequent ping. It seems to be resolved. |
|
@skef I am okay with your commit on github.com/linusromer/fontforge
Is this comment already enough for "sign off" or do I have to do something else? |
new parameter to the existing function and change the other calls.
|
After thinking about this I could hear @jtanx asking "Are there really enough existing calls that we need to add a new signature?" It turns out that there are only 5 so the answer is "no". I added a commit to go back to a single function and change those existing calls so I'll sit on this another day before merging. |
Adds a boolean parameter to ApproximateSplineFromPointsSlopes(). When the boolean is false it behaves as before but if the boolean is true the function uses a more accurate (but slower) method of calculation. The "Merge" function which lives inside splineutil2.c now uses the new algorithm while Simplify and Expand Stroke use the original one. Authored by Linus Romer
Added an additional function ApproximateSplineFromPointsSlopesAccurate() in splinefit.c which has a boolean at the end. When the boolean is false, it behaves as before but if the boolean is true, it supplies a more accurate (but slower) approximation. The "Merge" function which lives inside splineutil2.c now uses the more accurate version. "Expand stroke" and "Simplify" do still use the original function.
Closes #4552