Hi Gilles,
> package "io" does not belong in "core".
Ah, yes. I'd like to keep the core and euclidean IO modules separate since I'm
picturing a spherical IO module later on down the road. In that case, perhaps
we could just put the "io" portion of the module/package name before everything
else? That would give us
commons-geometry-io-core - contains package o.a.c.geometry.io.core
commons-geometry-io-euclidean - contains package o.a.c.geometry.io.euclidean
> Then, we had also discussed that it would have been more robust to use a
> parser generator...
Yes. I looked into that and ended up deciding that a full parser-generator
would be overkill for what I wanted to do and possibly more work to
maintain/customize. I've placed the current low-level parsing code (consisting
of the CharReadBuffer and SimpleTextParser classes) in the "internal" package
o.a.c.geometry.core.io.internal. I don't intend for these classes to be part of
the public API, which would allow us to switch to a parser-generator later if
needed.
> I haven't read all the code (sorry!)
No worries. There is a lot of it :-)
> I'm not convinced by the usage of file "extensions" to determine how the
> contents should be parsed. At first sight, an "enum"-based approach[3] would
> be more flexible and clearer from a code design POV.
In contrast to the rest of the library, I'd like to make these IO modules as
extensible as possible, the reason being that they form the main connection
between the library and external systems. If we use an enum to indicate the
format, we restrict usage of the API to only those formats. A string name, on
the other hand, allows users to define their own formats and use them with the
API. For example, we will soon be creating a custom geometry format for an
application at my day job and I would like to be able to use these IO modules
to read and write that format (using custom BoundaryReadHandler3D and
BoundaryWriteHandler3D implementations).
Thoughts?
Regards,
Matt J
________________________________
From: Gilles Sadowski <gillese...@gmail.com>
Sent: Wednesday, January 20, 2021 9:17 AM
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: [geometry] IO Modules
Hi Matt.
Le mer. 20 janv. 2021 à 05:03, Matt Juntunen
<matt.juntu...@hotmail.com> a écrit :
>
> Hello,
>
> I've created GEOMETRY-115 and an associated PR [1] containing new modules for
> IO functionality. The new modules are
>
> * commons-geometry-core-io - Common space-independent interfaces and
> classes
> * commons-geometry-euclidean-io - Euclidean IO classes; currently
> contains support for the 3D formats TXT, CSV, and OBJ
>
> The API is based on a core BoundaryIOManager class that delegates to
> BoundaryReadHandler and BoundaryWriteHandler implementations based on the
> requested data format. For Euclidean 3D space, a convenience IO3D class is
> provided with static methods that delegate to a default manager instance. In
> addition to reading and writing the core geometric types for the library
> (ConvexPolygon3D, Triangle3D), the Euclidean module also supports reading and
> writing a FacetDefinition interface, which exposes simple, unvalidated
> geometric data. This is intended for accessing raw (possibly invalid)
> geometric data from files and writing data contained in external data
> structures (for example, a custom facet class). The example below is from the
> IO3D class documentation and demonstrates a read, transform, write operation
> using streams.
>
> final Path origFile = tempDir.resolve("orig.obj");
> final Path scaledFile = tempDir.resolve("scaled.csv");
>
> final DoublePrecisionContext precision = new
> EpsilonDoublePrecisionContext(1e-10);
> final BoundarySource3D src = Parallelepiped.unitCube(precision);
>
> IO3D.write(src, origFile);
>
> final AffineTransformMatrix3D transform =
> AffineTransformMatrix3D.createScale(2);
>
> try (Stream<Triangle3D> stream = IO3D.triangles(origFile, precision))
> {
> IO3D.write(stream.map(t -> t.transform(transform)), scaledFile);
> }
>
> Feedback is welcome.
The high-level API, illustrated in the example above, looks quite fine.
But we should enforce "separation of concerns"; in this particular case:
package "io" does not belong in "core".
The Java package
org.apache.commons.geometry.core
is defined the within the (maven) module
commons-geometry-core
The new Java package
org.apache.commons.geometry.core.io
is defined the within the (maven) module
commons-geometry-core-io
The (maven) module name is also often used to define the
Java 9+ module name for JPMS.[1]
IIUC, the package
org.apache.commons.geometry.core
and its "sub-package"
org.apache.commons.geometry.core.io
cannot belong to different (JPMS) modules, because (IIUC) a package
hierarchy cannot be shared across modules (since one of the purposes
is to organize "visibility").
Components should be JPMS-compatible.[2]
Fixing that would just require to
1. remove the two modules introduced in PR #130
commons-geometry-core-io
commons-geometry-euclidean-io
2. create one (maven) module
commons-geometry-io
owning the Java packages
org.apache.commons.geometry.io
org.apache.commons.geometry.io.core
org.apache.commons.geometry.io.euclidean
[If you think that it's really worth having separate modules for loading
"euclidean" data vs other space-types (not yet implemented IIUC),
point (2) would be adapted accordingly.]
This would fix both the JPMS issue, and the design issue.
Then, we had also discussed that it would have been more robust to
use a parser generator...
In either case, I'd suggest that we put the actual I/O classes in an
"internal" package, and only advertize the above high-level API (i.e. we
should not be expected to maintain compatibility of the parser code).
I haven't read all the code (sorry!), but in class "IO3D", I'm not convinced
by the usage of file "extensions" to determine how the contents should be
parsed. At first sight, an "enum"-based approach[3] would be more flexible
and clearer from a code design POV.
Regards,
Gilles
[1] https://www.oracle.com/corporate/features/understanding-java-9-modules.html
[2] See
https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=tree;f=commons-rng-examples/examples-jpms;hb=HEAD
[3] See
https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=blob;f=commons-rng-simple/src/main/java/org/apache/commons/rng/simple/RandomSource.java;h=65ca02fdc285fbb7ea8305008dbce21f571191d7;hb=HEAD
>
> Regards,
> Matt J
>
> [1] https://github.com/apache/commons-geometry/pull/130
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
