gumyr / build123d

A python CAD programming library
Apache License 2.0
395 stars 72 forks source link

Clarify transformations in API #173

Closed snoyer closed 1 year ago

snoyer commented 1 year ago

The API is currently a bit confusing (confused?) regarding transformations. For example let's look at Shape:

class Shape:
    def locate(self, loc: Location) -> Shape:
        """Apply a location in absolute sense to self"""
    def located(self, loc: Location) -> Shape:
        """Apply a location in absolute sense to a copy of self"""

    def move(self, loc: Location) -> Shape:
        """Apply a location in relative sense (i.e. update current location) to self"""
    def moved(self, loc: Location) -> Shape:
        """Apply a location in relative sense (i.e. update current location) to a copy of self"""

First, "locate" sound like it would go look for something, I think "place", "place_at", "move_to", or "relocate" would be more intuitive (but maybe that's just the flavor of English I'm used to).

Second, and more importantly, using the same type for relative and absolute positioning is sub-optimal, having different types would allow the IDE to actively provide hints and help minimize possible confusion/mistakes.

Shape could look something like:

class Shape:
    def place_at(self, location: Location) -> Shape:
        """Update self's location"""
    def placed_at(self, location: Location) -> Shape:
        """Copy self to new a location"""

    def transform(self, transform: Transform) -> Shape:
        """Apply a transform to self's location"""
    def transformed(self, transform: Transform) -> Shape:
        """Create a copy of self at a transformed location"""

with users being able to create transformation from locations, and locations being transformable:

class Location:
    def to(self, other: Location) -> Transform:
        """Compute the Transform that, when applied, goes form self to other"""
    def from_(self, other: Location) -> Transform:
        """Compute the Transform that, when applied, goes form other to self"""

    def transform(self, transform: Transform) -> Location:
        """Apply a transform to self"""
    def transformed(self, transform: Transform) -> Location:
        """Create a transformed copy of self"""

and transform would basically be Matrix but prettier:

class Transform:
    def transform(self, transform: Transform) -> Transform:
        """Apply a transform to self"""
    def transformed(self, transform: Transform) -> Transform:
        """Create a transformed copy of self"""

    def invert(self) -> Transform:
        """Invert self"""
    def inverse(self) -> Transform:
        """Return the inverse transform of self"""

    @classmethod
    def Translation(cls, delta: VectorLike) -> Transform:
        """Create a new translation Transform"""

    # more factory methods for rotations and whatnot

Most of the geometry classes (Vector, Axis, Plane, ...) would benefit for having the same transform and transformed method uniformly based on Transform across the board.

Operator overloading should be looked into once a solid based has been established:

The main gotcha I can foresee right now is that while it would be nice to have generic affine transforms (see Matrix) in the geometry API they would be more powerful than "relative locations" in that they can represent more than just translation and rotation, which may not be what you want at CAD level. However if the user creates them from the methods in the Location class (or other high level helpers) then they would be constrained to the appropriate subset; and we can always add checks to the transform/transformed methods to validate the input on a object by object basis.

Thoughts, comments, criticism, and opinions welcome :)

bernhard-42 commented 1 year ago

Second ...

Transformation is mathematically a much broader concept than moving and locating (https://en.wikipedia.org/wiki/Transformation_(function)): Locating and moving doesn't change the topology (e.g. a flat square is still a flat square after the operation, just somewhere else). On the other side, the operation scale for example, is also a transformation, however it changes the underlying topology.

For tessellation and STEP exporting assemblies, location changes allow to work with references, while object transformations require to tessellate or export the object. So, for me it does make sense to distinguish:

Location changes

Generic object transformations:

Operator overloading should be looked into once a solid based has been established: shape @ loc for absolute placement

transform3 = transform1 transform2 plane transform ?

=> this is what alg123d does: obj @ (plane * location), i.e. position absolute on a plane and then move relative to this plane, see https://github.com/bernhard-42/Alg123d#placement-on-planes-and-at-locations)

Exception: now_loc - old_loc: What does (new_pos, new_quaternion) - (old_pos, old_quaternion) mean?

... same type for relative and absolute positioning ...

I had less a problem with the same type, but with Location combing position and rotation (quaternion). While it is mathematically correct, for me it seems to be easier to distinguish Pos(x, y, z) and Rot(x_angle, y_angle, z_angle) that inherit from Location but restrict it. You can still do Pos(x, y, z) * Rot(x_angle, y_angle, z_angle) or vice versa which makes the order of the operations clear. And, I think for Pos and Rot a meaningful interpretation of - could be found.

In alg123d I avoid the terminology issues by using operators. The general idea being to allow the underlying direct api to have as many concepts as it wants (e.g. move vs. translate), however make the API on top using a meaningful selection of these concepts.

These are my 2 cents on the subject.

gumyr commented 1 year ago

@snoyer as @bernhard-42 describes changing the location of an object is very different from transformations. There has been a long discussions on changing the move/moved, locate/located (and less so translate and rotate) methods here are some of the reasons for why they are the way they are:

I'm likely to change the implementation of translate and rotate to update the position and orientation sub-properties of Shapes. This can also be done with my_shape.position = Vector(1,1,1) or my_shape.position += Vector(1,1,1). I'm hesitating a little as there might be a valid reason to want to actually change the position of the vertices but I don't know of one.

snoyer commented 1 year ago

@bernhard-42, @gumyr, thank you both for the input.

These terms come directly from OpenCascade).

Maybe I'm looking at it from the wrong level, if the direct API is supposed to closely follow the OpenCascade one then fair enough. I was under the impression that it was aiming to sit at a significantly higher level.

However I still think that the typing and naming of higher level interfaces should be thought out in a way that makes sense without having to be justified/explained by implementation reasons. I don't know how much it actually applies here but here's my rant perspective anyway:

Ignoring OpenCascade history/tradition/bagage, the name "Location" implies a position (and orientation in this case) in space so, while it is fine and logical to move an object to a location, moving an object by a location is just not a thing. Of course I understand that the underlying data (position vector, and euler angles) can be used in both case. But again, from an API design point of view it is not optimal.

Having a Transformation Movement class that represents "going from one location to another" (whatever that is under the hood, maybe it's the same thing) would make the absolute/relative distinction explicit:

I think transformations (as in affine transforms) could still fit in the picture (by, internally, separately scaling/shearing the geometries, and translating/rotating the occt locations) but that's probably a separate issue that I was mistaken for lumping in.

gumyr commented 1 year ago

Similar (very long) discussions have be had over the use of Vector to represent a point in space. The added value to users just isn't there at least at this point. There are far larger problems to solve.

With respect to existing methods to change the position of a Shape:

However, neither algebra nor builder api users directly use these methods so there seems to be little value in changing them especially since direct api users are likely messing with OpenCascade where all these methods exist.

snoyer commented 1 year ago

Fair enough, closing then