Skip to content
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

Add Pose node #5978

Merged
merged 222 commits into from
May 11, 2023
Merged

Add Pose node #5978

merged 222 commits into from
May 11, 2023

Conversation

ad-daniel
Copy link
Contributor

@ad-daniel ad-daniel commented Mar 10, 2023

This PR aims at introducing a new node named Pose which basically inherits from Group, adding only the translation and rotation field. The Transform node inherit from Pose adding the scale node.
All Solid nodes inherit from Pose and not from Transform any more.
The Transform node may only contain Shape, CadShape, Group, Pose or Transform nodes in its children list.
This has 3 mains advantages:

  • It makes it clearer for the end-user that it is not possible to scale Solid nodes (however PROTOs of Solid nodes may still have a working scale parameter, which may be either scalar or vector).
  • It simplifies of the management of Solid nodes and bounding objects.
  • It simplifies the management of Device nodes (which inherit from Solid).

Scaling Solid node is not something realistic as in the real world, it is not possible (or very difficult) to scale objects, whereas it is super easy to change their translation and rotation.

See the most relevant update documentation pages:

@ad-daniel ad-daniel added the feature Implementation of a major feature label Mar 10, 2023
@ad-daniel ad-daniel added this to the R2023b milestone Mar 10, 2023
@ad-daniel ad-daniel self-assigned this Mar 10, 2023
@ad-daniel ad-daniel added the test suite Start the test suite label Mar 10, 2023
BenjaminDeleze
BenjaminDeleze previously approved these changes May 10, 2023
Copy link

@BenjaminDeleze BenjaminDeleze left a comment

Choose a reason for hiding this comment

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

Seems good to me

@omichel omichel requested a review from ygoumaz May 10, 2023 14:39
omichel
omichel previously approved these changes May 10, 2023
@omichel
Copy link
Member

omichel commented May 10, 2023

@stefaniapedrazzi and @ygoumaz: can you please also review this PR again before I merge it?

@stefaniapedrazzi
Copy link
Member

stefaniapedrazzi commented May 11, 2023

I didn't find any major issue.

  • However, I'm not sure it is good to allow to transform a Solid node into a Transform node (as in the example of the Table described by @BenjaminDeleze) if the Solid has Solid children because unexpectedly all the children are removed.
    I think it would be more user-friendly to prevent this operation.

  • The c3d_viewer.py controller is throwing an error because it imports a C3dBocyRepresentation with the scale field that has been removed.

@omichel
Copy link
Member

omichel commented May 11, 2023

However, I'm not sure it is good to allow to transform a Solid node into a Transform node (as in the example of the Table described by @BenjaminDeleze) if the Solid has Solid children because unexpectedly all the children are removed.
I think it would be more user-friendly to prevent this operation.

Yes, we discussed this point with @BenjaminDeleze yesterday and I believe we could indeed remove this option. However, since there is a warning about loss of information during this operation, I believe it is okay for the user. And also, it seems odd to allow users to transform a Transform into a Solid, but prevent the other way round. For example, with the BiscuitBox, it works better than with the Table because the children of the Solid are Pose nodes. Hence, in that case, you can transform the BiscuitBox to a Solid, and to a Transform and back to a Solid without loosing the geometries (but you will loose the physics, boundingObjects, etc.).

@stefaniapedrazzi
Copy link
Member

Loosing boundingObject or physics is expected and not as problematic as loosing all the children information.
Personally, I don't think that the info is enough in this case and should be more specific.
It is not possible to undo a node transform so it is really annoying to revert it.
Even more because this is a new behavior and was working the same in previous versions.

@omichel omichel dismissed stale reviews from BenjaminDeleze and themself via e02b4fd May 11, 2023 07:52
@ygoumaz
Copy link
Contributor

ygoumaz commented May 11, 2023

I agree with @stefaniapedrazzi that the warning should at least be more specific and list the information that will be lost, in particular children Solid objects.

@omichel
Copy link
Member

omichel commented May 11, 2023

I am improving this warning...

@omichel
Copy link
Member

omichel commented May 11, 2023

I just fixed the two issues reported by @stefaniapedrazzi. I guess we can merge it now?

@ygoumaz
Copy link
Contributor

ygoumaz commented May 11, 2023

When changing the scale of a simple Transform node, it affects the translation and rotation arrows in the view in a wrong way that is not present in develop. It makes them unusable.

scale

@stefaniapedrazzi
Copy link
Member

When changing the scale of a simple Transform node, it affects the translation and rotation arrows in the view in a wrong way that is not present in develop. It makes them unusable.

Just for note, I could reproduce similar issues on master/develop branches ( reported here #6063).

@ygoumaz
Copy link
Contributor

ygoumaz commented May 11, 2023

Ok, I still can produce a different behavior between both branches:

  1. Adding a Transform at the root of the scene tree.
  2. Changing the scale in develop doesn't affect handles, but in feature-pose-node, the handles are scaled too.

However, this additional behavior may be a side effect from the existing bug and not directly related to the addition of the Pose node. We may merge the PR and fix it when addressing #6063.

@omichel
Copy link
Member

omichel commented May 11, 2023

However, this additional behavior may be a side effect from the existing bug and not directly related to the addition of the Pose node. We may merge the PR and fix it when addressing #6063.

Yes, in any case #6063 needs to be fixed and it will change this code. So, I agree we can merge this PR and fix #6063 in a separate PR (this one is already very big).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Implementation of a major feature test sources Start the sources test on all platforms test suite Start the test suite
Development

Successfully merging this pull request may close these issues.

5 participants