search

GraphiteEditor / Graphite

2D vector & raster editor that melds traditional layers & tools with a modern node-based, non-destructive, procedural workflow.
https://graphite.rs
Apache License 2.0
9.63k stars 468 forks source link

Fix the spline node algorithm to be continuous across start/end points #2086

Closed Keavon closed 3 weeks ago

Keavon commented 3 weeks ago

Currently, the Spline from Points node doesn't handle continuity across the start/end point portion of the edge flow. This is what a square looks like when the node is applied, which fails in the top left corner where the start/end points connect:

capture

Instead, this example should look like a symmetrical blob, with the same roundness around all corners.

This task involves updating the spline calculation algorithm, as pointed out by this todo comment.

DivvSaxena commented 3 weeks ago

Hi @Keavon I was interested into contributing this code , I have already setup the project locally by following all the guidelines listed on the contribution page.

This will be the first time I'll be contributing to graphite, so I'm not totally aware about the codebase, still I wanted to solve this issue I need to add a calculate_spline_control_points function that will calculate the spline control points instead of solve_spline_first_handle

and Update the spline node code to handle closed paths.

But I'm little confused to be honest any feedback on how should I approach this problem.

Looking forward for your response ,

Divv

Keavon commented 3 weeks ago

Welcome @DivvSaxena! We can continue this discussion through Discord.

kalle95data commented 3 weeks ago

Just tried my first contribution, and have a working fix for this now on my computer. My method was to "overshoot" by using the two next nodes in the loop, and then copy the Bezier handle from the first "extra" node to the first node. This was also one of my first "real" steps in Rust.

Keavon commented 3 weeks ago

Thanks for working on it, @kalle95data! Could you please open a PR for it? (Let me know on Discord if you need help with the Git aspects.) I look forward to testing out your solution to see whether this produces a sufficiently close result to the mathematically "correct" approach. It'll certainly be a good improvement, even if we end up revisiting this later to implement the mathematically correct approach.

Keavon commented 3 weeks ago

@0HyperCube wrote in Discord:

Currently we implement equation 18 from https://mathworld.wolfram.com/CubicSpline.html#eqn19, if we use equation 19 with the periodic boundary condition (which requires an adapted version of the Thomas algorithm) we would solve it perfectly.

kalle95data commented 3 weeks ago

I haven't managed to get cubic spline with a loop / cyclic boundaries all the way yet (the best so far is where the handles were rotated 90 degrees), but unfortunately there is still something wrong. I submitted a solution now which is based on the "normal" cubic spline but to "overshoot" around the loop by 9 more segments and then replace the segments around the boundary with these "overlapping" segments. This solution is probably good enough, visually almost perfect (haven't "proved" it mathematically, but feels good enough). (But one day I will probably get the correct solution working, but it will be in the future)