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

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

Open
Keavon opened this issue Nov 1, 2024 · 6 comments · May be fixed by #2091
Open

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

Keavon opened this issue Nov 1, 2024 · 6 comments · May be fixed by #2091
Labels
Bug Something isn't working Good First Issue Good for newcomers Help Wanted Extra attention is needed Rust Involves Rust programming for the backend

Comments

@Keavon
Copy link
Member

Keavon commented Nov 1, 2024

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.

@Keavon Keavon added Bug Something isn't working Help Wanted Extra attention is needed Good First Issue Good for newcomers Rust Involves Rust programming for the backend labels Nov 1, 2024
@DivvSaxena
Copy link

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
Copy link
Member Author

Keavon commented Nov 3, 2024

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

@kalle95data
Copy link

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
Copy link
Member Author

Keavon commented Nov 3, 2024

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
Copy link
Member Author

Keavon commented Nov 3, 2024

@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
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Good First Issue Good for newcomers Help Wanted Extra attention is needed Rust Involves Rust programming for the backend
Projects
Status: Short-Term
Development

Successfully merging a pull request may close this issue.

3 participants