Re: [GEOMETRY] Quickhull Implementation
Hello,
thank you very much! I am looking forward to your review comments since
this is my first time trying to contribute to an open source project. I
also noticed a few things with my code, that should be corrected. Some
comments don’t make sense and there shouldn’t be any need for ordering the
edges like i do right now in my current implementation. Also I think the
code should then be refactored to fit better the desired API design. Also a
discussion should be held if face merging is necessary.
Greetings
Andreas
Matt Juntunen schrieb am Mi. 1. Feb. 2023 um
06:20:
> Hello!
>
> Andreas, thank you for your contribution! It's good to see some work going
> into commons-geometry since I've had to step away recently. Gilles is right
> in that we didn't include the hull module in the 1.0 release because I felt
> like it still needed work. What I'd like to do is remove that module
> completely and pull the convex hull generation directly into the euclidean
> module and hide the implementation algorithm details (as discussed in
> GEOMETRY-144). I've replied to your comment on that Jira issue and I hope
> to take a detailed look at your code over this weekend, if not before. If
> you're interested in seeing the API I was aiming for when working on this,
> my unfinished code is languishing on Github [1].
>
> Regards,
> Matt J
>
> [1]
>
> https://github.com/darkma773r/commons-geometry/blob/quickhull/commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/ConvexHull3D.java
>
>
>
>
> On Mon, Jan 30, 2023 at 2:38 AM Andreas Goss
> wrote:
>
> > Thanks for the quick reply. I need to create an Jira account first for
> > that. I requested an account by writing to priv...@commons.apache.org
> >
> > Kind Regards
> > Andreas
> >
> > Gilles Sadowski schrieb am Mo. 30. Jan. 2023 um
> > 00:11:
> >
> > > Hello.
> > >
> > > Le dim. 29 janv. 2023 à 19:25, Andreas Goss
> > > a écrit :
> > > >
> > > > Hello,
> > > >
> > > > I want to contribute a solution for the open Jira task GEOMETRY-110 (
> > > >
> > >
> >
> https://issues.apache.org/jira/projects/GEOMETRY/issues/GEOMETRY-110?filter=allopenissues
> > > ).
> > > > I tried my best to organize the code according to guidelines and at
> > least
> > > > the maven build was succesful. I created a pull-request for the
> github
> > > > clone. If someone could give me a code review and point out areas to
> > > > improve the code or errors i would be very thankful.
> > >
> > > Thank you for tackling this issue.
> > > It was part of a broader discussion about the API around the hull
> > > functionality.
> > > IIRC, the "commons-geometry-hull" module was not included in the first
> > > release of "Commons Geometry" because Matt Juntunen was convinced
> > > that a few enhancements were necessary. Hopefully, he'll chime in
> order
> > to
> > > make this recollection more precise.
> > >
> > > A general question is whether we want to expose (make "public") classes
> > > that implement the algorithm(s), such as "QuickHull3D". For example,
> we
> > > could perhaps have (untested and required comments missing...):
> > > ---CUT---
> > > public interface ConvexHull3D extends ConvexHull {
> > >
> > > public enum Generate {
> > > QUICK_HULL((c, p) -> new QuickHull3D(p).generate(c));
> > >
> > > private final BiFunction,
> > > DoubleEquivalence, ConvexHull3D> generator;
> > >
> > > private HullGenerator3D(BiFunction,
> > > DoubleEquivalence, ConvexHull3D> generator) {
> > > this.generator = generator;
> > > }
> > >
> > > public ConvexHull3D from(Collection points,
> > > DoubleEquivalence equivalence) {
> > > return generator.apply(points, equivalence));
> > > }
> > > }
> > >
> > > public Collection getFacets();
> > > }
> > >
> > > private class QuickHull3D {
> > > private final DoubleEquivalence precision;
> > >
> > > QuickHull3D(DoubleEquivalence precision) {
> > > this.precision = precision;
> > > }
> > >
> > > ConvexHull3D generate(Collection points) {
> > > // ...
> > > return new SimpleConvexHull3D(...);
> > > }
> > > }
> > >
> > > private class SimpleConvexHull3D implements ConvexHull3D {
> > > private final List vertices;
> > > private final ConvexVolume region;
> > > private Collection facets;
> > >
> > > SimpleConvexHull3D(...) {
> > > // ...
> > > }
> > >
> > > // ...
> > > }
> > > ---CUT---
> > >
> > > For other concrete (basic) remarks about the code, we can certainly
> > > continue the conversation in comments on the JIRA report.
> > >
> > > Best regards,
> > > Gilles
> > >
> > > -
> > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> > > For additional commands, e-mail: dev-h...@commons.apache.org
> > >
> > >
> >
>
Re: [GEOMETRY] Quickhull Implementation
Hello!
Andreas, thank you for your contribution! It's good to see some work going
into commons-geometry since I've had to step away recently. Gilles is right
in that we didn't include the hull module in the 1.0 release because I felt
like it still needed work. What I'd like to do is remove that module
completely and pull the convex hull generation directly into the euclidean
module and hide the implementation algorithm details (as discussed in
GEOMETRY-144). I've replied to your comment on that Jira issue and I hope
to take a detailed look at your code over this weekend, if not before. If
you're interested in seeing the API I was aiming for when working on this,
my unfinished code is languishing on Github [1].
Regards,
Matt J
[1]
https://github.com/darkma773r/commons-geometry/blob/quickhull/commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/ConvexHull3D.java
On Mon, Jan 30, 2023 at 2:38 AM Andreas Goss
wrote:
> Thanks for the quick reply. I need to create an Jira account first for
> that. I requested an account by writing to priv...@commons.apache.org
>
> Kind Regards
> Andreas
>
> Gilles Sadowski schrieb am Mo. 30. Jan. 2023 um
> 00:11:
>
> > Hello.
> >
> > Le dim. 29 janv. 2023 à 19:25, Andreas Goss
> > a écrit :
> > >
> > > Hello,
> > >
> > > I want to contribute a solution for the open Jira task GEOMETRY-110 (
> > >
> >
> https://issues.apache.org/jira/projects/GEOMETRY/issues/GEOMETRY-110?filter=allopenissues
> > ).
> > > I tried my best to organize the code according to guidelines and at
> least
> > > the maven build was succesful. I created a pull-request for the github
> > > clone. If someone could give me a code review and point out areas to
> > > improve the code or errors i would be very thankful.
> >
> > Thank you for tackling this issue.
> > It was part of a broader discussion about the API around the hull
> > functionality.
> > IIRC, the "commons-geometry-hull" module was not included in the first
> > release of "Commons Geometry" because Matt Juntunen was convinced
> > that a few enhancements were necessary. Hopefully, he'll chime in order
> to
> > make this recollection more precise.
> >
> > A general question is whether we want to expose (make "public") classes
> > that implement the algorithm(s), such as "QuickHull3D". For example, we
> > could perhaps have (untested and required comments missing...):
> > ---CUT---
> > public interface ConvexHull3D extends ConvexHull {
> >
> > public enum Generate {
> > QUICK_HULL((c, p) -> new QuickHull3D(p).generate(c));
> >
> > private final BiFunction,
> > DoubleEquivalence, ConvexHull3D> generator;
> >
> > private HullGenerator3D(BiFunction,
> > DoubleEquivalence, ConvexHull3D> generator) {
> > this.generator = generator;
> > }
> >
> > public ConvexHull3D from(Collection points,
> > DoubleEquivalence equivalence) {
> > return generator.apply(points, equivalence));
> > }
> > }
> >
> > public Collection getFacets();
> > }
> >
> > private class QuickHull3D {
> > private final DoubleEquivalence precision;
> >
> > QuickHull3D(DoubleEquivalence precision) {
> > this.precision = precision;
> > }
> >
> > ConvexHull3D generate(Collection points) {
> > // ...
> > return new SimpleConvexHull3D(...);
> > }
> > }
> >
> > private class SimpleConvexHull3D implements ConvexHull3D {
> > private final List vertices;
> > private final ConvexVolume region;
> > private Collection facets;
> >
> > SimpleConvexHull3D(...) {
> > // ...
> > }
> >
> > // ...
> > }
> > ---CUT---
> >
> > For other concrete (basic) remarks about the code, we can certainly
> > continue the conversation in comments on the JIRA report.
> >
> > Best regards,
> > Gilles
> >
> > -
> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> > For additional commands, e-mail: dev-h...@commons.apache.org
> >
> >
>
Re: [GEOMETRY] Quickhull Implementation
Thanks for the quick reply. I need to create an Jira account first for
that. I requested an account by writing to priv...@commons.apache.org
Kind Regards
Andreas
Gilles Sadowski schrieb am Mo. 30. Jan. 2023 um
00:11:
> Hello.
>
> Le dim. 29 janv. 2023 à 19:25, Andreas Goss
> a écrit :
> >
> > Hello,
> >
> > I want to contribute a solution for the open Jira task GEOMETRY-110 (
> >
> https://issues.apache.org/jira/projects/GEOMETRY/issues/GEOMETRY-110?filter=allopenissues
> ).
> > I tried my best to organize the code according to guidelines and at least
> > the maven build was succesful. I created a pull-request for the github
> > clone. If someone could give me a code review and point out areas to
> > improve the code or errors i would be very thankful.
>
> Thank you for tackling this issue.
> It was part of a broader discussion about the API around the hull
> functionality.
> IIRC, the "commons-geometry-hull" module was not included in the first
> release of "Commons Geometry" because Matt Juntunen was convinced
> that a few enhancements were necessary. Hopefully, he'll chime in order to
> make this recollection more precise.
>
> A general question is whether we want to expose (make "public") classes
> that implement the algorithm(s), such as "QuickHull3D". For example, we
> could perhaps have (untested and required comments missing...):
> ---CUT---
> public interface ConvexHull3D extends ConvexHull {
>
> public enum Generate {
> QUICK_HULL((c, p) -> new QuickHull3D(p).generate(c));
>
> private final BiFunction,
> DoubleEquivalence, ConvexHull3D> generator;
>
> private HullGenerator3D(BiFunction,
> DoubleEquivalence, ConvexHull3D> generator) {
> this.generator = generator;
> }
>
> public ConvexHull3D from(Collection points,
> DoubleEquivalence equivalence) {
> return generator.apply(points, equivalence));
> }
> }
>
> public Collection getFacets();
> }
>
> private class QuickHull3D {
> private final DoubleEquivalence precision;
>
> QuickHull3D(DoubleEquivalence precision) {
> this.precision = precision;
> }
>
> ConvexHull3D generate(Collection points) {
> // ...
> return new SimpleConvexHull3D(...);
> }
> }
>
> private class SimpleConvexHull3D implements ConvexHull3D {
> private final List vertices;
> private final ConvexVolume region;
> private Collection facets;
>
> SimpleConvexHull3D(...) {
> // ...
> }
>
> // ...
> }
> ---CUT---
>
> For other concrete (basic) remarks about the code, we can certainly
> continue the conversation in comments on the JIRA report.
>
> Best regards,
> Gilles
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
Re: [GEOMETRY] Quickhull Implementation
Hello.
Le dim. 29 janv. 2023 à 19:25, Andreas Goss
a écrit :
>
> Hello,
>
> I want to contribute a solution for the open Jira task GEOMETRY-110 (
> https://issues.apache.org/jira/projects/GEOMETRY/issues/GEOMETRY-110?filter=allopenissues).
> I tried my best to organize the code according to guidelines and at least
> the maven build was succesful. I created a pull-request for the github
> clone. If someone could give me a code review and point out areas to
> improve the code or errors i would be very thankful.
Thank you for tackling this issue.
It was part of a broader discussion about the API around the hull functionality.
IIRC, the "commons-geometry-hull" module was not included in the first
release of "Commons Geometry" because Matt Juntunen was convinced
that a few enhancements were necessary. Hopefully, he'll chime in order to
make this recollection more precise.
A general question is whether we want to expose (make "public") classes
that implement the algorithm(s), such as "QuickHull3D". For example, we
could perhaps have (untested and required comments missing...):
---CUT---
public interface ConvexHull3D extends ConvexHull {
public enum Generate {
QUICK_HULL((c, p) -> new QuickHull3D(p).generate(c));
private final BiFunction,
DoubleEquivalence, ConvexHull3D> generator;
private HullGenerator3D(BiFunction,
DoubleEquivalence, ConvexHull3D> generator) {
this.generator = generator;
}
public ConvexHull3D from(Collection points,
DoubleEquivalence equivalence) {
return generator.apply(points, equivalence));
}
}
public Collection getFacets();
}
private class QuickHull3D {
private final DoubleEquivalence precision;
QuickHull3D(DoubleEquivalence precision) {
this.precision = precision;
}
ConvexHull3D generate(Collection points) {
// ...
return new SimpleConvexHull3D(...);
}
}
private class SimpleConvexHull3D implements ConvexHull3D {
private final List vertices;
private final ConvexVolume region;
private Collection facets;
SimpleConvexHull3D(...) {
// ...
}
// ...
}
---CUT---
For other concrete (basic) remarks about the code, we can certainly
continue the conversation in comments on the JIRA report.
Best regards,
Gilles
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
[GEOMETRY] Quickhull Implementation
Hello, I want to contribute a solution for the open Jira task GEOMETRY-110 ( https://issues.apache.org/jira/projects/GEOMETRY/issues/GEOMETRY-110?filter=allopenissues). I tried my best to organize the code according to guidelines and at least the maven build was succesful. I created a pull-request for the github clone. If someone could give me a code review and point out areas to improve the code or errors i would be very thankful. Thanks! Andreas
