feat: geometry objects with new structure #373
Conversation
|
Thing that came to my mind first: splitting classes to different files will make it behave not like C# (for dev experience), because in Python we cannot assign namespaces. So every import will look slightly redundant: from...object.geometry.point import Point, while in C# it can be just used as Speckle.Geomerty.Point Not necessarily an issue, but an observation |
|
|
||
| raise IndexError(f"Face index {face_index} out of range") | ||
|
|
||
| def transform_to(self, transform) -> Tuple[bool, "Mesh"]: |
There was a problem hiding this comment.
type hints would be helpful (here and in the other methods and classes, including interfaces)
|
Re-exporting a module could be an option here. |
| return total_length | ||
|
|
||
| @property | ||
| def _domain(self) -> Interval: |
There was a problem hiding this comment.
why was domain made an internal property? both here and in ICurve interface
| domain: Interval = field(default_factory=Interval.unit_interval) | ||
|
|
||
| @property | ||
| def length(self) -> float: |
| transform this point using the given transform | ||
| """ | ||
| m = transform.matrix | ||
| tx = self.x * m[0] + self.y * m[1] + self.z * m[2] + m[3] |
There was a problem hiding this comment.
should we convert matrix to the objects' units first? Same for other geometry classes
There was a problem hiding this comment.
There is a known bug in the c# implementation that we do this incorrectly.
There was a problem hiding this comment.
Are we using an array of 16 values instead of matrix in c# for transform now? @JR-Morgan
| return cls(x=coords[0], y=coords[1], z=coords[2], units=units) | ||
|
|
||
| @classmethod | ||
| def from_coords(cls, x: float, y: float, z: float, units: str | Units) -> "Point": |
There was a problem hiding this comment.
in it's current state, "from_coords" acts exactly like the default constructor (because x, y, z, units are already enforced). Should we still keep this method?
|
|
||
| @property | ||
| def radius(self) -> float: | ||
| return self.startPoint.distance_to(self.plane.origin) |
There was a problem hiding this comment.
unlikely event, but if the StartPoint and Plane have different units (nothing prevents the user to construct it), we might need to convert the distance to the Arc's units
Same with "measure" property
There was a problem hiding this comment.
I think we will need to assume that an object never has mixed units. There's too much opportunity for undefined behaviour, and performance overheads if we start mixing units.
This fact sucks for DX, but I would pitch us moving away from using a our Point class, and using a light weight System.DoubleNumerics. Vector3 struct (dunno what a Python equivalent would be)
There was a problem hiding this comment.
maybe the less hardcore way would be to validate the units in the constructor, and in case of inconsistency (or null) throw an error?
There was a problem hiding this comment.
We'd not prefer to have mix units but there will be no errors anyway.
plane = Plane(
origin=Point(x=0, y=0, z=0, units="mm"),
normal=Vector(x=0, y=0, z=1, units="mm"),
xdir=Vector(x=1, y=0, z=0, units="mm"),
ydir=Vector(x=0, y=1, z=0, units="mm"),
units="mm"
)
arc = Arc(
plane=plane,
startPoint=Point(x=1, y=0, z=0, units="m"), # Different units
midPoint=Point(x=0.7071, y=0.7071, z=0, units="m"),
endPoint=Point(x=0, y=1, z=0, units="m"),
units="m"
)
# This will work - distance_to() handles the unit conversion
radius = arc.radius # Will be in meters since startPoint is in metersThe conversion logic will effect the performance though.
| transformed_vertices = [] | ||
|
|
||
| for i in range(0, len(self.vertices), 3): | ||
| point = Point( |
There was a problem hiding this comment.
why not use .vertices_count and .get_point here?
| ]) | ||
|
|
||
| current_area = self._area | ||
| current_volume = self._volume |
There was a problem hiding this comment.
these 2 will need to be recalculated. Let's add a @Property "area" and "volume", even if just placeholders for now and use it here.
| convert the mesh vertices to different units | ||
| """ | ||
| if isinstance(to_units, Units): | ||
| to_units = to_units.value |
There was a problem hiding this comment.
on the contrary, we can check if it's string instead (we will convert to Unit anyway in the next line)
| colors: List[int] = field(default_factory=list) | ||
| textureCoordinates: List[float] = field(default_factory=list) | ||
| _area: float = field(default=0.0, init=False, repr=False) | ||
| _volume: float = field(default=0.0, init=False, repr=False) |
There was a problem hiding this comment.
maybe area and volume should only be calculatable properties instead
| self._area *= scale_factor * scale_factor | ||
| self._volume *= scale_factor * scale_factor * scale_factor | ||
|
|
||
| def is_closed(self) -> bool: |
This PR splits the geometry objects to different files under geometry/ folder.
Addition to that: