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

Changed classes to inherete from compas.Data #387

Merged
merged 13 commits into from
Nov 8, 2023
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

* `CollisionMesh` now inherit from `compas.data.Data`
* `AttachedCollisionMesh` now inherit from `compas.data.Data`
* `Robot` now inherit from `compas.data.Data`
* `RobotSemantics` now inherit from `compas.data.Data`
* `Tool` now inherit from `compas.data.Data`

### Removed


Expand Down
7 changes: 5 additions & 2 deletions src/compas_fab/robots/planning_scene.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from __future__ import division
from __future__ import print_function

from compas.data import Data
from compas.datastructures import Mesh
from compas.geometry import Frame
from compas.geometry import Scale
Expand All @@ -13,7 +14,7 @@
]


class CollisionMesh(object):
class CollisionMesh(Data):
"""Represents a collision mesh.

Parameters
Expand Down Expand Up @@ -55,6 +56,7 @@ class CollisionMesh(object):
"""

def __init__(self, mesh, id, frame=None, root_name=None):
super(CollisionMesh, self).__init__()
self.id = id
self.mesh = mesh
self.frame = frame or Frame.worldXY()
Expand Down Expand Up @@ -130,7 +132,7 @@ def data(self, data_obj):
self.root_name = data_obj["root_name"]


class AttachedCollisionMesh(object):
class AttachedCollisionMesh(Data):
"""Represents a collision mesh that is attached to a :class:`Robot`'s :class:`~compas.robots.Link`.

Parameters
Expand Down Expand Up @@ -169,6 +171,7 @@ class AttachedCollisionMesh(object):
"""

def __init__(self, collision_mesh, link_name, touch_links=None, weight=1.0):
super(AttachedCollisionMesh, self).__init__()
self.collision_mesh = collision_mesh
if self.collision_mesh:
self.collision_mesh.root_name = link_name
Expand Down
39 changes: 35 additions & 4 deletions src/compas_fab/robots/robot.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import random

from compas.data import Data
from compas.geometry import Frame
from compas.geometry import Sphere
from compas.geometry import Transformation
Expand All @@ -21,7 +22,7 @@
]


class Robot(object):
class Robot(Data):
"""Represents a robot.

This class binds together several building blocks, such as the robot's
Expand All @@ -47,15 +48,43 @@ class Robot(object):
Dictionary mapping planning groups to the tool currently attached to them, if any.
"""

def __init__(self, model, artist=None, semantics=None, client=None):
def __init__(self, model=None, artist=None, semantics=None, client=None):
super(Robot, self).__init__()
# These attributes have to be initiated first,
# because they are used in the setters of the other attributes
self._scale_factor = 1.0
self.model = model
self._attached_tools = {} # { planning_group_name: robots.tool.Tool }
self._current_ik = {"request_id": None, "solutions": None}

self.model = model
self.artist = artist
self.semantics = semantics
self.client = client
self.attributes = {}
self._current_ik = {"request_id": None, "solutions": None}

@property
def data(self):
data = {
"_scale_factor": self._scale_factor,
"_attached_tools": self._attached_tools,
"_current_ik": self._current_ik,
Copy link
Member

Choose a reason for hiding this comment

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

why the _s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't invent this particular variable, and it was there all along, I just serialized it. I believe the intent of the existing code is that the model is not supposed to keep track of extrinsic or modifiable properties of the robot (in this case, the current_ik ). Hence the private _ notation. I should probably have another PR to address the intrinsic vs extrinsic state of the robot problem.

Copy link
Member

Choose a reason for hiding this comment

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

I would remove the _ in the keys.

Copy link
Member

Choose a reason for hiding this comment

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

also, current_ik should not be serialized

"model": self.model.data,
"semantics": self.semantics,
"attributes": self.attributes,
# The following attributes cannnot be serizlied
# "artist": self.artist.data if self.artist else None,
# "client": self.client.data if self.client else None,
yck011522 marked this conversation as resolved.
Show resolved Hide resolved
}
return data

@data.setter
def data(self, data):
self._scale_factor = data.get("_scale_factor", 1.0)
self._attached_tools = data.get("_attached_tools", {})
self._current_ik = data.get("_current_ik", {"request_id": None, "solutions": None})
Copy link
Member

Choose a reason for hiding this comment

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

mmh is this to stay compatible with previously serialized Robots? But it didn't implement the Data interface before, so there shouldn't be any, right?

You could also simply omit these from the Data dictionary if they take default values, __init__ is ran first anyways. Otherwise you can assume they're there, because they're put there by the getter.

self.model = RobotModel.from_data(data["model"])
self.semantics = data.get("semantics", None)
self.attributes = data.get("attributes", {})

@property
def artist(self):
Expand All @@ -65,6 +94,8 @@ def artist(self):
@artist.setter
def artist(self, artist):
self._artist = artist
if artist is None:
return
if len(self.model.joints) > 0 and len(self.model.links) > 0:
self.scale(self._scale_factor)
for tool in self.attached_tools.values():
Expand Down
35 changes: 34 additions & 1 deletion src/compas_fab/robots/semantics.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
from __future__ import print_function

from compas.files import XML
from compas.data import Data

__all__ = [
"RobotSemantics",
]


class RobotSemantics(object):
class RobotSemantics(Data):
"""Represents semantic information of a robot.

The semantic model is based on the
Expand All @@ -32,6 +33,7 @@ def __init__(
disabled_collisions=None,
group_states=None,
):
super(RobotSemantics, self).__init__()
self.robot_model = robot_model

self.groups = groups or {}
Expand All @@ -41,6 +43,37 @@ def __init__(
self.disabled_collisions = disabled_collisions or set()
self.group_states = group_states or {}

@property
def data(self):
data = {
"robot_model": self.robot_model,
"groups": self.groups,
"main_group_name": self.main_group_name,
"passive_joints": self.passive_joints,
"end_effectors": self.end_effectors,
"disabled_collisions": sorted(self.disabled_collisions),
gonzalocasas marked this conversation as resolved.
Show resolved Hide resolved
"group_states": self.group_states,
}
return data

@data.setter
def data(self, data):
self.robot_model = data.get("robot_model", None)
self.groups = data.get("groups", {})
self.main_group_name = data.get("main_group_name", None)
self.passive_joints = data.get("passive_joints", [])
self.end_effectors = data.get("end_effectors", [])
self.disabled_collisions = data.get("disabled_collisions", set())
if len(self.disabled_collisions) > 0:
self.disabled_collisions = {tuple(pair) for pair in self.disabled_collisions}
self.group_states = data.get("group_states", {})

@classmethod
def from_data(cls, data):
robot_semantics = cls(None)
robot_semantics.data = data
return robot_semantics
Comment on lines +71 to +75
Copy link
Member

Choose a reason for hiding this comment

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

@gonzalocasas commented on this. I'd say setting robot_model=None in arguments of __init__ is legit. Then you don't need a custom from_data. While I was not a fan, this convention grew on me.

Having said that, the trend in COMPAS 2.0 is to get rid of the data.setter acknowledging it's often redundant. So you'd then just have a data getter and a from_data which feeds the data dict to __init__.

Example (just for illustration, not sure if this works in in LTS): gramaziokohler/compas_timber@d11d83e#diff-5cbdc4545536c895404b80de4fdcc522ec5b95dfb4af73a47e4120888cc22031R42-R48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I get your argument.

Are you saying that the style is moving towards having default values for all parameters in the __init__ and then skipping the def from_data()?

Probably it would be good to have a more wholistic stylistic clean up separately. The style currently used in compas_fab is a mixed bag of things. I can do that as long as we can have some consensus on what convention we adopt.

Copy link
Member

Choose a reason for hiding this comment

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

yea sorry didn't wanna cause confusion. Let's leave it like that.


@property
def group_names(self):
return list(self.groups.keys())
Expand Down
4 changes: 3 additions & 1 deletion src/compas_fab/robots/tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import json

from compas.data import Data
from compas.robots import Geometry
from compas.robots import ToolModel

Expand All @@ -14,7 +15,7 @@
__all__ = ["Tool"]


class Tool(object):
class Tool(Data):
"""Represents a tool to be attached to the robot's flange.

Attributes
Expand All @@ -39,6 +40,7 @@ class Tool(object):
"""

def __init__(self, visual, frame_in_tool0_frame, collision=None, name="attached_tool", link_name=None):
super(Tool, self).__init__()
self.tool_model = ToolModel(visual, frame_in_tool0_frame, collision, name, link_name)

@classmethod
Expand Down
48 changes: 48 additions & 0 deletions tests/robots/test_robot.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import re

import pytest
from compas.data import json_dumps
from compas.data import json_loads
from compas.datastructures import Mesh
from compas.geometry import Frame
from compas.robots import RobotModel
Expand Down Expand Up @@ -289,6 +291,52 @@ def test_get_configurable_joints_wo_semantics(panda_robot_instance_wo_semantics)
assert all(matches)


def test_semantics_serialization(panda_srdf, panda_urdf):
model = RobotModel.from_urdf_file(panda_urdf)
semantics = RobotSemantics.from_srdf_file(panda_srdf, model)
semantics_string = json_dumps(semantics)
semantics2 = json_loads(semantics_string)
assert isinstance(semantics, RobotSemantics)
assert isinstance(semantics2, RobotSemantics)
for disabled_collision in semantics.disabled_collisions:
assert disabled_collision in semantics2.disabled_collisions
gonzalocasas marked this conversation as resolved.
Show resolved Hide resolved


def test_robot_serialization(panda_robot_instance):
robot = panda_robot_instance
robot.scale(0.001)
robot_string = json_dumps(robot)
robot2 = json_loads(robot_string)
robot2_string = json_dumps(robot2)
assert robot2_string != ""
assert isinstance(robot, Robot)
assert isinstance(robot2, Robot)
assert robot.model.name == robot2.model.name
assert robot._scale_factor == robot2._scale_factor
for index, joint in enumerate(robot.model.joints):
assert joint.name == robot2.model.joints[index].name
assert str(joint.origin) == str(robot2.model.joints[index].origin)
for index, link in enumerate(robot.model.links):
assert link.name == robot2.model.links[index].name


def test_robot_serialization_with_tool(ur5_robot_instance, robot_tool1):
robot = ur5_robot_instance
robot.attach_tool(robot_tool1)
# serialization and deserialization using compas data serialization
robot_string = json_dumps(robot)
robot2 = json_loads(robot_string)

for tool in robot2.attached_tools.values():
assert isinstance(tool, Tool)
assert len(robot2.attached_tools) == 1
assert robot.main_group_name == robot2.main_group_name
tool1 = robot.attached_tool
tool2 = robot2.attached_tool
assert tool1.name == tool2.name
assert str(tool1.frame) == str(tool2.frame)


Copy link
Member

Choose a reason for hiding this comment

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

I'd add another unittest checking deepcopy. should behave the same as (de)serializing but sometimes doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried deepcopy but It doesn't seem to work. I think some years ago, I heard from someone to not use deepcopy on these objects. I don't remember why. But basically I never deepcopied any compas objects since.

Just to give you a taste of that error message:

./tests/robots/test_robot.py::test_robot_deepcopy_with_tool Failed: [undefined]AttributeError: 'Visual' object has no attribute '_guid'
tests\robots\test_robot.py:345: in test_robot_deepcopy_with_tool
    robot2 = deepcopy(robot)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy
    y = _reconstruct(x, memo, *rv)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct
    state = deepcopy(state, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy
    y = _reconstruct(x, memo, *rv)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct
    state = deepcopy(state, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy
    y = _reconstruct(x, memo, *rv)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct
    state = deepcopy(state, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:206: in _deepcopy_list
    append(deepcopy(a, memo))
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:172: in deepcopy
    y = _reconstruct(x, memo, *rv)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:271: in _reconstruct
    state = deepcopy(state, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:231: in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:146: in deepcopy
    y = copier(x, memo)
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:206: in _deepcopy_list
    append(deepcopy(a, memo))
..\..\..\anaconda3\envs\cf-dev\Lib\copy.py:161: in deepcopy
    rv = reductor(4)
..\..\..\anaconda3\envs\cf-dev\Lib\site-packages\compas\data\data.py:121: in __getstate__
    "guid": str(self.guid),
..\..\..\anaconda3\envs\cf-dev\Lib\site-packages\compas\data\data.py:200: in guid
    if not self._guid:
E   AttributeError: 'Visual' object has no attribute '_guid'

Copy link
Member

Choose a reason for hiding this comment

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

it's known deepcopy errors produce the most beautiful stacks..
To this specific one, could there be a class called Visual (really?) somewhere which inherits from Data but doesn't call super().__init__()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone wants to debug this, this is the test code:

def test_robot_deepcopy(panda_robot_instance):
    import copy
    robot = panda_robot_instance
    robot.scale(0.001)
    robot2 = copy.deepcopy(robot)
    assert robot is not robot2

def test_robot_deepcopy2(ur5_robot_instance, robot_tool1):
    import copy
    robot = ur5_robot_instance
    robot.attach_tool(robot_tool1)
    robot2 = copy.deepcopy(robot)
    assert robot is not robot2

Both test won't work and they have different error message

def test_inverse_kinematics_repeated_calls_will_return_next_result(ur5_with_fake_ik):
robot = ur5_with_fake_ik

Expand Down
Loading