Merging SceneObjectNode and SceneObject

[Licini]

The relation between SceneObjectNode and SceneObject was too convoluted. This PR merge them into one by having SceneObject inherent from TreeNode. This simplifies a lot on operations to access parent/ children object or a tree that an object is attachted too. While the most-outer level user APIs stays untouched. This should also make it easier from creating a compas_model object.

from compas.scene import Scene
from compas.geometry import Box
from compas.geometry import Sphere
from compas.geometry import Cylinder
from compas.geometry import Frame
from compas.geometry import Sphere



box = Box.from_width_height_depth(1, 1, 1)
cylinder = Cylinder(1, 1, Frame.worldXY())
sphere = Sphere(1)

scene = Scene()
scene.clear()

obj = scene.add(box).add(cylinder).add(sphere)

scene.print_hierarchy()


print(obj.tree)

└── <TreeNode: ROOT>
    └── <GeometryObject: Box>
        └── <GeometryObject: Cylinder>
            └── <GeometryObject: Sphere>
<Tree with 4 nodes>

What type of change is this?

• [ ] Bug fix in a backwards-compatible manner.
• [ ] New feature in a backwards-compatible manner.
• [ ] Breaking change: bug fix or new feature that involve incompatible API changes.
• [x] Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [x] I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
• [x] I ran all tests on my computer and it's all green (i.e. invoke test).
• [x] I ran lint on my computer and there are no errors (i.e. invoke lint).
• [ ] I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
• [ ] I have added tests that prove my fix is effective or that my feature works.
• [x] I have added necessary documentation (if appropriate)

[tomvanmele]

i don't think this helps with the model problem. from an item you can access the parent node and from the parent node you can reach the tree. but you still can't reach the scene from the tree.

perhaps it would make sense to make Scene a subclass from Tree? might feel more logical since a scene object is a tree node. the problem of reaching the scene is them immediately solved.

[tomvanmele]

i added a few changes that make the scene work for recursive addition of objects in, for example a model. added the type annotations for debugging. we can remove them again...

[tomvanmele]

i removed viewer detection and the viewer instance attribute on the scene. as explained, i think we should rather do the opposite...

[Licini]

i don't think this helps with the model problem. from an item you can access the parent node and from the parent node you can reach the tree. but you still can't reach the scene from the tree.

perhaps it would make sense to make Scene a subclass from Tree? might feel more logical since a scene object is a tree node. the problem of reaching the scene is them immediately solved.

I also think making Scene inherent from Tree is a good idea. Especially when SceneObject is the node. Will give this a try now on this branch

[Licini]

@tomvanmele

1. How about now? Scene is now inheriting directly from Tree.
2. Regarding documenting the kwargs. Should we also explicitly write all of them out then pass on to super init, so it can be suggested pylance? For example:

def __init__(kwag_x=None, kwag_y=None):
    super(XXX, self).__init__(kwag_x=kwag_x, kwag_y=kwag_y)

[tomvanmele]

regarding**kwargs...

in my opinion this should only be used when additional named arguments can be expected that are not known to the current class and need to be passed on to the class ancestors. arguments that are expected and used in the current scope should always be listed, processed, and documented explicitly...

what i think we should most certainly NOT do is the following:

def __init__(self, **kwargs):
    super().__init__(**kwargs)
    self.a = kwargs.get('a')
    self.b = kwargs.get('b')

i would also vote for using ONLY named arguments in __init__ functions in the future, because passing of arguments to parent classes is complicated when there is a mixture of positional and named arguments...

[Licini]

regarding**kwargs...

in my opinion this should only be used when additional named arguments can be expected that are not known to the current class and need to be passed on to the class ancestors. arguments that are expected and used in the current scope should always be listed, processed, and documented explicitly...

what i think we should most certainly NOT do is the following:

def __init__(self, **kwargs):
    super().__init__(**kwargs)
    self.a = kwargs.get('a')
    self.b = kwargs.get('b')

i would also vote for using ONLY named arguments in __init__ functions in the future, because passing of arguments to parent classes is complicated when there is a mixture of positional and named arguments...

Sounds good, will make everything named argument in the scope of this PR and make a separate one after for rest of places. And will fix all the docs and remove the code commented out

[Licini]

@tomvanmele cleaned up, shall we merge?

[Licini]

@tomvanmele let's merge this?