BelfrySCAD / BOSL2

The Belfry OpenScad Library, v2.0. An OpenSCAD library of shapes, masks, and manipulators to make working with OpenSCAD easier. BETA
https://github.com/BelfrySCAD/BOSL2/wiki
BSD 2-Clause "Simplified" License
1.01k stars 115 forks source link

[BUG] path_merge_collinear(path, closed = true) will not remove a collinear point if it is the first point in a path #1385

Closed AUnicyclingProgrammer closed 9 months ago

AUnicyclingProgrammer commented 9 months ago

Describe the bug path_merge_collinear(path, closed = true) will not remove a collinear point if it is the starting point of the path, but it can be forced to remove the collinear point if the input path is reversed.

Code To Reproduce Bug

include <BOSL2/std.scad>

examplePath = [[10,0],[10,-10],[-10,-10],[-10, 10],[10, 10]];

// Current Merge Function Behavior

forwardPathMergeCollinearResult = path_merge_collinear(examplePath, closed = true);

backwardsPathMergeCollinearResult = path_merge_collinear(reverse(examplePath), closed = true);

xdistribute(spacing = 30) {    
    color("red")
    VisualizePath(examplePath);

    color("green")
    VisualizePath(forwardPathMergeCollinearResult);

    color("blue")
    VisualizePath(backwardsPathMergeCollinearResult);
}

// Personal Solution Implementing Expected Behavior

customMergeCollinearFunction = ExhaustiveClosedPathMergeCollinear(examplePath);

backwardsCustomMergeCollinearFunction = ExhaustiveClosedPathMergeCollinear(examplePath);

fwd(30)
xdistribute(spacing = 30) {    
    color("magenta")
    VisualizePath(customMergeCollinearFunction);

    color("cyan")
    VisualizePath(
    backwardsCustomMergeCollinearFunction);
}

// To hide file paths in console
echo("\n\n\n\n\n\n\n");

// ===== Helper Modules and Functions =====
// I took this directly from the pairs documentation
module VisualizePath(path) {
    for (p = pair(path, wrap=true))
        stroke(p, endcap2="arrow2");
}

// Functions I wrote
/*
    Performs the not operation on every element in a boolean list

    list : boolean list to invert
*/
function NotBooleanList(list) = [for (i = list) !i];
//

/*
    Extends the functionality of path_merge_collinear to verify that absolutely all
    co-linear points are removed (path_merge_collinear will not the start point of a closed
    path, even if it is collinear).

    path : path to perform collinearity check on
*/
function ExhaustiveClosedPathMergeCollinear(path) =
    let(
        allPointTriplets = [for (p = triplet(path, wrap=true)) p],

        collinearityBitMask = [for (triplet = allPointTriplets)
            if(is_collinear(triplet)) 1 else 0],

    collinearPath = bselect(path, NotBooleanList(collinearityBitMask))
    ) collinearPath;
//

Expected behavior I don't know if this is a bug or intentional. I expected the function to remove all collinear points regardless of where they are in the path.

Screenshots image

Additional context Add any other context about the problem here.

adrianVmariano commented 9 months ago

That looks like a bug. Does this work in all the cases?

function path_merge_collinear(path, closed, eps=EPSILON) =
    is_1region(path) ? path_merge_collinear(path[0], default(closed,true), eps) :
    let(closed=default(closed,false))
    assert(is_bool(closed))
    assert( is_path(path), "Invalid path in path_merge_collinear." )
    assert( is_undef(eps) || (is_finite(eps) && (eps>=0) ), "Invalid tolerance." )
    len(path)<=2 ? path :
    [
      if(!closed) path[0],
      for(triple=triplet(path,wrap=closed))
        if (!is_collinear(triple,eps=eps)) triple[1],
      if(!closed) last(path)
  ];

Note, your use of triplet was better than existing code, but you've overcomplicated things in various other ways.

AUnicyclingProgrammer commented 9 months ago

That does appear to work in all the cases.

Testing Proposed Patch

include <BOSL2/std.scad>

examplePath = [[10,0],[10,-10],[-10,-10],[-10, 10],[10, 10]];

// Patched path_merge_collinear(path, closed = true) Behavior

forwardPathMergeCollinearResult = path_merge_collinear(examplePath, closed = true);

backwardsPathMergeCollinearResult = path_merge_collinear(reverse(examplePath), closed = true);

xdistribute(spacing = 30) {    
    color("red")
    VisualizePath(examplePath);

    color("green")
    VisualizePath(forwardPathMergeCollinearResult);

    color("blue")
    VisualizePath(backwardsPathMergeCollinearResult);
}

// To hide file paths in console
echo("\n\n\n\n\n\n\n");

// Proposed patch
function path_merge_collinear(path, closed, eps=EPSILON) =
    is_1region(path) ? path_merge_collinear(path[0], default(closed,true), eps) :
    let(closed=default(closed,false))
    assert(is_bool(closed))
    assert( is_path(path), "Invalid path in path_merge_collinear." )
    assert( is_undef(eps) || (is_finite(eps) && (eps>=0) ), "Invalid tolerance." )
    len(path)<=2 ? path :
    [
      if(!closed) path[0],
      for(triple=triplet(path,wrap=closed))
        if (!is_collinear(triple,eps=eps)) triple[1],
      if(!closed) last(path)
  ];

// ===== Helper Modules and Functions =====
// I took this directly from the pairs documentation
module VisualizePath(path) {
    for (p = pair(path, wrap=true))
        stroke(p, endcap2="arrow2");
}

Screenshots image

I'm still working on getting more proficient in OpenSCAD and with BOSL2, so the compliment and advice are appreciated.