Skip to content

Update relation between frame and transformation#1443

Merged
Licini merged 7 commits intomainfrom
scene
Mar 19, 2025
Merged

Update relation between frame and transformation#1443
Licini merged 7 commits intomainfrom
scene

Conversation

@Licini
Copy link
Copy Markdown
Contributor

@Licini Licini commented Mar 5, 2025

Hey folks, I'd like propose to update the relation between frame and transformation in SceneObject, to align better with how they are currently defined in compas_model (https://github.com/BlockResearchGroup/compas_model/blob/b8d7a992956abcfe96d41eb11fecd5a064a12674/src/compas_model/elements/element.py#L271), which I found makes more sense and easier to understand.

  • We update the calculation of worldtransformation to the simple multiplication of all transformations along the scene tree.
  • We re-define the frame property as a read-only computed property which is derived from Frame.from_transformation(self.worldtransformation), and it can be think of as the reference frame of the local coordinate system of this object after all the transformations applied.

This relation also aligns better with other CAD systems and libraries like Threejs. And it will give a lot of advantage for compas_viewer where I will be able to push the transformation and scene tree into GPU and calculate the multiplication there.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.86%. Comparing base (a6a0dec) to head (558385b).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
- Coverage   61.88%   61.86%   -0.03%     
==========================================
  Files         208      208              
  Lines       22333    22327       -6     
==========================================
- Hits        13821    13812       -9     
- Misses       8512     8515       +3     

☔ 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
Copy Markdown
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.

i think we have to add a bit of documentation

Comment thread src/compas/scene/sceneobject.py
Comment thread src/compas/scene/sceneobject.py Outdated
frame_stack = [self.frame] if self.frame else []
parent = self.parent
transformations = [self.transformation] if self.transformation else []
parent: SceneObject = self.parent
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i like your enthusiasm, but this annotation can't be introduced yet in the current version of COMPAS...

Copy link
Copy Markdown
Contributor Author

@Licini Licini Mar 7, 2025

Choose a reason for hiding this comment

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

removed, i got so used to it 😂

assert sceneobj1.frame == Frame([10.0, 0.0, 0.0], [1.0, 0.0, 0.0], [0.0, 1.0, 0.0])

sceneobj2 = scene.add(Box(), parent=sceneobj1)
sceneobj2.frame = Frame([1.0, 1.0, 0.0], xaxis=[1.0, 0.0, 0.0], yaxis=[0.0, 1.0, 0.0])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

by simply removing the frame, you're not reproducing the same result as before...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, but if you see in next few lines, the assert condition was updated according to the new computing scheme, or perhaps I misunderstood something here?

@Licini Licini merged commit ea17a24 into main Mar 19, 2025
18 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.

2 participants