Reformating discussion to adapt to 2D-like sketching

[jorgepiloto]

🐞 Problem
Due to short timelines, we ended up developing quite fast till we finally came with the minimum required logic.

As of now, the ansys.geometry.core.sketch module assumes users to be working in the 3D global frame. As we already now, this complicates things, as we need to perform various checks to guarantee that the points of the sketch belong to its fundamental plane.

Although the reformat may seem easy, there are various things behind the scenes that need to be refactored first to successfully get a robust and solid implementation of the sketch side.

💡 Naming conventions

Right now, our ansys.geometry.core.math objects are not consistent in naming. We have Point, Vector and Matrix33. I would like to suggest the following naming conventions:

• Point, Point2D and Point3D

• Vector, Vector2D and Vector3D

• Matrix, Matrix2D and Matrix3D

  The following functions would be implemented, returning Matrix2D or Matrix3D:

  • get_rotation_matrix2D(angle) and rotation_matrix3D(angle, axis)
  • get_translation_matrix2D(Vector2D) and translation_matrix3D(Vector3D)
  • get_scaling_matrix2D(fx, fy) and get_scaling_matrix3D(fx, fy, fz)

These follow the naming of libraries like Eigen, C++. Notice that I suggested to bring back the 2D and 3D classes. This is has several advantages:

• It explicitly states the dimension of the object, 2D or 3D
• It allows to better isolate the checks for each one of the objects
• It would allow to use transformation matrices in 2D or 3D perspective

Previous naming conventions would allow us to support a C#-like (strongly typed) approach:

Pythonic support

circle = sketch.draw_circle(
    center = (0, 0) * u.mm, radius = 0.5 * u.mm
)

Strongly typed support (C#-like)

circle = sketch.draw_circle(
    center=Point2D(xy=(0, 0), units=u.mm), 
    radius=Distance(magnitude=1, units=u.mm)
)

Notice that right now, the following needs to be done:

circle = sketch.draw_circle(
    center=Point((0, 0), u.mm),  # How many coords does this `Point` class accept? N-coords?
    radius=Distance(1, u.mm)
)

💡 Remove setters

The reason behind this suggestion is to force users to pass through constructors and transformations to modify the objects they create.

💡 Proper overriding of matrix multiplication methods

There are two big dunder methods that apply when using the * and @ operators:

• The __mul__ allows to override the * method
• The __matmul is for matrix multiplication and allows to override the @ operator

We need to ensure that both methods are overriden and return our custom objects. Otherwise, we sometimes get np.ndarrays instead of our own objects.

💡 Using transformations in sketch objects

All shapes inheriting from BaseShape should implement the rotate, scale and translate methods. These would call the routines get_translation_matrix2D, get_rotation_matrix2D and get_translation_matrix2D behind the scenes to perform the requested transformation.

On the other hand, the Sketch class could implement the rotate, scale and translate methods too but from a 3D perspective. Here is where the get_*_matrix3D logic would be used.

[RobPasMue]

That is not totally right @jorgepiloto. We have a Matrix class in the same way that we have a Vector and a Point class. We decided to avoid specific XD notation to avoid duplication (precisely Point2D and Point3D).

Matrix33 and Matrix44 exist as simple extensions from the Matrix class.

Point and Vector classes are inteligent enough to determine whether (provided the inputs) you are working on 2D or 3D space. If you only give two coordinates it is clear that you are working on 2D space. The same happens for 3D.

These classes also have embedded support for knowing its dimensions. You can run point.is_2d or point.is_3d for this sole purpose. The same support is available in the Vector class.

We moved from the specific XD notation to avoid Code duplication. And honestly, I think it is easier for end users to just have a simple class (either Point or Vector) that you can build on the fly with either 2 or 3 dimensions (we are only supporting upto 3D - i.e. space). It doesn't make sense to deal with more dimensions inside a CAD library.

Plus, in terms of usability, it is again easier to just call rotate(...) and led the code deal with the implementation based on the embedded support to multiple dimensions within the same class.

[RobPasMue]

Answering your question above.

circle = sketch.draw_circle(
    center=Point((0, 0), u.mm),  # How many coords does this `Point` class accept? N-coords?
    radius=Distance(1, u.mm)
)

Point will be 2D for its entire lifespan. Since the numpy.ndarray is created with 2 values (and ndarrays do NOT morph). Thus, it will always be a 2D Point...

I don't understand the issue...

[jorgepiloto]

There are different advantages on having explicit classes for Object2D and Object3D:

• You have specific classes for modeling desired dimension objects, which makes the code more explicit
• You now where the dimension checks are performed: in the constructors of those classes
• Beartype can better filter the type of object, as there is a distinction between 2D and 3D
• The distinction between 2D and 3D applies in popular libraries and softwares, including Ansys ones

[RobPasMue]

Regarding the proper override of the dunder methods in the Matrix class, I fully agree. We must try to always deal with our own object. Not with simple ndarrays.

[RobPasMue]

I agree that the Matrix class is not aligned to Point and Vector, but we could align it as well if you were interested.

[RobPasMue]

FYI, if I remember properly, the Eigen library is using something similar to dynamic aliases for noting VectorXD. It is the same class behind the scenes - with dynamic dimensions. The only difference is that it accepts instantiating it as Vector3D

[chadqueen]

circle = sketch.draw_circle(
    center=Point2D(xy=(0, 0), units=u.mm), 
    radius=Distance(magnitude=1, units=u.mm)
)

Point2D creates more usable signatures for using the Sketch class IMO.

In the current signature, it is ambiguous whether a 3rd dimension is expected or not. A 2D point is a different coordinate system from the 3D point in this context even though they are the same method parameter. It would be much cleaner to restrict to 2D via a type since the sketch is a planar perspective.

[chadqueen]

I also agree with consistency in Ansys projects. Every project I have seen so far has 2D and 3D specific types for Point/Vector/Matrix, including the underlying service we are wrapping. Users could adopt our tool faster/better if the naming conventions are inline with SpaceClaim.

[chadqueen]

Other projects within Ansys may drop the specificity, but I am just unaware of them. My perspective here mainly comes from what SpaceClaim has established as naming conventions since that is the domain and userbase we are targeting.

[RobPasMue]

Correct me if I'm wrong but SpaceClaim names Point to everything.... They are not differentiating between 2D and 3D at naming...

[RobPasMue]

Whelp. If I'm the only against it, then go ahead 😄 ...

Honestly I think we are going to fall back into the same issue we had before (which was having numerous duplicated concepts and classes, repeated implementations, functionalities and so on).

If you align everything to ObjectND again remember adapting ALL of the classes: Point, Vector, UnitVector, QuantityVector...

The constructor for Point and Vector, as it is right now, works perfectly fine for both concepts. If a user provided 2 values it will behave as 2D. If the user provided 3 values it behaves as 3D. And It will always stay like that since (again) once you instantiate a numpy.ndarray it stays the same in terms of dimensions.

I can understand your reasons for 'clarifying' the interface, but honestly... I think it's the same thing and we will be duplicating everything. So I don't really see the benefit here.

BTW, SpaceClaim uses the sole concept of "Point" for example. They don't make any distinctions at "naming" AFAIK.... It's just "Point"

[RobPasMue]

Pinging @akaszynski for insights as well

[RobPasMue]

Reformatting has been performed. Pure 2D sketching is now implemented. Thank you all for your collaboration. I am now locking this discussion.