Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1461 +/- ##
==========================================
- Coverage 61.94% 61.93% -0.01%
==========================================
Files 208 208
Lines 22365 22401 +36
==========================================
+ Hits 13853 13875 +22
- Misses 8512 8526 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Consolidating _get_sceneobject_cls and get_sceneobject_cls into one function. Also cleaned up the kwargs. I removed sceneobject_type parameter because now you can call constructor directly with __new__ removed.
|
|
||
| def __new__(cls, *args, **kwargs): | ||
| # overwriting __new__ to revert to the default behavior of normal object, So an instance can be created directly without providing a registered item. | ||
| return object.__new__(cls) | ||
|
|
||
| @property | ||
| def __data__(self): | ||
| # type: () -> dict | ||
| data = { | ||
| "settings": self.settings, | ||
| "children": [child.__data__ for child in self.children], | ||
| } | ||
| return data |
There was a problem hiding this comment.
These customizations are no longer needed anymore, thanks to the more straight forward serialization/deserialization
| @property | ||
| def __data__(self): | ||
| # type: () -> dict | ||
| items = {str(object.item.guid): object.item for object in self.objects if object.item is not None} | ||
| return { | ||
| "name": self.name, | ||
| "root": self.root.__data__, # type: ignore | ||
| "items": list(items.values()), | ||
| "attributes": self.attributes, | ||
| "datastore": self.datastore, | ||
| "objectstore": self.objectstore, | ||
| "tree": self.tree, | ||
| } | ||
|
|
||
| @classmethod | ||
| def __from_data__(cls, data): | ||
| # type: (dict) -> Scene | ||
| scene = cls(data["name"]) | ||
| items = {str(item.guid): item for item in data["items"]} | ||
|
|
||
| def add(node, parent, items): | ||
| for child_node in node.get("children", []): | ||
| settings = child_node["settings"] | ||
| if "item" in child_node: | ||
| guid = child_node["item"] | ||
| sceneobject = parent.add(items[guid], **settings) | ||
| else: | ||
| sceneobject = parent.add(Group(**settings)) | ||
| add(child_node, sceneobject, items) | ||
|
|
||
| add(data["root"], scene, items) | ||
|
|
||
| return scene |
There was a problem hiding this comment.
We directly serialize datastore, objectstore and tree here. And during deserialization we can read them as they are without using a custom __from__data__ method.
| # type: (str, str | None) -> None | ||
| super(Scene, self).__init__(name=name) | ||
| super(Scene, self).add(TreeNode(name="ROOT")) | ||
| def __init__(self, context=None, datastore=None, objectstore=None, tree=None, **kwargs): |
There was a problem hiding this comment.
Users can passing their own customs stores and tree, if they know what they are doing.
| def item(self): | ||
| # type: () -> compas.data.Data | ||
| return self._item | ||
| return self.scene.datastore[self._item] |
There was a problem hiding this comment.
item now becomes a getter that retrieve the live object from datastore, what we actually store as _item is the guid
| @property | ||
| def is_root(self): | ||
| # type: () -> bool | ||
| return self.node.is_root | ||
|
|
||
| @property | ||
| def is_leaf(self): | ||
| # type: () -> bool | ||
| return self.node.is_leaf | ||
|
|
||
| @property | ||
| def is_branch(self): | ||
| # type: () -> bool | ||
| return self.node.is_branch | ||
|
|
||
| @property | ||
| def parentnode(self): | ||
| # type: () -> compas.datastructures.Node | None | ||
| return self.node.parent | ||
|
|
||
| @property | ||
| def childnodes(self): | ||
| # type: () -> list[compas.datastructures.Node] | ||
| return self.node.children |
There was a problem hiding this comment.
"shortcut" properties that allows us to still work with SceneObject like a TreeNode
|
@Licini shall we try to finish this? |
|
|
||
| from compas.scene import SceneObject | ||
| from compas.scene import sceneobject_factory | ||
|
|
||
|
|
||
| class CompasToRhinoGeometry(component): | ||
| def RunScript(self, cg): | ||
| if not cg: | ||
| return None | ||
|
|
||
| return SceneObject(item=cg).draw() | ||
| return sceneobject_factory(item=cg).draw() |
There was a problem hiding this comment.
@gonzalocasas @chenkasirer Please note here, with the factory pattern, you need call factory function in GH components instead of constructor
There was a problem hiding this comment.
This is a breaking change that affects a lot of external code. It needs a major release (compas 3!). Also, I don't like the method name sceneobject_factory, it feels old, could we add the factory method as a class/static method of SceneObject, like SceneObject.create()? the word factory on its own sounds too much java-like to my taste :P
There was a problem hiding this comment.
This is a breaking change that affects a lot of external code. It needs a major release (compas 3!). Also, I don't like the method name
sceneobject_factory, it feels old, could we add the factory method as a class/static method ofSceneObject, likeSceneObject.create()? the word factory on its own sounds too much java-like to my taste :P
Ok 🤣 I had impression that these are the only places SceneOjbect constructor are called. But sure SceneObject.create() is a good idea to me too. Regarding the full PR, I have been having more discussions @tomvanmele on potential issues and corresponding reworks for data store, so I will close this one for now and re-submit another one that only concerns the serialization part. We can deal with __new__ in the future in compas 3
Done with the requested changes |
3 participants
Hey guys a rather large PR, there are several components to this, let me explain:
1. Scene data store
Previously we inherent
ScenefromTreeandSceneObjectfromTreeNode. It was nice but the serialization was not very straightforward, because we don't want to serializeDataitem directly underSceneObjectsince they might be reused. So we had to customize the serialization and deserialization to only storeguidof item underSceneObject, then group all the distinct items as a list under scene.This PR basically makes the idea more explicit by reflecting this mechanism directly as the attributes of
Scene. UnderScene, we have 3 main aspects:datastore,objectstoreandtree. Thedatastoreis a bucket of uniqueDataitems, theobjectstoreis a bucket ofSceneObjects, both keyed by theirguids. Andtreestores hierarchy of the scene objects, but nothing more.For
SceneObject, instead maintaining a live link toDataitem, we only actually storeguidof that item and use the datastore of the scene to retrieve it (with minimal overhead).The benefit is a much more transparent serialization and deserialization process: now both
SceneandSceneObjectcan be json dumped/loaded without any customization using our default encoder/decoder. The structure of two classes directly reflect the serialized data with almost no change.This also makes it easier for further extension of
Scenefor example if you want to use a datastore that contains very large geometry but with lazy loading, you can pass in a customdatastoreyourself when creating the scene, using dict-like object which can be queried with aguid.For example a minimal serialized scene can look like this now:
2. SceneObjectFactory
We have been using
__new__for automatically choosing appropriateSceneObjectclass. It has been working well most of time. But it becomes annoying if one wants to initialize a customSceneObjectclass likeGroup, where I just want a straightforward initiation without triggering auto class detection. @tomvanmele has been advocating for more explicit Factory method instead__new__, to make the constructor behavior more predictable and more type-hint friendly. Which I quite agree.This does not change the external APIs, users still use
Scene.addthe same way, which internally calls the factory method now instead of the class constructor.3. Others
I added more tests on
Sceneto make sure it behaves correctly. In summary, although there are a big internal re-organization inSceneandSceneObject, the user-facing APIs aren't changed, there also shouldn't be any changes needed for all the extendedSceneObjects. I will drop some in-line comments below