webots icon indicating copy to clipboard operation
webots copied to clipboard

Add `Pose` node (like `Transform` node but without `scale`)

Open ad-daniel opened this issue 3 years ago • 1 comments

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 https://github.com/cyberbotics/webots/issues/4034

Tasks Add the list of tasks of this PR.

  • [x] add Pose node
    • [x] rename Transform to Pose
    • [x] rename AbstractTransform to AbstractPose
    • [x] rename node constants
    • [x] rename utilities like findUpperTransform, findUppermostTransform, ...
    • [x] rename variables, like WbPose *transform to WbPose *pose
  • [x] create WbTransform class that inherits from WbPose
  • [x] make WbMatter inherit from WbPose
  • [x] make WbSkin inherit from WbTransform instead of WbAbstractTransform
  • [x] 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 avatar Dec 23 '21 09:12 ad-daniel

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.

Justin-Fisher avatar May 23 '22 22:05 Justin-Fisher

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.

stefaniapedrazzi avatar Mar 08 '23 09:03 stefaniapedrazzi

~~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

ad-daniel avatar Mar 08 '23 10:03 ad-daniel

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.

stefaniapedrazzi avatar Mar 08 '23 10:03 stefaniapedrazzi

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

ad-daniel avatar Mar 08 '23 10:03 ad-daniel

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.

stefaniapedrazzi avatar Mar 08 '23 11:03 stefaniapedrazzi

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)?

ad-daniel avatar Mar 08 '23 11:03 ad-daniel

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.

stefaniapedrazzi avatar Mar 08 '23 11:03 stefaniapedrazzi

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 avatar Mar 08 '23 11:03 ad-daniel

continued here: https://github.com/cyberbotics/webots/pull/5978

ad-daniel avatar Mar 13 '23 10:03 ad-daniel