item as kwarg #1353

Licini · 2024-05-13T15:45:38Z

In SceneObject init, changing the item to be a kwarg. This will be very helpful in handling complicated multi-inheritance situations, in places like Viewer, where this is becoming very tricky.

src/compas/scene/sceneobject.py

Licini · 2024-05-22T17:27:00Z

@tomvanmele @gonzalocasas @chenkasirer Hey guys, I expanded this PR a bit to make a rather big update to clean up how data items are assigned to SceneObjects.

Previously, in SceneObject class __init__, we accept the geometry/datastructrue item as the first positional argument, in form of:

class SceneObject:
    def __init__(self, item, **kwargs):
        self.item=item
        ...

Then its subclasses like MeshObject we add on top:

class MeshObject(SceneObject):
    def __init__(self, mesh, **kwargs):
        super().init(item=mesh, **kwargs)
        self.mesh=mesh
        ...

Meanwhile we also created general sceneobject class for different context like RhinoSceneObject and BlenderSceneObject where their __init__ does not take positional arguments, only keyword arguments:

class RhinoSceneObject(SceneObject):
    def __init__(self, **kwargs):
        super().init(**kwargs)
        ...

At last we need to make specific sceneobject for context such as MeshObject for Rhino, which looks like:

class MeshObject(RhinoSceneObject, BaseMeshObject):
    def __init__(self, mesh, **kwargs):
        super().init(mesh=mesh, **kwargs)
        ...

Especially for the last one which involves multi-inheritance, it becomes very difficult to know how to pass in data item to the super init function, whether as positional or kwargs. And it is very often we encounter error of conflicted args or kwargs while extending further these classes.

So the idea here is to remove all this complexity. We do not use positional arguments at all. And we only catch item as kwarg ONCE at the root SceneObject class. And if we need alias such as MeshObject.mesh, we make it a property that fetches self.item, which is the "single source of truth".

I didn't do it for all, just few of them as a demonstration how it can look like. Please let me know if you guys think this is the good direction, then I will apply the same to all rest of SceneObject classes.

Licini · 2024-05-22T17:40:26Z

src/compas/scene/geometryobject.py

+    def __init__(self, pointcolor=None, linecolor=None, surfacecolor=None, pointsize=1.0, linewidth=1.0, show_points=False, show_lines=True, show_surfaces=True, **kwargs):
+        super(GeometryObject, self).__init__(**kwargs)
+        self.pointcolor = pointcolor or self.color
+        self.linecolor = linecolor or self.color
+        self.surfacecolor = surfacecolor or self.color
+        self.pointsize = pointsize
+        self.linewidth = linewidth
+        self.show_points = show_points
+        self.show_lines = show_lines
+        self.show_surfaces = show_surfaces


So after adjustment, any __init__ of subclasses should only care about additional parameters that its parent classes do not have, the data item will be passed along automatically within **kwargs and eventualy received by the root __init__ of SceneObject

Licini · 2024-05-22T17:41:52Z

src/compas/scene/geometryobject.py

+    @property
+    def geometry(self):
+        return self.item


If we need alias we create a property that points to self.item instead of creating another attribute.

Licini · 2024-05-22T17:43:49Z

src/compas/scene/graphobject.py

-    @graph.setter
-    def graph(self, graph):


These setters I removed for now, because item in SceneObject is currently read-only

Licini · 2024-05-22T17:45:24Z

src/compas_rhino/scene/brepobject.py

-    """A scene object for drawing a RhinoBrep.
-
-    Parameters
-    ----------
-    brep : :class:`compas_rhino.geometry.RhinoBrep`
-        The Brep to draw.
-
-    """
-
-    def __init__(self, brep, **kwargs):
-        super(RhinoBrepObject, self).__init__(geometry=brep, **kwargs)
+    """A scene object for drawing a RhinoBrep."""


In many cases the __init__ function of subclass can be entirely omitted, because the don't do anything else than passing in the positional argument using a different name.

src/compas/scene/graphobject.py

src/compas/scene/meshobject.py

src/compas/scene/volmeshobject.py

Licini · 2024-06-10T16:39:05Z

src/compas_blender/scene/meshobject.py

@@ -120,7 +140,7 @@ def draw_vertices(self) -> List[bpy.types.Object]:
        for vertex in vertices:
            name = f"{self.mesh.name}.vertex.{vertex}"
            color = self.vertexcolor[vertex]
-            point = self.vertex_xyz[vertex]
+            point = self.mesh.vertex_coordinates(vertex)


I did this here because the update_object will apply transformation afterwards. If using vertex_xyz, the transformation will be applied twice, similar changes are done for both BlenderMeshObject and BlenderVolmeshObjectr

Licini · 2024-06-10T16:40:32Z

@tomvanmele Ok this one is done and ready. Could you take a look again?
I know it's a lot files but all following same pattern.

tomvanmele

LGTM. just a few notes/reminders

show_wires (Blender) is actually the same as show_lines. show_wires is a better name in my opinion
disjoint (Rhino) and not shade_smooth (Blender) are the same
Frame, Plane, Shape, Curve, Surface should have dedicated base classes
points of Line, Polyline, Polygon, Polyhedron, Curve, Surface are in fact "control points" and should perhaps have a dedicated color attribute for when they are turned "on".

src/compas/scene/geometryobject.py

src/compas/scene/volmeshobject.py

tomvanmele · 2024-06-10T16:52:45Z

src/compas_blender/scene/circleobject.py

-
-    def __init__(self, circle: Circle, **kwargs: Any):
-        super().__init__(geometry=circle, **kwargs)
+    """Scene object for drawing circles in Blender."""

    def draw(self, color: Optional[Color] = None, collection: Optional[str] = None) -> list[bpy.types.Object]:


here we should also remove the parameters, but this can be in the next PR

tomvanmele · 2024-06-10T16:57:53Z

src/compas_blender/scene/meshobject.py

@@ -153,7 +173,7 @@ def draw_edges(self) -> List[bpy.types.Object]:
        for u, v in edges:
            name = f"{self.mesh.name}.edge.{u}-{v}"
            color = self.edgecolor[u, v]
-            curve = conversions.line_to_blender_curve(Line(self.vertex_xyz[u], self.vertex_xyz[v]))
+            curve = conversions.line_to_blender_curve(Line(self.mesh.vertex_coordinates(u), self.mesh.vertex_coordinates(v)))


why do you change this back? vertex_xyz are the cached coordinates...

tomvanmele · 2024-06-10T16:58:00Z

src/compas_blender/scene/meshobject.py

@@ -177,7 +197,7 @@ def draw_faces(self) -> List[bpy.types.Object]:
        for face in faces:
            name = f"{self.mesh.name}.face.{face}"
            color = self.facecolor[face]
-            points = [self.vertex_xyz[vertex] for vertex in self.mesh.face_vertices(face)]
+            points = [self.mesh.vertex_coordinates(vertex) for vertex in self.mesh.face_vertices(face)]


tomvanmele · 2024-06-10T16:58:17Z

src/compas_blender/scene/volmeshobject.py

@@ -152,7 +171,7 @@ def draw_edges(self) -> List[bpy.types.Object]:
        for u, v in edges:
            name = f"{self.volmesh.name}.edge.{u}-{v}"
            color = self.edgecolor[u, v]
-            curve = conversions.line_to_blender_curve(Line(self.vertex_xyz[u], self.vertex_xyz[v]))
+            curve = conversions.line_to_blender_curve(Line(self.volmesh.vertex_coordinates(u), self.volmesh.vertex_coordinates(v)))


why do you change this back?

tomvanmele · 2024-06-10T17:06:08Z

the revert of using self.vertex_xyz is not correct in my opinion...

Licini · 2024-06-11T12:15:21Z

the revert of using self.vertex_xyz is not correct in my opinion...

This was because the update_object that is called later inside these function will apply the transformation again. So the object will be transformed twice:

from compas.scene import Scene
from compas.datastructures import Graph

from compas.geometry import Translation


graph = Graph.from_nodes_and_edges([(0, 0, 0), (0, -1.5, 0), (-1, 1, 0), (1, 1, 0)], [(0, 1), (0, 2), (0, 3)])



scene = Scene()

obj = scene.add(graph, nodesize=0.2)
obj.transformation = Translation.from_vector([10, 0, 0])
scene.draw()

See in picture it is moved 20 instead of 10

tomvanmele · 2024-06-11T15:24:01Z

src/compas/scene/volmeshobject.py

@@ -78,7 +78,7 @@ def __init__(
        cellcolor=None,
        vertexsize=1.0,
        edgewidth=1.0,
-        **kwargs, # fmt: skip
+        **kwargs,  # fmt: skip


this is only useful if you remove the comma after **kwargs

item as kwarg

42dcc41

Licini requested review from gonzalocasas, chenkasirer and tomvanmele May 13, 2024 15:45

tomvanmele reviewed May 13, 2024

View reviewed changes

src/compas/scene/sceneobject.py Show resolved Hide resolved

Licini added 2 commits May 22, 2024 18:54

all kwargs and item only

d876e20

Merge branch 'main' into update/sceneobject

af635d7

Licini marked this pull request as draft May 22, 2024 16:55

explicit kwargs

6b12b2e

Licini commented May 22, 2024

View reviewed changes

items are read-only

cdb7eae

Licini commented May 22, 2024

View reviewed changes

Licini requested a review from tomvanmele May 22, 2024 17:46

tomvanmele marked this pull request as ready for review May 23, 2024 10:13

tomvanmele reviewed May 23, 2024

View reviewed changes

src/compas/scene/graphobject.py Outdated Show resolved Hide resolved

src/compas/scene/meshobject.py Outdated Show resolved Hide resolved

src/compas/scene/volmeshobject.py Outdated Show resolved Hide resolved

Licini added 3 commits June 7, 2024 11:17

Merge branch 'main' into update/sceneobject

afa6eef

explicit kwargs

0ac89d6

kwargs for rhino scene objects

5f7dd1d

Licini mentioned this pull request Jun 7, 2024

Fix Scene Object implementation consistency and Scene serialisation #1367

Merged

10 tasks

Licini added 8 commits June 10, 2024 14:57

Merge branch 'main' into update/sceneobject

70f63f9

post-merge adjust

70df234

lint

2ccb969

remove redundant inits for ghpython

83db8e9

lint

e13685e

same for blender

0f051b0

remove

34e0650

fix double translation issue

47a0f53

Licini requested a review from tomvanmele June 10, 2024 16:37

Licini commented Jun 10, 2024

View reviewed changes

tomvanmele approved these changes Jun 10, 2024

View reviewed changes

Licini mentioned this pull request Jun 11, 2024

Notes for visualisation parameters #1370

Open

Licini added 2 commits June 11, 2024 14:16

small fixes

6f3c1c4

lint

0c8826e

tomvanmele reviewed Jun 11, 2024

View reviewed changes

tomvanmele merged commit 9856318 into main Jun 11, 2024
15 checks passed

Licini mentioned this pull request Jun 13, 2024

Item as kwarg compas-dev/compas_viewer#177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

item as kwarg #1353

item as kwarg #1353

Licini commented May 13, 2024

Licini commented May 22, 2024 •

edited

Loading

Licini May 22, 2024

Licini May 22, 2024

Licini May 22, 2024 •

edited

Loading

Licini May 22, 2024 •

edited

Loading

Licini Jun 10, 2024

Licini commented Jun 10, 2024

tomvanmele left a comment

tomvanmele Jun 10, 2024

tomvanmele Jun 10, 2024

tomvanmele Jun 10, 2024

tomvanmele Jun 10, 2024

tomvanmele commented Jun 10, 2024

Licini commented Jun 11, 2024 •

edited

Loading

tomvanmele Jun 11, 2024

item as kwarg #1353

item as kwarg #1353

Conversation

Licini commented May 13, 2024

Licini commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licini May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Licini May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licini commented Jun 10, 2024

tomvanmele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomvanmele commented Jun 10, 2024

Licini commented Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Licini commented May 22, 2024 •

edited

Loading

Licini May 22, 2024 •

edited

Loading

Licini May 22, 2024 •

edited

Loading

Licini commented Jun 11, 2024 •

edited

Loading