-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scene object refactoring #1209
Scene object refactoring #1209
Conversation
@tomvanmele @gonzalocasas @chenkasirer Ok this one is ready. As discussed I'd like to make this PR only about renaming and make the basic scene setup work. Plz see the script at
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example here
@@ -17,11 +17,11 @@ | |||
# Visualisation | |||
# ============================================================================== | |||
|
|||
Artist.clear() | |||
SceneObject.clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong and there are many like this.
however, all files in this folder were copied from occ to be able to do some compatibility testing.
they are not meant to be part of the docs.
please remove...
src/compas/scene/geometryobject.py
Outdated
class GeometryArtist(Artist): | ||
"""Base class for artists for geometry objects. | ||
class GeometryObject(SceneObject): | ||
"""Base class for sceneobjects for geometry objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps do find replace scene object
@@ -145,7 +145,7 @@ def draw_nodes( | |||
for node in nodes or self.network.nodes(): # type: ignore | |||
name = f"{self.network.name}.node.{node}" # type: ignore | |||
color = self.node_color[node] # type: ignore | |||
point = self.node_xyz[node] # type: ignore | |||
point = self.network.nodes_attributes("xyz")[node] # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you changing this everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These was because they were creating double transformation. the node_xyz
already preforms the transformation, then later self.update_object
is doing that again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but then perhaps remove the node_xyz dict
@@ -153,7 +153,7 @@ def draw_vertices( | |||
for vertex in vertices or self.volmesh.vertices(): # type: ignore | |||
name = f"{self.volmesh.name}.vertex.{vertex}" # type: ignore | |||
color = self.vertex_color[vertex] # type: ignore | |||
point = self.vertex_xyz[vertex] | |||
point = self.volmesh.vertices_attributes("xyz")[vertex] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing this everywhere?
@tomvanmele all done, let me know how it looks now : ) |
@@ -193,7 +193,7 @@ def get_geometry(self, with_features=False): | |||
""" | |||
Returns a transformed copy of the part's geometry. | |||
|
|||
The returned type can be drawn with an Artist. | |||
The returned type can be drawn with an scene object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an => a
src/compas/scene/volmeshobject.py
Outdated
class VolMeshArtist(Artist): | ||
"""Artist for drawing volmesh data structures. | ||
class VolMeshObject(SceneObject): | ||
"""Sceneobject for drawing volmesh data structures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scene object?
SceneObject?
class BoxArtist(BlenderArtist, GeometryArtist): | ||
"""Artist for drawing box shapes in Blender. | ||
class BoxObject(BlenderSceneObject, GeometryObject): | ||
"""Sceneobject for drawing box shapes in Blender. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question everywhere i guess
Oops, all fixed 🤣 @tomvanmele |
Hey guys, so this PR is about the refactoring all
Artist
toSceneObject
. It may look like a huge PR, but besides the addition ofscene
class, all the rests are renaming and adjusting importing path. Basicallycompas.artists
is nowcompas.scene
.Before I make the file changing list even longer, I want to make sure if we all agree on this new folder structure and naming convention:
Artist
toSceneObject
MeshArtist
toMeshObject
,NetworkArtist
toNetworkObject
etc.Scene
andSceneObject
is now undercompas.scene
, let me know if you guys think there should be a separate folder ofcompas.sceneobjects
.Now I have done renaming for compas core folder. If you guys are OK with this pattern I will finish the rest for compas_rhino, compas_ghpython and compas_blender.