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

small improovement for getResampledBySpacing #6361

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

jonasfehr
Copy link
Contributor

To avoid bigger gaps at the end of the resampled polyline, adding almost a full spacing will help adding a rather small last space than a big one. The last point is replaced anyway.

To avoid bigger gaps at the end of the resampled polyline, adding almost a full spacing will help adding a rather small last space than a big one. The last point is replaced anyway.
@ofTheo
Copy link
Member

ofTheo commented Aug 27, 2019

Thanks for this!
I actually ran into this bug recently where we were doing large spacing and couldn't figure out the big gap at the end of a resampled poly.

I was also trying to figure out why your fix worked as it didn't quite make sense at first ( going over the total length of the line ), but as you said it moved the last point to be where the last point of the original line is which can create the big gap.

I think this might be a slightly cleaner fix - replacing the move of the last vertex with an add of the last vertex. it has the same visual effect but avoids doing extra calculation in the for loop.

    float f=0;
    for(f=0; f<totalLength; f += spacing) {
        poly.lineTo(getPointAtLength(f));
    }
    
    if(!isClosed()) {
        if( f != totalLength ){
            poly.lineTo(points.back());
        }
        poly.setClosed(false);
    } else {
        poly.setClosed(true);
    }

For reference here is how it looked before your PR:
TestResample 2019-08-26 21_50_15

And with your fix and the fix I suggested above:
TestResampleWithFix 2019-08-26 21_50_55

@ofTheo ofTheo self-assigned this Aug 27, 2019
@jonasfehr
Copy link
Contributor Author

jonasfehr commented Aug 27, 2019 via email

@ofTheo
Copy link
Member

ofTheo commented Aug 27, 2019

Ah yes :)
Looks good - thanks!!

@ofTheo ofTheo merged commit 459145e into openframeworks:master Aug 27, 2019
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

Successfully merging this pull request may close these issues.

2 participants