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 (like Transform node but without scale) #4079

Closed
wants to merge 14 commits into from

Conversation

ad-daniel
Copy link
Contributor

@ad-daniel ad-daniel commented Dec 23, 2021

Description
Scaling of robots is problematic due to the presence of sensors and physics. The Pose node, from which Transform, Solids and Robots will inherit, is much like a Transform node but without the scale parameter. This latter will be exposed by the "new" Transform node, in order to maintain backwards compatibility.

Related Issues
Fixes #4034

Tasks
Add the list of tasks of this PR.

  • add Pose node
    • rename Transform to Pose
    • rename AbstractTransform to AbstractPose
    • rename node constants
    • rename utilities like findUpperTransform, findUppermostTransform, ...
    • rename variables, like WbPose *transform to WbPose *pose
  • create WbTransform class that inherits from WbPose
  • make WbMatter inherit from WbPose
  • make WbSkin inherit from WbTransform instead of WbAbstractTransform
  • remove scale from Pose (and all references to it in Solid, Robot, ... )
  • check WbTrack functionality
  • only allow Shape children in Transform nodes
  • boundingObjects should allow only Pose
  • x3d exporting of transform/pose
  • update docs
  • update node chart
  • script to automatically detect/change Transform { Solid {}} structures into Pose { Solid {}} (should be rare)

@ad-daniel ad-daniel added the enhancement Implementation of a minor feature label Dec 23, 2021
@ad-daniel ad-daniel added this to the R2022b milestone Dec 23, 2021
@ad-daniel ad-daniel self-assigned this Dec 23, 2021
@Justin-Fisher
Copy link
Contributor

Justin-Fisher commented May 23, 2022

If it were possible, my inclination would be to instead move in the direction of letting scale work meaningfully with more things, rather than reducing the number of things that have a scale field. E.g., it'd be nice to be able to make oblate spheroid bounding objects. Or is it prohibitively difficult to get the physics to work right with those?

I did run into a lot of perplexment once when I had scaled a Camera node to make its visual appear the way I wanted, and discovered that this scale also squashed the camera's image (or at least the location of recognition objects on the image)! That surprised me and took awhile to debug, but in the end I decided that was probably sort of working as designed. But this "fix" probably would have kept me from stumbling into such perplexment, by preventing me from scaling the camera at all.

Also I have doubts about the claim that it "should be rare" that people would have Solid descendants inside a Transform node. I think I have a fair number of those, since this is an intuitive way to position a group of items, including some uses in places that your script wouldn't reach (content to be imported into the world via supervisor). The backwards incompatibility issues here would work out to be a headache for me, though perhaps I'm unusual. Again, that would make me lean in the direction of a different solution, e.g., making scale work as you would intuitively expect it to on solids if that's feasible, or just making scale fields have no effect on solids/robots, but not actually removing those fields or forcing people to convert old node-types to new ones.

@ad-daniel ad-daniel modified the milestones: R2022b, R2022c Jun 22, 2022
@ygoumaz ygoumaz modified the milestones: R2023a, R2023b Nov 14, 2022
@stefaniapedrazzi
Copy link
Member

make WbSkin inherit from WbTransform instead of WbAbstractTransform

Note that the Skinnode should not have the children field, so if Transform does it then Skin cannot inherit from Transform and we still need an intermediate node like WbAbstractTransform.

@ad-daniel
Copy link
Contributor Author

ad-daniel commented Mar 8, 2023

mmh, that might be a bit of a problem. I assume the skin node does require the scale, correct?
A better approach might be to consider the Skin an exception and be its own thing, otherwise it gets messy

@stefaniapedrazzi
Copy link
Member

stefaniapedrazzi commented Mar 8, 2023

mmh, that might be a bit of a problem. I assume the skin node does require the scale, correct?

Yes.
The WbAbstractTrasform class has been introduced to avoid duplicating all the translation/rotation/scale-related code in WbTransform and WbSkin.
Changing WbSkin so that inherits from the new WbAbstractPose would at least avoid duplication of code related to translation and rotation. It should be evaluated the size of the scale-related code.

@ad-daniel
Copy link
Contributor Author

ad-daniel commented Mar 8, 2023

Changing WbSkin so that inherits from the new WbAbstractPose would at least avoid duplication of code related to translation and rotation. It should be evaluated the size of the scale-related code.

I see, I'm not entirely sure WbAbstractPose would even need to exist after the rework to be honest. Since the scale-related methods would be moved down to WbTransform, the only difference that remains between WbPose & WbAbstractPose is the children field. After discussing with Olivier, it might be cleaner to just have a Pose node that inherits from Group (hence avoid multiple inheritance too) and consider the Skin its own thing. It would entail some duplication that's true, but frankly given that the scale now drop down at the Transform level, I'm not convinced the benefit of code-reutilization justifies adding an intermediate Frankenstein just for the Skin in order to have both a scale but not the children field, making the two coexist might turn out worse than duplicating translation/rotation. There's a lot of moving pieces though, so I'll investigate further

As it stands, it would look something like this, but like I mentioned there might be roadblocks:

WbGroup (children)
   |
   |
   |__ WbPose (add translation + rotation)
         |
         |__ WbSolid (add solid stuff) __ WbRobot
         |
         |__ WbTransform (add scale)

WbSkin

@stefaniapedrazzi
Copy link
Member

stefaniapedrazzi commented Mar 8, 2023

If WbSkin doesn't have a common base class without the children field with WbPose you have to duplicate all the code to handle the translation and rotation in the WbSkin.
This will mean that almost all the code that now it is in WbAbstractTransform should be added both in WbSkin AND WbPose.
It is quite a lot of code and duplicating (even ignoring the scale part) will pose many maintenance issues on the long term.

@ad-daniel
Copy link
Contributor Author

ad-daniel commented Mar 8, 2023

How do you suggest Skin to have scale but not children in this context (i.e. with the addition of the Pose node that doesn't have scale)?

@stefaniapedrazzi
Copy link
Member

I would keep at least the WbAbstractPose class including all the translation/rotation code and use it as base class for the WbPose and WbSkin classes.

@ad-daniel
Copy link
Contributor Author

ad-daniel commented Mar 8, 2023

I'm fine with that, but that would still leave duplication for the scale, as I don't see another way of adding scale support without also getting children in the mix (or at least, not without making a mess with the classes)

@ad-daniel
Copy link
Contributor Author

continued here: #5978

@ad-daniel ad-daniel closed this Mar 13, 2023
@ad-daniel ad-daniel deleted the enhancement-remove-scale-from-transform branch March 13, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Implementation of a minor feature
Development

Successfully merging this pull request may close these issues.

4 participants