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

Possible bug in offset_scaling #16

Open
hellow554 opened this issue Feb 7, 2022 · 1 comment
Open

Possible bug in offset_scaling #16

hellow554 opened this issue Feb 7, 2022 · 1 comment

Comments

@hellow554
Copy link

if extremeties.len() == 0 || true {

Why is there a || true? This is either a temporary fix and should be commented or it's a bug.

Can you clarify this please?

@Logicalshift
Copy link
Owner

It does appear to be something left in from when I was testing this, and it appears to mask a bug, as removing it doesn't go well:

Screenshot 2022-02-07 at 19 26 14

Seems to be because find_extremities() returns unordered results, so the subdivisions are all over the place later on. The purpose of this is to reduce the number of subdivisions around sections of the curve that can't be scaled around a point - a downside of the scaling algorithm is it always subdivides the same number of times even if the resulting curve can be constructed without quite so many sections.

I've been wondering if there's an even better approach to subdividing the curve: the scaling algorithm requires the curve to be subdivided into sections such that the normals all converge on a roughly equidistant point: better algorithms subdivide less (but anything that subdivides will eventually find curves good enough to use so it's hard to tell if any given algorithm is 'best' or not).

Anyway, this should now be fixed so it works as it was intended (improving other aspects of the algorithm is at earliest a v0.7 thing I think)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants