prusa3d / PrusaSlicer

G-code generator for 3D printers (RepRap, Makerbot, Ultimaker etc.)
https://www.prusa3d.com/prusaslicer/
GNU Affero General Public License v3.0
7.79k stars 1.94k forks source link

Polygon::filter_points_by_vectors is strange #13266

Open supermerill opened 3 months ago

supermerill commented 3 months ago

Description of the bug

In Polygon::filter_points_by_vectors the comment say and returns true if the point is to be copied to the output. but it's the next_point that is copied in the output If I read the code correctly...

// filter function receives two vectors:
// v1: this_point - previous_point
// v2: next_point - this_point
// and returns true if the point is to be copied to the output.
template<typename FilterFn>
Points filter_points_by_vectors(const Points &poly, FilterFn filter)
{
    // Last point is the first point visited.
    Point p1 = poly.back();
    // Previous vector to p1.
    Vec2d v1 = (p1 - *(poly.end() - 2)).cast<double>();

    Points out;
    for (Point p2 : poly) {
        // p2 is next point to the currently visited point p1.
        Vec2d v2 = (p2 - p1).cast<double>();
        if (filter(v1, v2))
            out.emplace_back(p2);
        v1 = v2;
        p1 = p2;
    }

    return out;
}

Is it a possible error or i'm wrong?

Project file & How to reproduce

https://github.com/prusa3d/PrusaSlicer/blob/fd02400245df25f1286059aa642739660df76663/src/libslic3r/Polygon.cpp#L242

Checklist of files included above

Version of PrusaSlicer

master

Operating system

github

Printer model

visual studio

supermerill commented 3 months ago

What I would write:

    for (size_t idx = 1; idx < poly.size(); ++idx) {
        const Point &p2 = poly[idx];
        // p2 is next point to the currently visited point p1.
        Vec2d v2 = (p2 - p1).cast<double>();
        if (filter(v1, v2))
            out.push_back(p1);
        v1 = v2;
        p1 = p2;
    }

    // also check last point.
    {
        const Point &p2 = poly.front();
        // p2 is next point to the currently visited point p1.
        Vec2d v2 = (p2 - p1).cast<double>();
        if (filter(v1, v2))
            out.push_back(p1);
    }