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 <gillese...@gmail.com>
Sent: Wednesday, August 21, 2019 7:49 AM
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
Hi.
Le mer. 21 août 2019 à 04:42, Matt Juntunen
<matt.juntu...@hotmail.com> 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 <gillese...@gmail.com>
> Sent: Tuesday, August 20, 2019 8:58 AM
> To: Commons Developers List <dev@commons.apache.org>
> Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
>
> Hello.
>
> Le lun. 19 août 2019 à 05:14, Matt Juntunen
> <matt.juntu...@hotmail.com> 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 <gillese...@gmail.com>
> > Sent: Saturday, August 17, 2019 6:00 PM
> > To: Commons Developers List <dev@commons.apache.org>
> > Subject: Re: [geometry] GEOMTRY-32 Feedback Requested
> >
> > Hello.
> >
> > Le sam. 17 août 2019 à 04:58, Matt Juntunen
> > <matt.juntu...@hotmail.com> 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 <gillese...@gmail.com>
> > > Sent: Friday, August 16, 2019 11:37 AM
> > > To: Commons Developers List <dev@commons.apache.org>
> > > 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
> > > <matt.juntu...@hotmail.com> 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: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
