Fix trivial_hull implementation for collinear points

[tyilo]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

Fixes #905

[urschrei]

Thanks! This looks good to me, but it would be great if you could add a brief explanation of the fix to your comment above, for those of us who can't easily visualise what's going on.

[tyilo]

If include_hull is false, then given the input [(0, 0), (1, 1), (2,2)] (in any order) the output should be [(0, 0), (2, 2), (0, 0)] (or [(2, 2), (0, 0), (2, 2)]).

Before this fix, with input [(0, 0), (1, 1), (2, 2)] the output is [(0, 0), (1, 1), (2, 2), (0, 0)].

To fix it, we need to remove the "middle" point if the three points are colinear.
To do this, we first sort the three points (first by x, then by y) and then remove the point at index 1, if they are colinear.

The sorting is needed to give the correct output for any permutation of [(0, 0), (1, 1), (2, 2)], for instance [(2, 2), (0, 0), (1, 1)].

[rmanoka]

Thanks for the PR @tyilo ! Just have a few minor suggestions, but the overall logic looks good to me.

[rmanoka]

Is it possible to avoid this extra Vec allocation? Can you sort the input mut-slice directly?

[tyilo]

We can sort the mut-slice directly, however we can't do ls.remove(1); on a slice, so I don't see how we can do it without creating a new Vec.

@rmanoka I find it weird that the convex hull algorithms (trivial_hull, graham_hull and quick_hull) takes points: &mut [Coordinate<T>] as input.
We don't guarantee anything about how points has been mutated by the algorithm, right?

So wouldn't it be better to take either &[Coordinate<T>] or Vec<Coordinate<T>> as input?

[urschrei]

I think we need mutable points for the partition_slice function which reorders coordinates based on a predicate?

[tyilo]

I think we need mutable points for the partition_slice function which reorders coordinates based on a predicate?

That would also work if we took input as Vec<Coordinate<T>>.

[urschrei]

I think we need mutable points for the partition_slice function which reorders coordinates based on a predicate?

That would also work if we took input as Vec<Coordinate<T>>.

I am confused – could you say more about what you have in mind here?

[tyilo]

I think we need mutable points for the partition_slice function which reorders coordinates based on a predicate?

That would also work if we took input as Vec<Coordinate<T>>.

I am confused – could you say more about what you have in mind here?

So there are basically three options for taking slices as input in rust:

• Owned: Box<[Coordinate<T>]> or Vec<Coordinate<T>>. Makes sense if we need to consume the input to do the computation.
• Borrowed: &[Coordinate<T>]. Makes sense if we only need to read the input and consuming the input (as in "Owned") doesn't speed up the computation.
• Mutably borrowed: &mut [Coordinate<T>]. Makes sense if the computation needs to change the input, but also the user of the function should (at least in some situations) be interested in the mutated input. If mutated input is never useful for the user, we should prefer to use an owned input.

I'm arguing that the mutated points input is never useful for the user and thus to prevent the user from accidentally using the now garbage list of points and to make the api more clear, we should choose the "Owned" option.

[rmanoka]

Yeah, the function signature was primarily whatever works for the implementation. Note that the usage is anyway via the trait. If it simplifies code without extra allocations, we can move to something more intuitive.

[tyilo]

Yeah, the function signature was primarily whatever works for the implementation. Note that the usage is anyway via the trait. If it simplifies code without extra allocations, we can move to something more intuitive.

Makes sense. I don't think that change is relevant to this pull request.
Do I need to fix anything else before this can get merged?

[rmanoka]

@tyilo pls. pull changes from main to fix the other CI issues. I think it's ready to merge once CI passes.

[tyilo]

@tyilo pls. pull changes from main to fix the other CI issues. I think it's ready to merge once CI passes.

Done.

[urschrei]

bors r=rmanoka

[bors]

Build succeeded:

• ci result