Skip to content

add group#1444

Merged
Licini merged 5 commits intomainfrom
group
Mar 6, 2025
Merged

add group#1444
Licini merged 5 commits intomainfrom
group

Conversation

@Licini
Copy link
Contributor

@Licini Licini commented Mar 5, 2025

Adding a Group object to Scene, which is a custom SceneObject. The usages are similar to Three.js:

from compas.scene import Group
from compas.scene import Scene
from compas.geometry import Point

scene = Scene()

group = Group(name="My Group")
scene.add(group)

point = Point(0, 0, 0)
group.add(point)

print(scene)
<Tree with 3 nodes>
└── <TreeNode: ROOT>
    └── <Group: My Group>
        └── <GeometryObject: Point>

@Licini Licini requested a review from tomvanmele March 5, 2025 13:31
@codecov
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.85%. Comparing base (333d0a8) to head (fc52d3d).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
+ Coverage   61.83%   61.85%   +0.01%     
==========================================
  Files         207      208       +1     
  Lines       22321    22333      +12     
==========================================
+ Hits        13803    13814      +11     
- Misses       8518     8519       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, it's surprising that such a small change creates this feature, I guess this is just giving it a cleaner name to abusing empty SceneObjects

@Licini
Copy link
Contributor Author

Licini commented Mar 6, 2025

I think this looks good, it's surprising that such a small change creates this feature, I guess this is just giving it a cleaner name to abusing empty SceneObjects

@gonzalocasas I realized an issue when I'm adding group to the serialization test, where Data can not be serialized without __ data __ implemented, which means we still have to extend Data even it is not really doing much here..., in this case allowing SceneObject to take None item is perhaps a better route. It can also reduce footprint of serialized Scene especially when we have a lot of groups. Could you take a look again let me know what you think?

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just a minor remark

): # fmt: skip
# type: (...) -> None
if not isinstance(item, Data):
if not isinstance(item, (Data, type(None))):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be: if item and not isinstance(item, Data)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, looks more elegant, updated!

@tomvanmele
Copy link
Member

tomvanmele commented Mar 6, 2025

Also, i was wondering if it would be cleaner to use a factory classmethod on SceneObject, instead of overwriting __new__

@Licini
Copy link
Contributor Author

Licini commented Mar 6, 2025

Also, i was wondering if it would be cleaner to use a factory classmethod on SceneObject, instead of overwriting __new__

Sure, Factory pattern could be more readable than __ new __, I can experiment the idea a bit in another PR

@Licini Licini merged commit 8fb5ecb into main Mar 6, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants