[geometry] GEOMTRY-32 Feedback Requested

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[geometry] GEOMTRY-32 Feedback Requested

Matt Juntunen
Hi all,

I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can. Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:

  *   More user-friendly API
     *   Does not require users to make unchecked casts to implementation types.
     *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
     *   Uses builder pattern instead of large factory methods to build complicated geometries.
     *   Most classes are immutable.
  *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
  *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
  *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
  *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.

Regards,
Matt

[1] https://github.com/apache/commons-geometry/pull/34
Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Gilles Sadowski-2
Hello Matt.

Thanks for continuing to work on this.

Le jeu. 15 août 2019 à 17:56, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi all,
>
> I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.

There is a spurious change of ".travis.yml".  Also there are wrong EOL
characters
in that file:
---CUT---
$ file .travis.yml
.travis.yml: ASCII text, with CRLF line terminators
---CUT---
Perhaps you should merge or rebase on "master".

> Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
>
>   *   More user-friendly API
>      *   Does not require users to make unchecked casts to implementation types.
>      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
>      *   Uses builder pattern instead of large factory methods to build complicated geometries.

That would suit me. ;-)
Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
Where should I start looking?

>      *   Most classes are immutable.
>   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.

Can it be associated with a surface element?

>   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
>   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.

Nice.

>   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.

Impressive.

It would be great to create a maven module for systematizing the benchmarks;
see e.g. what is being done in "Commons RNG":
    https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh

A "commons-geometry-examples" module would be a place to collect
useful code, e.g. simple "howtos" (like the one I mentioned above) and
conversion routines from/to popular formats, without the requirement of
backwards compatibility (even between minor releases).

Best,
Gilles

>
> Regards,
> Matt
>
> [1] https://github.com/apache/commons-geometry/pull/34

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Matt Juntunen
Hi Gilles,

  1.  I just rebased from master so hopefully that fixed the repo weirdness.
  2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes. These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
  3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?
  4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.

-Matt

DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);

// Create a pyramid region with its base on the xy plane and its apex
// along the positive z axis. The base is 2 units to a side and the pyramid
// is 2 units high.
Vector3D[] vertices = {
    Vector3D.of(1, 1, 0),
    Vector3D.of(1, -1, 0),
    Vector3D.of(-1, -1, 0),
    Vector3D.of(-1, 1, 0),
    Vector3D.of(0, 0, 2)
};

RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
        .withVertexList(vertices)

        // add the faces; these could alternatively be given as a 2d array
        .addIndexedFacet(0, 1, 2, 3)
        .addIndexedFacet(0, 4, 1)
        .addIndexedFacet(1, 4, 2)
        .addIndexedFacet(2, 4, 3)
        .addIndexedFacet(3, 4, 0)
        .build();

System.out.println("# Pyramid" +
        "\nbarycenter= " + region.getBarycenter() +
        "\nvolume= " + region.getSize() +
        "\nsurface area= " + region.getBoundarySize());

// Create a unit box centered at the origin
RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
        .addCenteredCube(Vector3D.ZERO, 1)
        .build();

System.out.println("\n# Box" +
        "\nbarycenter= " + box.getBarycenter() +
        "\nvolume= " + box.getSize() +
        "\nsurface area= " + box.getBoundarySize());

// Move the box center to the pyramid center and lengthen it
// along the z-axis
AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
        .translate(region.getBarycenter())
        .scale(0.75, 0.75, 4);

box.transform(mat);

System.out.println("\n# Transformed Box" +
        "\nbarycenter= " + box.getBarycenter() +
        "\nvolume= " + box.getSize() +
        "\nsurface area= " + box.getBoundarySize());

// Subtract the box from the pyramid. The pyramid is modified but
// the box is not.
region.difference(box);

System.out.println("\n# Pyramid with hole" +
        "\nbarycenter= " + region.getBarycenter() +
        "\nvolume= " + region.getSize() +
        "\nsurface area= " + region.getBoundarySize());

// Print out the vertices of the faces that make up the modified
// pyramid. The faces are not simplified; they represent the internal
// structure of the bsp tree.
System.out.println("\n# Faces");
for (ConvexSubPlane face : region.boundaries()) {
    System.out.println(face.getVertices());
}

________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Friday, August 16, 2019 11:37 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] GEOMTRY-32 Feedback Requested

Hello Matt.

Thanks for continuing to work on this.

Le jeu. 15 août 2019 à 17:56, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi all,
>
> I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.

There is a spurious change of ".travis.yml".  Also there are wrong EOL
characters
in that file:
---CUT---
$ file .travis.yml
.travis.yml: ASCII text, with CRLF line terminators
---CUT---
Perhaps you should merge or rebase on "master".

> Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
>
>   *   More user-friendly API
>      *   Does not require users to make unchecked casts to implementation types.
>      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
>      *   Uses builder pattern instead of large factory methods to build complicated geometries.

That would suit me. ;-)
Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
Where should I start looking?

>      *   Most classes are immutable.
>   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.

Can it be associated with a surface element?

>   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
>   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.

Nice.

>   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.

Impressive.

It would be great to create a maven module for systematizing the benchmarks;
see e.g. what is being done in "Commons RNG":
    https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh

A "commons-geometry-examples" module would be a place to collect
useful code, e.g. simple "howtos" (like the one I mentioned above) and
conversion routines from/to popular formats, without the requirement of
backwards compatibility (even between minor releases).

Best,
Gilles

>
> Regards,
> Matt
>
> [1] https://github.com/apache/commons-geometry/pull/34

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Gilles Sadowski-2
Hello.

Le sam. 17 août 2019 à 04:58, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi Gilles,
>
>   1.  I just rebased from master so hopefully that fixed the repo weirdness.

It's fixed indeed.

>   2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes.

Thanks for the pointer.

> These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
>   3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?

I think that what I called surface element is the equivalent of "facet".

>   4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.

Great.

>
> -Matt
>
> DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);
>
> // Create a pyramid region with its base on the xy plane and its apex
> // along the positive z axis. The base is 2 units to a side and the pyramid
> // is 2 units high.
> Vector3D[] vertices = {
>     Vector3D.of(1, 1, 0),
>     Vector3D.of(1, -1, 0),
>     Vector3D.of(-1, -1, 0),
>     Vector3D.of(-1, 1, 0),
>     Vector3D.of(0, 0, 2)
> };
>
> RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
>         .withVertexList(vertices)
>
>         // add the faces; these could alternatively be given as a 2d array
>         .addIndexedFacet(0, 1, 2, 3)
>         .addIndexedFacet(0, 4, 1)
>         .addIndexedFacet(1, 4, 2)
>         .addIndexedFacet(2, 4, 3)
>         .addIndexedFacet(3, 4, 0)
>         .build();
>
> System.out.println("# Pyramid" +
>         "\nbarycenter= " + region.getBarycenter() +
>         "\nvolume= " + region.getSize() +
>         "\nsurface area= " + region.getBoundarySize());
>
> // Create a unit box centered at the origin
> RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
>         .addCenteredCube(Vector3D.ZERO, 1)
>         .build();

I've had a quick look at "RegionBSPTree3D" as you suggested.

First questions (from a newbie POV):
* Do you intend to add more convenience methods to the
"Builder" (such as "addCube" and  "addCenteredCube")?  If not, shouldn't
the class be extensible (currently it is "final")?  If yes, where do we stop
("addSphere", "addPyramid", "addTorus", ...)?
* Wouldn't it be cleaner to separate "core" operations ("addFacet",
"addIndexedFacets", ...) from convenience methods that use them
('addCube", ...)?
* Then couldn't we define a more generic way to specify how to construct
all sorts of shapes?  E.g.:
---CUT---
public class RegionBSPTree3D {
    // ...
    public final class Builder {
        public Builder addFacets(List<List<Vector3D>> facets) {
            for (List<Vector3D> f : facets) {
                addFacet(f);
            }
        }
        public Builder add(FacetGenerator gen) {
            return addFacets(gen.build());
        }
        // ...
    }
    // ...
}

And e.g.
---CUT---
public classs RectangleGenerator implements FacetGenerator {
    /*
     * @param a first corner point in the rectangular prism (opposite
of {@code b})
     * @param b second corner point in the rectangular prism (opposite
of {@code a})
     */
    public RectangleGenerator(Vector3D a, Vector3D b) {
        // ...
    }

    @Override
    List<List<Vector3D>> build() {
        // ...
    }
}
---CUT---

WDYT?

Various nit-picks:

* Please put a space around operators:
---CUT---
for (int i=0; i<vertexIndices.length; ++i)
---CUT---
should be
---CUT---
for (int i = 0; i < vertexIndices.length; i++)
---CUT---

* Use "final" local variables wherever the reference won't be assigned a
new value.

* Use one condition per line:
---CUT---
if (precision.eq(minX, maxX) || precision.eq(minY, maxY) ||
precision.eq(minZ, maxZ))
---CUT---
should be
---CUT---
if (precision.eq(minX, maxX) ||
    precision.eq(minY, maxY) ||
    precision.eq(minZ, maxZ))
---CUT---


Best,
Gilles

> System.out.println("\n# Box" +
>         "\nbarycenter= " + box.getBarycenter() +
>         "\nvolume= " + box.getSize() +
>         "\nsurface area= " + box.getBoundarySize());
>
> // Move the box center to the pyramid center and lengthen it
> // along the z-axis
> AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
>         .translate(region.getBarycenter())
>         .scale(0.75, 0.75, 4);
>
> box.transform(mat);
>
> System.out.println("\n# Transformed Box" +
>         "\nbarycenter= " + box.getBarycenter() +
>         "\nvolume= " + box.getSize() +
>         "\nsurface area= " + box.getBoundarySize());
>
> // Subtract the box from the pyramid. The pyramid is modified but
> // the box is not.
> region.difference(box);
>
> System.out.println("\n# Pyramid with hole" +
>         "\nbarycenter= " + region.getBarycenter() +
>         "\nvolume= " + region.getSize() +
>         "\nsurface area= " + region.getBoundarySize());
>
> // Print out the vertices of the faces that make up the modified
> // pyramid. The faces are not simplified; they represent the internal
> // structure of the bsp tree.
> System.out.println("\n# Faces");
> for (ConvexSubPlane face : region.boundaries()) {
>     System.out.println(face.getVertices());
> }
>
> ________________________________
> From: Gilles Sadowski <[hidden email]>
> Sent: Friday, August 16, 2019 11:37 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello Matt.
>
> Thanks for continuing to work on this.
>
> Le jeu. 15 août 2019 à 17:56, Matt Juntunen
> <[hidden email]> a écrit :
> >
> > Hi all,
> >
> > I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.
>
> There is a spurious change of ".travis.yml".  Also there are wrong EOL
> characters
> in that file:
> ---CUT---
> $ file .travis.yml
> .travis.yml: ASCII text, with CRLF line terminators
> ---CUT---
> Perhaps you should merge or rebase on "master".
>
> > Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
> >
> >   *   More user-friendly API
> >      *   Does not require users to make unchecked casts to implementation types.
> >      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
> >      *   Uses builder pattern instead of large factory methods to build complicated geometries.
>
> That would suit me. ;-)
> Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
> Where should I start looking?
>
> >      *   Most classes are immutable.
> >   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
>
> Can it be associated with a surface element?
>
> >   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
> >   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
>
> Nice.
>
> >   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.
>
> Impressive.
>
> It would be great to create a maven module for systematizing the benchmarks;
> see e.g. what is being done in "Commons RNG":
>     https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh
>
> A "commons-geometry-examples" module would be a place to collect
> useful code, e.g. simple "howtos" (like the one I mentioned above) and
> conversion routines from/to popular formats, without the requirement of
> backwards compatibility (even between minor releases).
>
> Best,
> Gilles
>
> >
> > Regards,
> > Matt
> >
> > [1] https://github.com/apache/commons-geometry/pull/34
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Matt Juntunen
Hi Gilles,

I did intend on adding more convenience methods for generating standard shapes but I definitely think we should abstract it like you're suggesting. Your design got me thinking and I believe we could take the idea of a FacetGenerator further and make it represent anything at all that can produce a sequence of facets, be it a shape generator, a mesh, a 3D model file, a database table, a RegionBSPTree3D, etc. We could make full use of the streams API as well. The main interface would look like this:

public interface FacetSource {
    Stream<ConvexSubPlane> facetStream();
}

This would essentially perform the same task as your FacetGenerator, but it would allow facets to be streamed in one at a time instead of loaded into memory up front for each use. This would allow us to have code like this:

RegionBSPTree3D tree = RegionBSPTree3D.builder(precision)
    .add(new RectangleFacetSource(p1, p2))
    .build();

-- or --

RegionBSPTree3D tree = RegionBSPTree3D.empty();
new RectangleFacetSource(p1, p2).facetStream()
   .map(f -> f.transform(transform))
   .filter(f -> f.getPlane().getNormal().dot(vec) > 0)
   .forEach(tree::insert);

-- or even --

RegionBSPTree3D tree;
try (Stream<ConvexSubPlane> stream = new OBJFacetSource("model.obj").facetStream()) {
    tree = stream.map(f -> f.transform(transform))
             .collect(FacetCollectors.toTree());
}

What do you think of this slightly modified approach?

-Matt

________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Saturday, August 17, 2019 6:00 PM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] GEOMTRY-32 Feedback Requested

Hello.

Le sam. 17 août 2019 à 04:58, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi Gilles,
>
>   1.  I just rebased from master so hopefully that fixed the repo weirdness.

It's fixed indeed.

>   2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes.

Thanks for the pointer.

> These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
>   3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?

I think that what I called surface element is the equivalent of "facet".

>   4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.

Great.

>
> -Matt
>
> DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);
>
> // Create a pyramid region with its base on the xy plane and its apex
> // along the positive z axis. The base is 2 units to a side and the pyramid
> // is 2 units high.
> Vector3D[] vertices = {
>     Vector3D.of(1, 1, 0),
>     Vector3D.of(1, -1, 0),
>     Vector3D.of(-1, -1, 0),
>     Vector3D.of(-1, 1, 0),
>     Vector3D.of(0, 0, 2)
> };
>
> RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
>         .withVertexList(vertices)
>
>         // add the faces; these could alternatively be given as a 2d array
>         .addIndexedFacet(0, 1, 2, 3)
>         .addIndexedFacet(0, 4, 1)
>         .addIndexedFacet(1, 4, 2)
>         .addIndexedFacet(2, 4, 3)
>         .addIndexedFacet(3, 4, 0)
>         .build();
>
> System.out.println("# Pyramid" +
>         "\nbarycenter= " + region.getBarycenter() +
>         "\nvolume= " + region.getSize() +
>         "\nsurface area= " + region.getBoundarySize());
>
> // Create a unit box centered at the origin
> RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
>         .addCenteredCube(Vector3D.ZERO, 1)
>         .build();

I've had a quick look at "RegionBSPTree3D" as you suggested.

First questions (from a newbie POV):
* Do you intend to add more convenience methods to the
"Builder" (such as "addCube" and  "addCenteredCube")?  If not, shouldn't
the class be extensible (currently it is "final")?  If yes, where do we stop
("addSphere", "addPyramid", "addTorus", ...)?
* Wouldn't it be cleaner to separate "core" operations ("addFacet",
"addIndexedFacets", ...) from convenience methods that use them
('addCube", ...)?
* Then couldn't we define a more generic way to specify how to construct
all sorts of shapes?  E.g.:
---CUT---
public class RegionBSPTree3D {
    // ...
    public final class Builder {
        public Builder addFacets(List<List<Vector3D>> facets) {
            for (List<Vector3D> f : facets) {
                addFacet(f);
            }
        }
        public Builder add(FacetGenerator gen) {
            return addFacets(gen.build());
        }
        // ...
    }
    // ...
}

And e.g.
---CUT---
public classs RectangleGenerator implements FacetGenerator {
    /*
     * @param a first corner point in the rectangular prism (opposite
of {@code b})
     * @param b second corner point in the rectangular prism (opposite
of {@code a})
     */
    public RectangleGenerator(Vector3D a, Vector3D b) {
        // ...
    }

    @Override
    List<List<Vector3D>> build() {
        // ...
    }
}
---CUT---

WDYT?

Various nit-picks:

* Please put a space around operators:
---CUT---
for (int i=0; i<vertexIndices.length; ++i)
---CUT---
should be
---CUT---
for (int i = 0; i < vertexIndices.length; i++)
---CUT---

* Use "final" local variables wherever the reference won't be assigned a
new value.

* Use one condition per line:
---CUT---
if (precision.eq(minX, maxX) || precision.eq(minY, maxY) ||
precision.eq(minZ, maxZ))
---CUT---
should be
---CUT---
if (precision.eq(minX, maxX) ||
    precision.eq(minY, maxY) ||
    precision.eq(minZ, maxZ))
---CUT---


Best,
Gilles

> System.out.println("\n# Box" +
>         "\nbarycenter= " + box.getBarycenter() +
>         "\nvolume= " + box.getSize() +
>         "\nsurface area= " + box.getBoundarySize());
>
> // Move the box center to the pyramid center and lengthen it
> // along the z-axis
> AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
>         .translate(region.getBarycenter())
>         .scale(0.75, 0.75, 4);
>
> box.transform(mat);
>
> System.out.println("\n# Transformed Box" +
>         "\nbarycenter= " + box.getBarycenter() +
>         "\nvolume= " + box.getSize() +
>         "\nsurface area= " + box.getBoundarySize());
>
> // Subtract the box from the pyramid. The pyramid is modified but
> // the box is not.
> region.difference(box);
>
> System.out.println("\n# Pyramid with hole" +
>         "\nbarycenter= " + region.getBarycenter() +
>         "\nvolume= " + region.getSize() +
>         "\nsurface area= " + region.getBoundarySize());
>
> // Print out the vertices of the faces that make up the modified
> // pyramid. The faces are not simplified; they represent the internal
> // structure of the bsp tree.
> System.out.println("\n# Faces");
> for (ConvexSubPlane face : region.boundaries()) {
>     System.out.println(face.getVertices());
> }
>
> ________________________________
> From: Gilles Sadowski <[hidden email]>
> Sent: Friday, August 16, 2019 11:37 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello Matt.
>
> Thanks for continuing to work on this.
>
> Le jeu. 15 août 2019 à 17:56, Matt Juntunen
> <[hidden email]> a écrit :
> >
> > Hi all,
> >
> > I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.
>
> There is a spurious change of ".travis.yml".  Also there are wrong EOL
> characters
> in that file:
> ---CUT---
> $ file .travis.yml
> .travis.yml: ASCII text, with CRLF line terminators
> ---CUT---
> Perhaps you should merge or rebase on "master".
>
> > Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
> >
> >   *   More user-friendly API
> >      *   Does not require users to make unchecked casts to implementation types.
> >      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
> >      *   Uses builder pattern instead of large factory methods to build complicated geometries.
>
> That would suit me. ;-)
> Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
> Where should I start looking?
>
> >      *   Most classes are immutable.
> >   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
>
> Can it be associated with a surface element?
>
> >   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
> >   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
>
> Nice.
>
> >   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.
>
> Impressive.
>
> It would be great to create a maven module for systematizing the benchmarks;
> see e.g. what is being done in "Commons RNG":
>     https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh
>
> A "commons-geometry-examples" module would be a place to collect
> useful code, e.g. simple "howtos" (like the one I mentioned above) and
> conversion routines from/to popular formats, without the requirement of
> backwards compatibility (even between minor releases).
>
> Best,
> Gilles
>
> >
> > Regards,
> > Matt
> >
> > [1] https://github.com/apache/commons-geometry/pull/34
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Gilles Sadowski-2
Hello.

Le lun. 19 août 2019 à 05:14, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi Gilles,
>
> I did intend on adding more convenience methods for generating standard shapes but I definitely think we should abstract it like you're suggesting. Your design got me thinking and I believe we could take the idea of a FacetGenerator further and make it represent anything at all that can produce a sequence of facets,

+1

> be it a shape generator, a mesh, a 3D model file, a database table, a RegionBSPTree3D, etc. We could make full use of the streams API as well.

+1

> The main interface would look like this:
>
> public interface FacetSource {
>     Stream<ConvexSubPlane> facetStream();
> }

IIUC the code in "RegionBSPTree3D", a facet is a "List<Vector3D>" (thus
not a "ConvexSubPlane".  What am I missing?

>
> This would essentially perform the same task as your FacetGenerator, but it would allow facets to be streamed in one at a time instead of loaded into memory up front for each use. This would allow us to have code like this:
>
> RegionBSPTree3D tree = RegionBSPTree3D.builder(precision)
>     .add(new RectangleFacetSource(p1, p2))
>     .build();
>
> -- or --
>
> RegionBSPTree3D tree = RegionBSPTree3D.empty();
> new RectangleFacetSource(p1, p2).facetStream()
>    .map(f -> f.transform(transform))
>    .filter(f -> f.getPlane().getNormal().dot(vec) > 0)
>    .forEach(tree::insert);
>
> -- or even --
>
> RegionBSPTree3D tree;
> try (Stream<ConvexSubPlane> stream = new OBJFacetSource("model.obj").facetStream()) {
>     tree = stream.map(f -> f.transform(transform))
>              .collect(FacetCollectors.toTree());
> }
>
> What do you think of this slightly modified approach?

Looks neat.

Regards,
Gilles

>
> -Matt
>
> ________________________________
> From: Gilles Sadowski <[hidden email]>
> Sent: Saturday, August 17, 2019 6:00 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello.
>
> Le sam. 17 août 2019 à 04:58, Matt Juntunen
> <[hidden email]> a écrit :
> >
> > Hi Gilles,
> >
> >   1.  I just rebased from master so hopefully that fixed the repo weirdness.
>
> It's fixed indeed.
>
> >   2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes.
>
> Thanks for the pointer.
>
> > These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
> >   3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?
>
> I think that what I called surface element is the equivalent of "facet".
>
> >   4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.
>
> Great.
>
> >
> > -Matt
> >
> > DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);
> >
> > // Create a pyramid region with its base on the xy plane and its apex
> > // along the positive z axis. The base is 2 units to a side and the pyramid
> > // is 2 units high.
> > Vector3D[] vertices = {
> >     Vector3D.of(1, 1, 0),
> >     Vector3D.of(1, -1, 0),
> >     Vector3D.of(-1, -1, 0),
> >     Vector3D.of(-1, 1, 0),
> >     Vector3D.of(0, 0, 2)
> > };
> >
> > RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
> >         .withVertexList(vertices)
> >
> >         // add the faces; these could alternatively be given as a 2d array
> >         .addIndexedFacet(0, 1, 2, 3)
> >         .addIndexedFacet(0, 4, 1)
> >         .addIndexedFacet(1, 4, 2)
> >         .addIndexedFacet(2, 4, 3)
> >         .addIndexedFacet(3, 4, 0)
> >         .build();
> >
> > System.out.println("# Pyramid" +
> >         "\nbarycenter= " + region.getBarycenter() +
> >         "\nvolume= " + region.getSize() +
> >         "\nsurface area= " + region.getBoundarySize());
> >
> > // Create a unit box centered at the origin
> > RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
> >         .addCenteredCube(Vector3D.ZERO, 1)
> >         .build();
>
> I've had a quick look at "RegionBSPTree3D" as you suggested.
>
> First questions (from a newbie POV):
> * Do you intend to add more convenience methods to the
> "Builder" (such as "addCube" and  "addCenteredCube")?  If not, shouldn't
> the class be extensible (currently it is "final")?  If yes, where do we stop
> ("addSphere", "addPyramid", "addTorus", ...)?
> * Wouldn't it be cleaner to separate "core" operations ("addFacet",
> "addIndexedFacets", ...) from convenience methods that use them
> ('addCube", ...)?
> * Then couldn't we define a more generic way to specify how to construct
> all sorts of shapes?  E.g.:
> ---CUT---
> public class RegionBSPTree3D {
>     // ...
>     public final class Builder {
>         public Builder addFacets(List<List<Vector3D>> facets) {
>             for (List<Vector3D> f : facets) {
>                 addFacet(f);
>             }
>         }
>         public Builder add(FacetGenerator gen) {
>             return addFacets(gen.build());
>         }
>         // ...
>     }
>     // ...
> }
>
> And e.g.
> ---CUT---
> public classs RectangleGenerator implements FacetGenerator {
>     /*
>      * @param a first corner point in the rectangular prism (opposite
> of {@code b})
>      * @param b second corner point in the rectangular prism (opposite
> of {@code a})
>      */
>     public RectangleGenerator(Vector3D a, Vector3D b) {
>         // ...
>     }
>
>     @Override
>     List<List<Vector3D>> build() {
>         // ...
>     }
> }
> ---CUT---
>
> WDYT?
>
> Various nit-picks:
>
> * Please put a space around operators:
> ---CUT---
> for (int i=0; i<vertexIndices.length; ++i)
> ---CUT---
> should be
> ---CUT---
> for (int i = 0; i < vertexIndices.length; i++)
> ---CUT---
>
> * Use "final" local variables wherever the reference won't be assigned a
> new value.
>
> * Use one condition per line:
> ---CUT---
> if (precision.eq(minX, maxX) || precision.eq(minY, maxY) ||
> precision.eq(minZ, maxZ))
> ---CUT---
> should be
> ---CUT---
> if (precision.eq(minX, maxX) ||
>     precision.eq(minY, maxY) ||
>     precision.eq(minZ, maxZ))
> ---CUT---
>
>
> Best,
> Gilles
>
> > System.out.println("\n# Box" +
> >         "\nbarycenter= " + box.getBarycenter() +
> >         "\nvolume= " + box.getSize() +
> >         "\nsurface area= " + box.getBoundarySize());
> >
> > // Move the box center to the pyramid center and lengthen it
> > // along the z-axis
> > AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
> >         .translate(region.getBarycenter())
> >         .scale(0.75, 0.75, 4);
> >
> > box.transform(mat);
> >
> > System.out.println("\n# Transformed Box" +
> >         "\nbarycenter= " + box.getBarycenter() +
> >         "\nvolume= " + box.getSize() +
> >         "\nsurface area= " + box.getBoundarySize());
> >
> > // Subtract the box from the pyramid. The pyramid is modified but
> > // the box is not.
> > region.difference(box);
> >
> > System.out.println("\n# Pyramid with hole" +
> >         "\nbarycenter= " + region.getBarycenter() +
> >         "\nvolume= " + region.getSize() +
> >         "\nsurface area= " + region.getBoundarySize());
> >
> > // Print out the vertices of the faces that make up the modified
> > // pyramid. The faces are not simplified; they represent the internal
> > // structure of the bsp tree.
> > System.out.println("\n# Faces");
> > for (ConvexSubPlane face : region.boundaries()) {
> >     System.out.println(face.getVertices());
> > }
> >
> > ________________________________
> > From: Gilles Sadowski <[hidden email]>
> > Sent: Friday, August 16, 2019 11:37 AM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> >
> > Hello Matt.
> >
> > Thanks for continuing to work on this.
> >
> > Le jeu. 15 août 2019 à 17:56, Matt Juntunen
> > <[hidden email]> a écrit :
> > >
> > > Hi all,
> > >
> > > I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.
> >
> > There is a spurious change of ".travis.yml".  Also there are wrong EOL
> > characters
> > in that file:
> > ---CUT---
> > $ file .travis.yml
> > .travis.yml: ASCII text, with CRLF line terminators
> > ---CUT---
> > Perhaps you should merge or rebase on "master".
> >
> > > Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
> > >
> > >   *   More user-friendly API
> > >      *   Does not require users to make unchecked casts to implementation types.
> > >      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
> > >      *   Uses builder pattern instead of large factory methods to build complicated geometries.
> >
> > That would suit me. ;-)
> > Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
> > Where should I start looking?
> >
> > >      *   Most classes are immutable.
> > >   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
> >
> > Can it be associated with a surface element?
> >
> > >   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
> > >   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
> >
> > Nice.
> >
> > >   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.
> >
> > Impressive.
> >
> > It would be great to create a maven module for systematizing the benchmarks;
> > see e.g. what is being done in "Commons RNG":
> >     https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh
> >
> > A "commons-geometry-examples" module would be a place to collect
> > useful code, e.g. simple "howtos" (like the one I mentioned above) and
> > conversion routines from/to popular formats, without the requirement of
> > backwards compatibility (even between minor releases).
> >
> > Best,
> > Gilles
> >
> > >
> > > Regards,
> > > Matt
> > >
> > > [1] https://github.com/apache/commons-geometry/pull/34
> >

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Matt Juntunen
Gilles,

ConvexSubPlane is the main class representing 3D facets. There are convenience methods in the RegionBSPTree3D.Builder class for adding facets directly from vertices since that's the most common way to represent them. These methods ultimately end up creating ConvexSubPlane instances, either directly via ConvexSubPlane.fromVertexLoop() or indirectly via SubPlane.toConvex().

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Tuesday, August 20, 2019 8:58 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] GEOMTRY-32 Feedback Requested

Hello.

Le lun. 19 août 2019 à 05:14, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi Gilles,
>
> I did intend on adding more convenience methods for generating standard shapes but I definitely think we should abstract it like you're suggesting. Your design got me thinking and I believe we could take the idea of a FacetGenerator further and make it represent anything at all that can produce a sequence of facets,

+1

> be it a shape generator, a mesh, a 3D model file, a database table, a RegionBSPTree3D, etc. We could make full use of the streams API as well.

+1

> The main interface would look like this:
>
> public interface FacetSource {
>     Stream<ConvexSubPlane> facetStream();
> }

IIUC the code in "RegionBSPTree3D", a facet is a "List<Vector3D>" (thus
not a "ConvexSubPlane".  What am I missing?

>
> This would essentially perform the same task as your FacetGenerator, but it would allow facets to be streamed in one at a time instead of loaded into memory up front for each use. This would allow us to have code like this:
>
> RegionBSPTree3D tree = RegionBSPTree3D.builder(precision)
>     .add(new RectangleFacetSource(p1, p2))
>     .build();
>
> -- or --
>
> RegionBSPTree3D tree = RegionBSPTree3D.empty();
> new RectangleFacetSource(p1, p2).facetStream()
>    .map(f -> f.transform(transform))
>    .filter(f -> f.getPlane().getNormal().dot(vec) > 0)
>    .forEach(tree::insert);
>
> -- or even --
>
> RegionBSPTree3D tree;
> try (Stream<ConvexSubPlane> stream = new OBJFacetSource("model.obj").facetStream()) {
>     tree = stream.map(f -> f.transform(transform))
>              .collect(FacetCollectors.toTree());
> }
>
> What do you think of this slightly modified approach?

Looks neat.

Regards,
Gilles

>
> -Matt
>
> ________________________________
> From: Gilles Sadowski <[hidden email]>
> Sent: Saturday, August 17, 2019 6:00 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello.
>
> Le sam. 17 août 2019 à 04:58, Matt Juntunen
> <[hidden email]> a écrit :
> >
> > Hi Gilles,
> >
> >   1.  I just rebased from master so hopefully that fixed the repo weirdness.
>
> It's fixed indeed.
>
> >   2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes.
>
> Thanks for the pointer.
>
> > These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
> >   3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?
>
> I think that what I called surface element is the equivalent of "facet".
>
> >   4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.
>
> Great.
>
> >
> > -Matt
> >
> > DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);
> >
> > // Create a pyramid region with its base on the xy plane and its apex
> > // along the positive z axis. The base is 2 units to a side and the pyramid
> > // is 2 units high.
> > Vector3D[] vertices = {
> >     Vector3D.of(1, 1, 0),
> >     Vector3D.of(1, -1, 0),
> >     Vector3D.of(-1, -1, 0),
> >     Vector3D.of(-1, 1, 0),
> >     Vector3D.of(0, 0, 2)
> > };
> >
> > RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
> >         .withVertexList(vertices)
> >
> >         // add the faces; these could alternatively be given as a 2d array
> >         .addIndexedFacet(0, 1, 2, 3)
> >         .addIndexedFacet(0, 4, 1)
> >         .addIndexedFacet(1, 4, 2)
> >         .addIndexedFacet(2, 4, 3)
> >         .addIndexedFacet(3, 4, 0)
> >         .build();
> >
> > System.out.println("# Pyramid" +
> >         "\nbarycenter= " + region.getBarycenter() +
> >         "\nvolume= " + region.getSize() +
> >         "\nsurface area= " + region.getBoundarySize());
> >
> > // Create a unit box centered at the origin
> > RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
> >         .addCenteredCube(Vector3D.ZERO, 1)
> >         .build();
>
> I've had a quick look at "RegionBSPTree3D" as you suggested.
>
> First questions (from a newbie POV):
> * Do you intend to add more convenience methods to the
> "Builder" (such as "addCube" and  "addCenteredCube")?  If not, shouldn't
> the class be extensible (currently it is "final")?  If yes, where do we stop
> ("addSphere", "addPyramid", "addTorus", ...)?
> * Wouldn't it be cleaner to separate "core" operations ("addFacet",
> "addIndexedFacets", ...) from convenience methods that use them
> ('addCube", ...)?
> * Then couldn't we define a more generic way to specify how to construct
> all sorts of shapes?  E.g.:
> ---CUT---
> public class RegionBSPTree3D {
>     // ...
>     public final class Builder {
>         public Builder addFacets(List<List<Vector3D>> facets) {
>             for (List<Vector3D> f : facets) {
>                 addFacet(f);
>             }
>         }
>         public Builder add(FacetGenerator gen) {
>             return addFacets(gen.build());
>         }
>         // ...
>     }
>     // ...
> }
>
> And e.g.
> ---CUT---
> public classs RectangleGenerator implements FacetGenerator {
>     /*
>      * @param a first corner point in the rectangular prism (opposite
> of {@code b})
>      * @param b second corner point in the rectangular prism (opposite
> of {@code a})
>      */
>     public RectangleGenerator(Vector3D a, Vector3D b) {
>         // ...
>     }
>
>     @Override
>     List<List<Vector3D>> build() {
>         // ...
>     }
> }
> ---CUT---
>
> WDYT?
>
> Various nit-picks:
>
> * Please put a space around operators:
> ---CUT---
> for (int i=0; i<vertexIndices.length; ++i)
> ---CUT---
> should be
> ---CUT---
> for (int i = 0; i < vertexIndices.length; i++)
> ---CUT---
>
> * Use "final" local variables wherever the reference won't be assigned a
> new value.
>
> * Use one condition per line:
> ---CUT---
> if (precision.eq(minX, maxX) || precision.eq(minY, maxY) ||
> precision.eq(minZ, maxZ))
> ---CUT---
> should be
> ---CUT---
> if (precision.eq(minX, maxX) ||
>     precision.eq(minY, maxY) ||
>     precision.eq(minZ, maxZ))
> ---CUT---
>
>
> Best,
> Gilles
>
> > System.out.println("\n# Box" +
> >         "\nbarycenter= " + box.getBarycenter() +
> >         "\nvolume= " + box.getSize() +
> >         "\nsurface area= " + box.getBoundarySize());
> >
> > // Move the box center to the pyramid center and lengthen it
> > // along the z-axis
> > AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
> >         .translate(region.getBarycenter())
> >         .scale(0.75, 0.75, 4);
> >
> > box.transform(mat);
> >
> > System.out.println("\n# Transformed Box" +
> >         "\nbarycenter= " + box.getBarycenter() +
> >         "\nvolume= " + box.getSize() +
> >         "\nsurface area= " + box.getBoundarySize());
> >
> > // Subtract the box from the pyramid. The pyramid is modified but
> > // the box is not.
> > region.difference(box);
> >
> > System.out.println("\n# Pyramid with hole" +
> >         "\nbarycenter= " + region.getBarycenter() +
> >         "\nvolume= " + region.getSize() +
> >         "\nsurface area= " + region.getBoundarySize());
> >
> > // Print out the vertices of the faces that make up the modified
> > // pyramid. The faces are not simplified; they represent the internal
> > // structure of the bsp tree.
> > System.out.println("\n# Faces");
> > for (ConvexSubPlane face : region.boundaries()) {
> >     System.out.println(face.getVertices());
> > }
> >
> > ________________________________
> > From: Gilles Sadowski <[hidden email]>
> > Sent: Friday, August 16, 2019 11:37 AM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> >
> > Hello Matt.
> >
> > Thanks for continuing to work on this.
> >
> > Le jeu. 15 août 2019 à 17:56, Matt Juntunen
> > <[hidden email]> a écrit :
> > >
> > > Hi all,
> > >
> > > I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.
> >
> > There is a spurious change of ".travis.yml".  Also there are wrong EOL
> > characters
> > in that file:
> > ---CUT---
> > $ file .travis.yml
> > .travis.yml: ASCII text, with CRLF line terminators
> > ---CUT---
> > Perhaps you should merge or rebase on "master".
> >
> > > Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
> > >
> > >   *   More user-friendly API
> > >      *   Does not require users to make unchecked casts to implementation types.
> > >      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
> > >      *   Uses builder pattern instead of large factory methods to build complicated geometries.
> >
> > That would suit me. ;-)
> > Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
> > Where should I start looking?
> >
> > >      *   Most classes are immutable.
> > >   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
> >
> > Can it be associated with a surface element?
> >
> > >   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
> > >   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
> >
> > Nice.
> >
> > >   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.
> >
> > Impressive.
> >
> > It would be great to create a maven module for systematizing the benchmarks;
> > see e.g. what is being done in "Commons RNG":
> >     https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh
> >
> > A "commons-geometry-examples" module would be a place to collect
> > useful code, e.g. simple "howtos" (like the one I mentioned above) and
> > conversion routines from/to popular formats, without the requirement of
> > backwards compatibility (even between minor releases).
> >
> > Best,
> > Gilles
> >
> > >
> > > Regards,
> > > Matt
> > >
> > > [1] https://github.com/apache/commons-geometry/pull/34
> >

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Gilles Sadowski-2
 Hi.

Le mer. 21 août 2019 à 04:42, Matt Juntunen
<[hidden email]> a écrit :
>
> Gilles,
>
> ConvexSubPlane is the main class representing 3D facets. There are convenience methods in the RegionBSPTree3D.Builder class for adding facets directly from vertices since that's the most common way to represent them. These methods ultimately end up creating ConvexSubPlane instances, either directly via ConvexSubPlane.fromVertexLoop() or indirectly via SubPlane.toConvex().

In the "FacetSource" interface, shouldn't the method be called just "stream()",
instead of "facetStream()"?

Shouldn't some converter objects be defined?  E.g.
---CUT---
public VertexListToConvexSubPlane
    implements java.util.function.Function<Collection<Vector3D>,
ConvexSubPlane> {
    private final DoublePrecisionContext prec;
    private final boolean makeLoop;

    public VertexListToConvexSubPlane(DoublePrecisionContext prec,
boolean makeLoop) {
        this.prec = prec;
        this.makeLoop = makeLoop;
    }

    @Override
    public ConvexSubPlane apply(Collection<Vector3D> vertices) {
        return ConvexSubPlane.fromVertices(vertices, prec, makeLoop);
    }
}
---CUT---

Sidenote: I don't understand the usefulness of having defined two methods
"fromVertexLoop" and "fromVertices" only to hide one flag (that is actually
"moved" into the method's name).

Regards,
Gilles

>
> -Matt
> ________________________________
> From: Gilles Sadowski <[hidden email]>
> Sent: Tuesday, August 20, 2019 8:58 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello.
>
> Le lun. 19 août 2019 à 05:14, Matt Juntunen
> <[hidden email]> a écrit :
> >
> > Hi Gilles,
> >
> > I did intend on adding more convenience methods for generating standard shapes but I definitely think we should abstract it like you're suggesting. Your design got me thinking and I believe we could take the idea of a FacetGenerator further and make it represent anything at all that can produce a sequence of facets,
>
> +1
>
> > be it a shape generator, a mesh, a 3D model file, a database table, a RegionBSPTree3D, etc. We could make full use of the streams API as well.
>
> +1
>
> > The main interface would look like this:
> >
> > public interface FacetSource {
> >     Stream<ConvexSubPlane> facetStream();
> > }
>
> IIUC the code in "RegionBSPTree3D", a facet is a "List<Vector3D>" (thus
> not a "ConvexSubPlane".  What am I missing?
>
> >
> > This would essentially perform the same task as your FacetGenerator, but it would allow facets to be streamed in one at a time instead of loaded into memory up front for each use. This would allow us to have code like this:
> >
> > RegionBSPTree3D tree = RegionBSPTree3D.builder(precision)
> >     .add(new RectangleFacetSource(p1, p2))
> >     .build();
> >
> > -- or --
> >
> > RegionBSPTree3D tree = RegionBSPTree3D.empty();
> > new RectangleFacetSource(p1, p2).facetStream()
> >    .map(f -> f.transform(transform))
> >    .filter(f -> f.getPlane().getNormal().dot(vec) > 0)
> >    .forEach(tree::insert);
> >
> > -- or even --
> >
> > RegionBSPTree3D tree;
> > try (Stream<ConvexSubPlane> stream = new OBJFacetSource("model.obj").facetStream()) {
> >     tree = stream.map(f -> f.transform(transform))
> >              .collect(FacetCollectors.toTree());
> > }
> >
> > What do you think of this slightly modified approach?
>
> Looks neat.
>
> Regards,
> Gilles
>
> >
> > -Matt
> >
> > ________________________________
> > From: Gilles Sadowski <[hidden email]>
> > Sent: Saturday, August 17, 2019 6:00 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> >
> > Hello.
> >
> > Le sam. 17 août 2019 à 04:58, Matt Juntunen
> > <[hidden email]> a écrit :
> > >
> > > Hi Gilles,
> > >
> > >   1.  I just rebased from master so hopefully that fixed the repo weirdness.
> >
> > It's fixed indeed.
> >
> > >   2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes.
> >
> > Thanks for the pointer.
> >
> > > These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
> > >   3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?
> >
> > I think that what I called surface element is the equivalent of "facet".
> >
> > >   4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.
> >
> > Great.
> >
> > >
> > > -Matt
> > >
> > > DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);
> > >
> > > // Create a pyramid region with its base on the xy plane and its apex
> > > // along the positive z axis. The base is 2 units to a side and the pyramid
> > > // is 2 units high.
> > > Vector3D[] vertices = {
> > >     Vector3D.of(1, 1, 0),
> > >     Vector3D.of(1, -1, 0),
> > >     Vector3D.of(-1, -1, 0),
> > >     Vector3D.of(-1, 1, 0),
> > >     Vector3D.of(0, 0, 2)
> > > };
> > >
> > > RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
> > >         .withVertexList(vertices)
> > >
> > >         // add the faces; these could alternatively be given as a 2d array
> > >         .addIndexedFacet(0, 1, 2, 3)
> > >         .addIndexedFacet(0, 4, 1)
> > >         .addIndexedFacet(1, 4, 2)
> > >         .addIndexedFacet(2, 4, 3)
> > >         .addIndexedFacet(3, 4, 0)
> > >         .build();
> > >
> > > System.out.println("# Pyramid" +
> > >         "\nbarycenter= " + region.getBarycenter() +
> > >         "\nvolume= " + region.getSize() +
> > >         "\nsurface area= " + region.getBoundarySize());
> > >
> > > // Create a unit box centered at the origin
> > > RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
> > >         .addCenteredCube(Vector3D.ZERO, 1)
> > >         .build();
> >
> > I've had a quick look at "RegionBSPTree3D" as you suggested.
> >
> > First questions (from a newbie POV):
> > * Do you intend to add more convenience methods to the
> > "Builder" (such as "addCube" and  "addCenteredCube")?  If not, shouldn't
> > the class be extensible (currently it is "final")?  If yes, where do we stop
> > ("addSphere", "addPyramid", "addTorus", ...)?
> > * Wouldn't it be cleaner to separate "core" operations ("addFacet",
> > "addIndexedFacets", ...) from convenience methods that use them
> > ('addCube", ...)?
> > * Then couldn't we define a more generic way to specify how to construct
> > all sorts of shapes?  E.g.:
> > ---CUT---
> > public class RegionBSPTree3D {
> >     // ...
> >     public final class Builder {
> >         public Builder addFacets(List<List<Vector3D>> facets) {
> >             for (List<Vector3D> f : facets) {
> >                 addFacet(f);
> >             }
> >         }
> >         public Builder add(FacetGenerator gen) {
> >             return addFacets(gen.build());
> >         }
> >         // ...
> >     }
> >     // ...
> > }
> >
> > And e.g.
> > ---CUT---
> > public classs RectangleGenerator implements FacetGenerator {
> >     /*
> >      * @param a first corner point in the rectangular prism (opposite
> > of {@code b})
> >      * @param b second corner point in the rectangular prism (opposite
> > of {@code a})
> >      */
> >     public RectangleGenerator(Vector3D a, Vector3D b) {
> >         // ...
> >     }
> >
> >     @Override
> >     List<List<Vector3D>> build() {
> >         // ...
> >     }
> > }
> > ---CUT---
> >
> > WDYT?
> >
> > Various nit-picks:
> >
> > * Please put a space around operators:
> > ---CUT---
> > for (int i=0; i<vertexIndices.length; ++i)
> > ---CUT---
> > should be
> > ---CUT---
> > for (int i = 0; i < vertexIndices.length; i++)
> > ---CUT---
> >
> > * Use "final" local variables wherever the reference won't be assigned a
> > new value.
> >
> > * Use one condition per line:
> > ---CUT---
> > if (precision.eq(minX, maxX) || precision.eq(minY, maxY) ||
> > precision.eq(minZ, maxZ))
> > ---CUT---
> > should be
> > ---CUT---
> > if (precision.eq(minX, maxX) ||
> >     precision.eq(minY, maxY) ||
> >     precision.eq(minZ, maxZ))
> > ---CUT---
> >
> >
> > Best,
> > Gilles
> >
> > > System.out.println("\n# Box" +
> > >         "\nbarycenter= " + box.getBarycenter() +
> > >         "\nvolume= " + box.getSize() +
> > >         "\nsurface area= " + box.getBoundarySize());
> > >
> > > // Move the box center to the pyramid center and lengthen it
> > > // along the z-axis
> > > AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
> > >         .translate(region.getBarycenter())
> > >         .scale(0.75, 0.75, 4);
> > >
> > > box.transform(mat);
> > >
> > > System.out.println("\n# Transformed Box" +
> > >         "\nbarycenter= " + box.getBarycenter() +
> > >         "\nvolume= " + box.getSize() +
> > >         "\nsurface area= " + box.getBoundarySize());
> > >
> > > // Subtract the box from the pyramid. The pyramid is modified but
> > > // the box is not.
> > > region.difference(box);
> > >
> > > System.out.println("\n# Pyramid with hole" +
> > >         "\nbarycenter= " + region.getBarycenter() +
> > >         "\nvolume= " + region.getSize() +
> > >         "\nsurface area= " + region.getBoundarySize());
> > >
> > > // Print out the vertices of the faces that make up the modified
> > > // pyramid. The faces are not simplified; they represent the internal
> > > // structure of the bsp tree.
> > > System.out.println("\n# Faces");
> > > for (ConvexSubPlane face : region.boundaries()) {
> > >     System.out.println(face.getVertices());
> > > }
> > >
> > > ________________________________
> > > From: Gilles Sadowski <[hidden email]>
> > > Sent: Friday, August 16, 2019 11:37 AM
> > > To: Commons Developers List <[hidden email]>
> > > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> > >
> > > Hello Matt.
> > >
> > > Thanks for continuing to work on this.
> > >
> > > Le jeu. 15 août 2019 à 17:56, Matt Juntunen
> > > <[hidden email]> a écrit :
> > > >
> > > > Hi all,
> > > >
> > > > I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.
> > >
> > > There is a spurious change of ".travis.yml".  Also there are wrong EOL
> > > characters
> > > in that file:
> > > ---CUT---
> > > $ file .travis.yml
> > > .travis.yml: ASCII text, with CRLF line terminators
> > > ---CUT---
> > > Perhaps you should merge or rebase on "master".
> > >
> > > > Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
> > > >
> > > >   *   More user-friendly API
> > > >      *   Does not require users to make unchecked casts to implementation types.
> > > >      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
> > > >      *   Uses builder pattern instead of large factory methods to build complicated geometries.
> > >
> > > That would suit me. ;-)
> > > Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
> > > Where should I start looking?
> > >
> > > >      *   Most classes are immutable.
> > > >   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
> > >
> > > Can it be associated with a surface element?
> > >
> > > >   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
> > > >   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
> > >
> > > Nice.
> > >
> > > >   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.
> > >
> > > Impressive.
> > >
> > > It would be great to create a maven module for systematizing the benchmarks;
> > > see e.g. what is being done in "Commons RNG":
> > >     https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh
> > >
> > > A "commons-geometry-examples" module would be a place to collect
> > > useful code, e.g. simple "howtos" (like the one I mentioned above) and
> > > conversion routines from/to popular formats, without the requirement of
> > > backwards compatibility (even between minor releases).
> > >
> > > Best,
> > > Gilles
> > >
> > > >
> > > > Regards,
> > > > Matt
> > > >
> > > > [1] https://github.com/apache/commons-geometry/pull/34
> > >

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Matt Juntunen
Hi Gilles,

I like the idea of modifying the getVertices()/getVectorLoop() methods. Perhaps we should move this discussion to JIRA so we can track it easier?

-Matt
________________________________
From: Gilles Sadowski <[hidden email]>
Sent: Wednesday, August 21, 2019 7:49 AM
To: Commons Developers List <[hidden email]>
Subject: Re: [geometry] GEOMTRY-32 Feedback Requested

Hi.

Le mer. 21 août 2019 à 04:42, Matt Juntunen
<[hidden email]> a écrit :
>
> Gilles,
>
> ConvexSubPlane is the main class representing 3D facets. There are convenience methods in the RegionBSPTree3D.Builder class for adding facets directly from vertices since that's the most common way to represent them. These methods ultimately end up creating ConvexSubPlane instances, either directly via ConvexSubPlane.fromVertexLoop() or indirectly via SubPlane.toConvex().

In the "FacetSource" interface, shouldn't the method be called just "stream()",
instead of "facetStream()"?

Shouldn't some converter objects be defined?  E.g.
---CUT---
public VertexListToConvexSubPlane
    implements java.util.function.Function<Collection<Vector3D>,
ConvexSubPlane> {
    private final DoublePrecisionContext prec;
    private final boolean makeLoop;

    public VertexListToConvexSubPlane(DoublePrecisionContext prec,
boolean makeLoop) {
        this.prec = prec;
        this.makeLoop = makeLoop;
    }

    @Override
    public ConvexSubPlane apply(Collection<Vector3D> vertices) {
        return ConvexSubPlane.fromVertices(vertices, prec, makeLoop);
    }
}
---CUT---

Sidenote: I don't understand the usefulness of having defined two methods
"fromVertexLoop" and "fromVertices" only to hide one flag (that is actually
"moved" into the method's name).

Regards,
Gilles

>
> -Matt
> ________________________________
> From: Gilles Sadowski <[hidden email]>
> Sent: Tuesday, August 20, 2019 8:58 AM
> To: Commons Developers List <[hidden email]>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello.
>
> Le lun. 19 août 2019 à 05:14, Matt Juntunen
> <[hidden email]> a écrit :
> >
> > Hi Gilles,
> >
> > I did intend on adding more convenience methods for generating standard shapes but I definitely think we should abstract it like you're suggesting. Your design got me thinking and I believe we could take the idea of a FacetGenerator further and make it represent anything at all that can produce a sequence of facets,
>
> +1
>
> > be it a shape generator, a mesh, a 3D model file, a database table, a RegionBSPTree3D, etc. We could make full use of the streams API as well.
>
> +1
>
> > The main interface would look like this:
> >
> > public interface FacetSource {
> >     Stream<ConvexSubPlane> facetStream();
> > }
>
> IIUC the code in "RegionBSPTree3D", a facet is a "List<Vector3D>" (thus
> not a "ConvexSubPlane".  What am I missing?
>
> >
> > This would essentially perform the same task as your FacetGenerator, but it would allow facets to be streamed in one at a time instead of loaded into memory up front for each use. This would allow us to have code like this:
> >
> > RegionBSPTree3D tree = RegionBSPTree3D.builder(precision)
> >     .add(new RectangleFacetSource(p1, p2))
> >     .build();
> >
> > -- or --
> >
> > RegionBSPTree3D tree = RegionBSPTree3D.empty();
> > new RectangleFacetSource(p1, p2).facetStream()
> >    .map(f -> f.transform(transform))
> >    .filter(f -> f.getPlane().getNormal().dot(vec) > 0)
> >    .forEach(tree::insert);
> >
> > -- or even --
> >
> > RegionBSPTree3D tree;
> > try (Stream<ConvexSubPlane> stream = new OBJFacetSource("model.obj").facetStream()) {
> >     tree = stream.map(f -> f.transform(transform))
> >              .collect(FacetCollectors.toTree());
> > }
> >
> > What do you think of this slightly modified approach?
>
> Looks neat.
>
> Regards,
> Gilles
>
> >
> > -Matt
> >
> > ________________________________
> > From: Gilles Sadowski <[hidden email]>
> > Sent: Saturday, August 17, 2019 6:00 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> >
> > Hello.
> >
> > Le sam. 17 août 2019 à 04:58, Matt Juntunen
> > <[hidden email]> a écrit :
> > >
> > > Hi Gilles,
> > >
> > >   1.  I just rebased from master so hopefully that fixed the repo weirdness.
> >
> > It's fixed indeed.
> >
> > >   2.  The best place to start working with the new code is probably the RegionBSPTreeXD classes.
> >
> > Thanks for the pointer.
> >
> > > These are the replacements for the old IntervalsSet, PolygonsSet, and PolyhedronsSet classes. I'm including a short example at the end of this email that shows some of the functionality.
> > >   3.  Anything could be stored in the AttributeBSPTree nodes, but there isn't specific functionality for computing the surface of portions of the tree, since that requires the concept of inside and outside nodes, which is present in the RegionBSPTree subclasses but not here. Is that what you were asking?
> >
> > I think that what I called surface element is the equivalent of "facet".
> >
> > >   4.  I was picturing adding both an examples module and a benchmark module a littler later, along with a user guide.
> >
> > Great.
> >
> > >
> > > -Matt
> > >
> > > DoublePrecisionContext precision = new EpsilonDoublePrecisionContext(1e-10);
> > >
> > > // Create a pyramid region with its base on the xy plane and its apex
> > > // along the positive z axis. The base is 2 units to a side and the pyramid
> > > // is 2 units high.
> > > Vector3D[] vertices = {
> > >     Vector3D.of(1, 1, 0),
> > >     Vector3D.of(1, -1, 0),
> > >     Vector3D.of(-1, -1, 0),
> > >     Vector3D.of(-1, 1, 0),
> > >     Vector3D.of(0, 0, 2)
> > > };
> > >
> > > RegionBSPTree3D region = RegionBSPTree3D.builder(precision)
> > >         .withVertexList(vertices)
> > >
> > >         // add the faces; these could alternatively be given as a 2d array
> > >         .addIndexedFacet(0, 1, 2, 3)
> > >         .addIndexedFacet(0, 4, 1)
> > >         .addIndexedFacet(1, 4, 2)
> > >         .addIndexedFacet(2, 4, 3)
> > >         .addIndexedFacet(3, 4, 0)
> > >         .build();
> > >
> > > System.out.println("# Pyramid" +
> > >         "\nbarycenter= " + region.getBarycenter() +
> > >         "\nvolume= " + region.getSize() +
> > >         "\nsurface area= " + region.getBoundarySize());
> > >
> > > // Create a unit box centered at the origin
> > > RegionBSPTree3D box = RegionBSPTree3D.builder(precision)
> > >         .addCenteredCube(Vector3D.ZERO, 1)
> > >         .build();
> >
> > I've had a quick look at "RegionBSPTree3D" as you suggested.
> >
> > First questions (from a newbie POV):
> > * Do you intend to add more convenience methods to the
> > "Builder" (such as "addCube" and  "addCenteredCube")?  If not, shouldn't
> > the class be extensible (currently it is "final")?  If yes, where do we stop
> > ("addSphere", "addPyramid", "addTorus", ...)?
> > * Wouldn't it be cleaner to separate "core" operations ("addFacet",
> > "addIndexedFacets", ...) from convenience methods that use them
> > ('addCube", ...)?
> > * Then couldn't we define a more generic way to specify how to construct
> > all sorts of shapes?  E.g.:
> > ---CUT---
> > public class RegionBSPTree3D {
> >     // ...
> >     public final class Builder {
> >         public Builder addFacets(List<List<Vector3D>> facets) {
> >             for (List<Vector3D> f : facets) {
> >                 addFacet(f);
> >             }
> >         }
> >         public Builder add(FacetGenerator gen) {
> >             return addFacets(gen.build());
> >         }
> >         // ...
> >     }
> >     // ...
> > }
> >
> > And e.g.
> > ---CUT---
> > public classs RectangleGenerator implements FacetGenerator {
> >     /*
> >      * @param a first corner point in the rectangular prism (opposite
> > of {@code b})
> >      * @param b second corner point in the rectangular prism (opposite
> > of {@code a})
> >      */
> >     public RectangleGenerator(Vector3D a, Vector3D b) {
> >         // ...
> >     }
> >
> >     @Override
> >     List<List<Vector3D>> build() {
> >         // ...
> >     }
> > }
> > ---CUT---
> >
> > WDYT?
> >
> > Various nit-picks:
> >
> > * Please put a space around operators:
> > ---CUT---
> > for (int i=0; i<vertexIndices.length; ++i)
> > ---CUT---
> > should be
> > ---CUT---
> > for (int i = 0; i < vertexIndices.length; i++)
> > ---CUT---
> >
> > * Use "final" local variables wherever the reference won't be assigned a
> > new value.
> >
> > * Use one condition per line:
> > ---CUT---
> > if (precision.eq(minX, maxX) || precision.eq(minY, maxY) ||
> > precision.eq(minZ, maxZ))
> > ---CUT---
> > should be
> > ---CUT---
> > if (precision.eq(minX, maxX) ||
> >     precision.eq(minY, maxY) ||
> >     precision.eq(minZ, maxZ))
> > ---CUT---
> >
> >
> > Best,
> > Gilles
> >
> > > System.out.println("\n# Box" +
> > >         "\nbarycenter= " + box.getBarycenter() +
> > >         "\nvolume= " + box.getSize() +
> > >         "\nsurface area= " + box.getBoundarySize());
> > >
> > > // Move the box center to the pyramid center and lengthen it
> > > // along the z-axis
> > > AffineTransformMatrix3D mat = AffineTransformMatrix3D.identity()
> > >         .translate(region.getBarycenter())
> > >         .scale(0.75, 0.75, 4);
> > >
> > > box.transform(mat);
> > >
> > > System.out.println("\n# Transformed Box" +
> > >         "\nbarycenter= " + box.getBarycenter() +
> > >         "\nvolume= " + box.getSize() +
> > >         "\nsurface area= " + box.getBoundarySize());
> > >
> > > // Subtract the box from the pyramid. The pyramid is modified but
> > > // the box is not.
> > > region.difference(box);
> > >
> > > System.out.println("\n# Pyramid with hole" +
> > >         "\nbarycenter= " + region.getBarycenter() +
> > >         "\nvolume= " + region.getSize() +
> > >         "\nsurface area= " + region.getBoundarySize());
> > >
> > > // Print out the vertices of the faces that make up the modified
> > > // pyramid. The faces are not simplified; they represent the internal
> > > // structure of the bsp tree.
> > > System.out.println("\n# Faces");
> > > for (ConvexSubPlane face : region.boundaries()) {
> > >     System.out.println(face.getVertices());
> > > }
> > >
> > > ________________________________
> > > From: Gilles Sadowski <[hidden email]>
> > > Sent: Friday, August 16, 2019 11:37 AM
> > > To: Commons Developers List <[hidden email]>
> > > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> > >
> > > Hello Matt.
> > >
> > > Thanks for continuing to work on this.
> > >
> > > Le jeu. 15 août 2019 à 17:56, Matt Juntunen
> > > <[hidden email]> a écrit :
> > > >
> > > > Hi all,
> > > >
> > > > I've been working on the BSP tree refactor and general API cleanup for a while now and I finally have the core and Euclidean classes complete. I have the code in a draft PR on github [1] and I'm hoping to get as much feedback on it as I can.
> > >
> > > There is a spurious change of ".travis.yml".  Also there are wrong EOL
> > > characters
> > > in that file:
> > > ---CUT---
> > > $ file .travis.yml
> > > .travis.yml: ASCII text, with CRLF line terminators
> > > ---CUT---
> > > Perhaps you should merge or rebase on "master".
> > >
> > > > Everything is in its final state from my point of view with the exception of the spherical module, which still needs to be switched over to the new API. Here are some highlights of the new code:
> > > >
> > > >   *   More user-friendly API
> > > >      *   Does not require users to make unchecked casts to implementation types.
> > > >      *   BSP tree classes are much more powerful and easy to use. State is managed internally so that users do not need to be experts in BSP trees to avoid corrupting the data structures.
> > > >      *   Uses builder pattern instead of large factory methods to build complicated geometries.
> > >
> > > That would suit me. ;-)
> > > Actually, I'd like to try it with a simple geometry (i.e. build a sphere).
> > > Where should I start looking?
> > >
> > > >      *   Most classes are immutable.
> > > >   *   A general-purpose AttributeBSPTree class is available so that users can associate arbitrary data with spatial partitionings. I'm picturing this being used for spatial data lookups, painter's algorithms, etc.
> > >
> > > Can it be associated with a surface element?
> > >
> > > >   *   All geometric types now support arbitrary transforms (eg, rotate, scale, translate, etc) via the Transform interface.
> > > >   *   The Transform interface is greatly simplified (GEOMETRY-24). It is now functional and simply extends java.util.Function.
> > >
> > > Nice.
> > >
> > > >   *   Better performance. My highly unsophisticated stdout benchmarking put the new code at about 3-4 times faster than the old when performing boolean operations on 3D regions.
> > >
> > > Impressive.
> > >
> > > It would be great to create a maven module for systematizing the benchmarks;
> > > see e.g. what is being done in "Commons RNG":
> > >     https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jmh
> > >
> > > A "commons-geometry-examples" module would be a place to collect
> > > useful code, e.g. simple "howtos" (like the one I mentioned above) and
> > > conversion routines from/to popular formats, without the requirement of
> > > backwards compatibility (even between minor releases).
> > >
> > > Best,
> > > Gilles
> > >
> > > >
> > > > Regards,
> > > > Matt
> > > >
> > > > [1] https://github.com/apache/commons-geometry/pull/34
> > >

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [geometry] GEOMTRY-32 Feedback Requested

Gilles Sadowski-2
Hello.

Le dim. 25 août 2019 à 18:08, Matt Juntunen
<[hidden email]> a écrit :
>
> Hi Gilles,
>
> I like the idea of modifying the getVertices()/getVectorLoop() methods. Perhaps we should move this discussion to JIRA so we can track it easier?

Fine with me.

Gilles

> > > [...]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]