search

ianmackenzie / elm-geometry

2D/3D geometry package for Elm
Mozilla Public License 2.0
183 stars 26 forks source link

Potential breaking changes for 2.0 #18

Closed ianmackenzie closed 7 years ago

ianmackenzie commented 7 years ago

This is a meta-issue for potential breaking changes to make for a 2.0 version. Feel free to post suggestions in the comments and I'll edit this issue text.

Module naming

I used prefixed module names like OpenSolid.Point2d to avoid potential naming conflicts with modules from other packages, but based on elm-lang/elm-compiler#1625 it seems that this might be considered "silly module renaming". Should the prefixes be removed? I think they should remain in a few cases like OpenSolid.Geometry.Types and OpenSolid.Geometry.Decode, but otherwise module names could be switched to just plain Point2d, Triangle2d etc.

This would be more consistent with other Elm packages - elm-community/elm-test uses top-level Expect, Fuzz and Test modules, mdgriffith/style-elements uses top-level Style and Element modules, terezka/elm-plot uses a top-level Plot module, etc.

Additionally, if the non-prefixed names don't work out and run into conflicts with other packages, then these conflicts will provide additional data points for elm-lang/elm-compiler#1625.

Renames

I used things like radialDistanceFrom to disambiguate from plain distanceFrom (which takes another point as argument) while keeping the nice 'read as a phrase' quality. However, I've come to think these names are a little too 'cute'/obscure and it would probably be better to use more 'boring' names, even though Point3d.distanceFromAxis axis doesn't read quite as nicely as Point3d.radialDistanceFrom axis.

Removals

Type changes

Signature changes

Opaque types?

Should some or all of the types in this package be switched to opaque with constructor functions?

Pros

Cons

mpizenberg commented 7 years ago

Perhaps move primitive types (points, vectors, directions, axes, planes, frames, bounding boxes) ...

In my use case, I've needed also the polygons so I'd have needed the whole lot anyway. How do you choose/define those primitive types? Would this imply that rendering packages should also be split? I see why you would want that however, especially with all other 2d/3d types in waiting (torus, sphere, cylinder, ...). Maybe an organization by categories of objects? But I've no idea how to categorize them.

Circles and disks would probably both be considered to have 'circumference'. This clears up some ambiguity

Disk and circle as different entities fits well in the library in my opinion. Though I'm not sure it makes sense for a disk to have a circumference. Probably better to have an "inexpensive" conversion to and from a disk to it's corresponding circle.

Not a breaking change, but I've felt the rectangle type as a missing component. Sure polygon can do the trick but it's so much easier to manipulate rectangles than polygons when needing only rectangles. I thought of this now because in case it's added to opensolid, I feel there is also a lack of terminology to distinguish between rectangle the contour (bounding box?) and the surface.

About the types of objects in the library. Have you reconsidered moving to opaque? Similar to what style-elements does with its Internal modules for example. Have you tried this with few examples to see if there is a lack of expressivity, flexibility, performance, ... by doing so? This would also be a breaking change, so better re-investigate this now than later.

ianmackenzie commented 7 years ago

In my use case, I've needed also the polygons so I'd have needed the whole lot anyway. How do you choose/define those primitive types? Would this imply that rendering packages should also be split? I see why you would want that however, especially with all other 2d/3d types in waiting (torus, sphere, cylinder, ...). Maybe an organization by categories of objects? But I've no idea how to categorize them.

Primitives: points, vectors, directions, axes, planes, frames, sketch planes, bounding boxes Shapes: line segments, triangles, circles, arcs, splines, polylines, polygons

This is partially for technical reasons (the primitive types circularly depend on each other and so can't be split into separate packages, while the shape types build on top of the primitives), but I think also makes sense at a higher level - the shapes are things you might actually draw (in SVG or WebGL), while the primitives you typically wouldn't - they're the building blocks that make up everything else.

I expect that a lot of people will be like you and would need both packages, but there might be some people who just want some lightweight point and vector types and don't want to pull in a much larger package. The primary downside is that in the common case instead of

import OpenSolid.Geometry.Types exposing (..)

you'd need something like 

import OpenSolid.Primitive.Types exposing (..)
import OpenSolid.Shape.Types exposing (..)

in all of your source files.

ianmackenzie commented 7 years ago

Not a breaking change, but I've felt the rectangle type as a missing component. Sure polygon can do the trick but it's so much easier to manipulate rectangles than polygons when needing only rectangles. I thought of this now because in case it's added to opensolid, I feel there is also a lack of terminology to distinguish between rectangle the contour (bounding box?) and the surface.

Logged as a separate issue in #19.

ianmackenzie commented 7 years ago

About the types of objects in the library. Have you reconsidered moving to opaque? Similar to what style-elements does with its Internal modules for example. Have you tried this with few examples to see if there is a lack of expressivity, flexibility, performance, ... by doing so? This would also be a breaking change, so better re-investigate this now than later.

I'm still a bit reluctant to do this because I still don't really see they 'why'. I realize that opaque types should be the default choice in most cases, and I love them and use them almost exclusively in some other packages I'm working on (e.g. opensolid/scene, opensolid/step) where they are extremely useful. But I don't want to use them blindly/dogmatically, and especially for low-level stuff like points and triangles, I'm a lot more confident about committing to a specific exposed representation. Plus, writing stuff like

myAxis =
    Axis3d
        { originPoint = Point3d ( 1, 2, 3 )
        , direction = Direction3d.y
        }

is just so convenient; otherwise presumably we'd have something like

myAxis =
    Axis3d.with
        { originPoint = Point3d.withCoordinates ( 1, 2, 3 )
        , direction = Direction3d.y
        }

which is not bad but is not a naming convention I've seen in other libraries. How would you see replacing the exposed constructors?

mpizenberg commented 7 years ago

But I don't want to use them blindly/dogmatically, and especially for low-level stuff like points and triangles, I'm a lot more confident about committing to a specific exposed representation.

Fine, that makes sense. Just one remark about the exposed constructors: as an elm beginner, you tend to get quite early that capitalized words are types and the rest are constants and functions, ... except until you realize not exactly. Type aliases as constructors, or taggers (in unions) are also functions. And that may bring the first confusions for newcomers. An alternative here might be to spotlight the default constructor as being the one with the same name that the type:

import OpenSolid.Geometry.Types exposing (..)
import OpenSolid.Point3d as Point3d exposing (point3d)
import OpenSolid.Axis3d as Axis3d exposing (axis3d)
import OpenSolid.Direction3d as Direction3d exposing (direction3d)

myAxis =
    axis3d
        { originPoint = point3d ( 1, 2, 3 )
        , direction = Direction3d.y
        }

Now, with just a little more experience in elm, I guess the version with the tagger feel better since you know it's a function and you see directly the return type :). Whereas with the function version, there might be a little voice in your head saying, "are you sure this returns what you think it returns" and you might check, just to be sure.

ianmackenzie commented 7 years ago

I certainly want to make this package friendly for newcomers! One potentially interesting compromise could be to make some of the more primitive types opaque, while keeping the higher-level record-based ones exposed; I think for the more primitive types there are more obvious names for constructor functions, and it means that they could in theory be switched to having some different (native?) implementation in the future (although I think I'd prefer to just wait for the Elm compiler to get better at optimizing code rather than write custom JavaScript to work around it). Something like

point =
    Point3d.fromCoordinates ( 1, 2, 3 )

direction =
    Direction3d.fromComponents ( 0, 0, -1 )

lineSegment =
    LineSegment3d.fromEndpoints ( p1, p2 )

triangle =
    Triangle3d.fromVertices ( pt1, pt2, pt3 )

polygon =
    Polygon2d.fromVertices [ p1, p2, p3, p4, p5 ]

boundingBox =
    BoundingBox2d.fromExtrema
        { minX = -1
        , maxX = 3
        , minY = 2
        , maxY = 4
        }

spline =
    QuadraticSpline2d.fromControlPoints ( p1, p2, p3 )

axis =
    Axis3d { originPoint = point, direction = direction }

arc =
    Arc2d
        { centerPoint = Point2d.origin
        , startPoint = Point2d.fromCoordinates ( 2, 3 )
        , sweptAngle = degrees 30
        }

One advantage of constructor functions is that they look nicer in pipelines:

fromVec3 : Vec3 -> Point3d
fromVec3 =
    Math.Vector3.toTuple >> Point3d.fromCoordinates

vs

fromVec3 : Vec3 -> Point3d
fromVec3 =
    Math.Vector3.toTuple >> Point3d

which looks a bit funky.

Of course, having some types be constructed directly and other types be opaque with constructor functions could just make things more confusing...

mpizenberg commented 7 years ago

One advantage of constructor functions is that they look nicer in pipelines

Yes, and this should be the preferred way to write pipeline, whether direct type constructors exist or not. However, I agree with the following:

having some types be constructed directly and other types be opaque with constructor functions could just make things more confusing...

Actually, if you didn't run into needing another internal representation, it might be wiser to just wait if the need arises. You might come to need other potential breaking changes in the way. As Donald said, premature optimization is the root of all evil. And I tend to have compulsive need for premature optimizations too often.

By the way, do you know other people using this library for their projects?

ianmackenzie commented 7 years ago

Yeah, I'm happy to sit on these sorts of potential breaking changes for a while until some sort of consensus or clear answer emerges - no real hurry.

I know of a small number of others using OpenSolid, but not a lot yet...I suspect it might become more popular once I publish opensolid/scene, since I think 'make cool 3D animations' is a much easier sell than 'work with abstract 2D and 3D geometry' (the latter takes a bit more imagination to see how it is actually useful to you).

xarvh commented 7 years ago

My personal takes, as someone who has yet to start using the library:

Module Renaming

Having the Opensolid. prefix helps understanding where stuff comes from. I find it frustraing when a module name does not tell me from what package it comes from, it makes it a lot harder to understand new code.

I wonder about a library that wraps Opensolid (say, a videogame engine) and wants to export a higher level module that's called Point2d, would this be a practical case? I guess it will have to export it as WhateverEngine.Point2d.

What about a CAD that needs to have several object types, augmented with UI state? Again, would the user create a UI.Point2d module?

Regardless, no big deal either way.

Renames

The renames proposed seem like a good idea. axis is not really a good, descriptive name for a variable. A better example would be Point3d.distanceFromAxis lineOfSight which IMHO reads quite well.

Opaque types

In general, having the user instantiate types directly is not a good idea:

If instantiation performance is an issue, provide a direction3dUnsafe constructor. Unsafety should be the exception, not the rule.

Math.Vector3 exposes vec3, so I think it makes sense to use Axis3d.axis3d { originPoint = ..., direction = ...}

Axis3d.with has no practical advantage over Axis3d.axis3d.

I would advise against making only some types opaque, simply because of consistency, it might make things more confusing for newbies. If there is little semantic difference between primitives and shapes, the user should not see the difference.

The problem I see with opaque types is that they limit the efficiency of external libraries. Example: Random.Extra.constant is implemented by calling Random.map, which must generate a value, and then throwing it out. This can be worked out with a proper API (IIRC, there were plans to have constant inside Random).

ianmackenzie commented 7 years ago

Great comments @xarvh! Keep them coming =)

I wonder about a library that wraps OpenSolid (say, a videogame engine) and wants to export a higher level module that's called Point2d, would this be a practical case? I guess it will have to export it as WhateverEngine.Point2d.

Yes, that would probably make the most sense. If the module was just in an app, though (not published as a package), then you could call your local high-level module Point2d and do import OpenSolid.Point2d as OSPoint2d or something whenever you need to access the OpenSolid type directly (which might only be within the higher-level local module).

If instantiation performance is an issue, provide a direction3dUnsafe constructor.

I think I like the idea of renaming Direction#d.withComponents to Direction#d.unsafe, and then also Frame#d.with -> Frame#d.unsafe and SketchPlane3d.with -> SketchPlane3d.unsafe (since it's possible to construct invalid frames/sketch planes by passing non-orthogonal directions). You could also make an argument that BoundingBox#d.with should be called unsafe since you can pass (e.g.) { minX = 3, maxX = 2, ... } but that's a little harder to mess up. And Circle#d.with can just call abs on the radius value that gets passed in...

Math.Vector3 exposes vec3, so I think it makes sense to use Axis3d.axis3d { originPoint = ..., direction = ...}

I do like the consistency of having all values and functions prefixed with the module (an axis3d function seems designed to be used un-prefixed), and I think it scales better to multiple constructors. For example, switching between Point2d.withCoordinates and Point2d.midpoint seems pretty natural while switching from point2d to Point2d.midpoint seems a bit weird. Similar for Circle2d.with/ Circle2d.throughPoints, etc.

The problem I see with opaque types is that they limit the efficiency of external libraries.

I don't think this should be an issue as long as there are some zero-overhead 'directly construct' functions like Point3d.withCoordinates that basically just call the underlying constructor (and therefore seem likely to get inlined by the JavaScript VM or perhaps even the Elm compiler).

ianmackenzie commented 7 years ago

I think 2.0 is almost ready to be released - I've placed my draft release notes and a copy of the docs.json for 2.0 here. If anyone has time to take a look and see if things generally make sense (or if you have feedback on any of the changes), that would be great!

xarvh commented 7 years ago

Fantastic job @ianmackenzie !

I went through the README.md, I think 2.0 is a great improvement.

For the sake of nitpicking, I have a few comments.

The withCoordinates constructors seem to be unnecessarily verbose. On the other hand, the from constructors seem a bit obscure. Also, the use of with and from seems to be a bit inconsistent (why Point2d.withPolarCoordinates rather than Point2d.fromPolarCoordinates?)

If we follow linear-algebra/core convention, the obvious constructor has the name of the type (eg, point3d) and all other constructors are in the form from${Type} where Type describes the constructor argument. This is, for example, what Vec3.vec3 does.

Another straightforward convention would be a conversion: instead of naming a function ${contructedType}From${Argument} a common, and easy to think about naming pattern is ${argument}To${contructedType}.

Of course, which one you use depends on where you want the focus to be.

Regardless of the above, the API seems good!

ianmackenzie commented 7 years ago

Hmm @xarvh you have a point - there's a bit of inconsistency in that 'from' sometimes means "convert from" (as in SketchPlane3d.fromPlane), while in other cases it means "from one point to another" (Vector2d.from, Direction3d.from, LineSegment2d.from etc.). I do like the conciseness of the latter, and they work well with partial application, e.g.

points |> List.map (Vector2d.from Point2d.origin)

to get a list of radial vectors from the origin point to a bunch of other points, but I guess it does really come into conflict with the general Elm pattern of from meaning a conversion of some sort.

As for 'with', that was largely a result of just trying to find something that read well, for example

Axis2d.with
    { originPoint = Point2d.withCoordinates ( 2, 3 )
    , direction = Direction2d.withPolarAngle (degrees 30)
    }

can be read pretty literally as "an Axis2d with the origin point being a Point2d with coordinates ( 2, 3 ) and the direction being a Direction2d with a polar angle of 30 degrees". Something like

axis2d
    { originPoint = point2d ( 2, 3 )
    , direction = Direction2d.fromAngle (degrees 30)
    }

is more concise but I don't think reads quite as well (and is a bit less consistent, where one constructor is prefixed but the other two aren't). Perhaps a compromise could be to just use from for everything except the bare with functions:

Axis2d.with
    { originPoint = Point2d.fromCoordinates ( 2, 3 )
    , direction = Direction2d.fromAngle (degrees 30)
    }

This would keep what I think is the nice property of always using the module as a prefix, and be pretty explicit, but cut down on the unconventional use of 'with'. Not sure what to do with the bare from functions, though...Vector2d.from and Direction2d.from could switch back to their original forms Point2d.vectorFrom and Point2d.directionFrom, but Point2d.lineSegmentFrom would be pushing it.

On a separate note, I have no particular desire to follow any given pattern just because linear-algebra uses it - I mean, if there are some good ideas there, great, but I personally think there's a lot of room for improvement in coming up with a more elegant and clear API (there's a reason this package has its own point, vector and direction types instead of just using Vec2 and Vec3!). And I'm not sure I see the vec3 pattern as something that a lot of other Elm packages use - I'm much more convinced by "use all functions prefixed by the module name" as a general pattern to follow, which to me implies finding some sort of Point3d.functionName that works well instead of just using point3d. What did you mean by 'core convention'? I guess there's Regex.regex which would make sense to use unqualified, but then there's Color.rgb instead of Color.color...and while linear-algebra is on the native whitelist, I don't see it as being a true 'core' library.

mpizenberg commented 7 years ago

From what I'm reading, it seems great. But take my impression lightly since I've not used a lot of this package functionalities, mostly simplest stuff. By the way, it may be old but did you drop the idea of splitting the package in "core" and "advanced" features? I just thought of this while looking at the doc preview. You were right on the fact that it feels a bit overwhelming, even if I was not really in favor of splitting.

xarvh commented 7 years ago

Again, bear in mind that I'm nitpicking and I think the API looks perfectly OK.

When it means "from one point to another" TBH I find it a bit confusing, because it leaves the "to another" part completely out. Since, as an outsider, I would expect a "from" function to take a single argument, seeing it in a partial application makes it especially confusing.

mpizenberg commented 7 years ago

Regarding the from issue:

ianmackenzie commented 7 years ago

OK how about this: change most withX to fromX...

Current Proposed
Point3d.withCoordinates Point3d.fromCoordinates
Point2d.withPolarCoordinates Point2d.fromPolarCoordinates
Vector3d.withComponents Vector3d.fromComponents
LineSegment2d.withEndpoints LineSegment2d.fromEndpoints
QuadraticSpline3d.withControlPoints QuadraticSpline3d.fromControlPoints
Polyline2d.withVertices Polyline2d.fromVertices
etc. etc.

...simplify the Direction2d angle-related function names a bit (there's an argument to be made for toAngle instead of just angle, but I think angle is more consistent with stuff like Point2d.coordinates and Direction3d.azimuth/Direction3d.elevation)...

Current Proposed
Direction2d.withPolarAngle Direction2d.fromAngle
Direction2d.polarAngle Direction2d.angle

...change Vector#d.withLength to Vector#d.with, since just renaming it to Vector#d.fromLength would be super weird...

Current Proposed
Vector2d.withLength 3 Direction2d.x Vector2d.with { length = 3, direction = Direction2d.x }

...and keep all the existing plain with and from functions the same. I think this should make things much more conventional/less weird in general (especially the switch from 'with' to 'from' in most cases).

@xarvh, I agree with your points about the bare from functions, but having looked through my own code I really like how they read and how succinct/useful they are, so I think I'm willing to spend a bit of strangeness budget on them. (I also don't really think of stuff like Vector2d.from p1 p2 as a conversion, so I don't think it should necessarily follow the from${Type} pattern that's used elsewhere.)

Thanks all for the comments - this is really useful feedback!

ianmackenzie commented 7 years ago

@xarvh:

I pointed out the linear algebra convention more as a "FYI" than a suggestion. If you have deliberately decided to break with it, I'm 100% behind it, especially since I do prefer to keep the module name as prefix.

Glad we're in agreement - hope I didn't come off as snippy! (The general comment about following linear-algebra conventions has just now come up enough times from enough different people that I wanted to make my own perspective clear.)

ianmackenzie commented 7 years ago

@mpizenberg:

By the way, it may be old but did you drop the idea of splitting the package in "core" and "advanced" features? I just thought of this while looking at the doc preview. You were right on the fact that it feels a bit overwhelming, even if I was not really in favor of splitting.

It's still something I'm considering (placeholder issue here), although I'm currently tempted to publish 2.0 and maybe split the package into two for a 3.0. Fortunately, the new module layout in 2.0 should make that even easier for end users since code won't change at all, just elm-package.json - the code will still just be

import OpenSolid.Point2d as Point2d exposing (Point2d)
import OpenSolid.Triangle2d as Triangle2d exposing (Triangle2d)

even if in the future those modules actually come from two different packages like opensolid/primitives and opensolid/geometry.

xarvh commented 7 years ago

LGTM =)

ianmackenzie commented 7 years ago

2.0 is now released! Release notes are here. Closing this issue, I'll start a new one for 3.0 =)