Initial, internal review of RFC to be submitted to o3de/sig-simulation #1
Conversation
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
michalpelka
changed the title
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
rfcs/RFC Simulation Interfaces.md
Outdated
| The implementation will be split into three (or more system components): | ||
| - ROS 2 Entities manager \ | ||
| It will be responsible for the lifetime of spawned objects, it will cache initial positions. | ||
| - ROS 2 Named poses manager \ | ||
| It will be responsible for aggregating information in the Named Pose Game Component. | ||
| - ROS 2 Simulator manager \ | ||
| It will be responsible for modifying the global state of the simulation (e.g., pausing, reloading). | ||
|
|
||
| We will decouple the implementation of those managers from the ROS 2 service. | ||
| Every manager will need to expose public methods that will be callable both from C++ and ROS 2. | ||
| The purpose of that approach is to enable testability without the need for a ROS domain. |
There was a problem hiding this comment.
Should all system components land in the ROS 2 Gem? As a downside of the RFC you mentioned the increase of the ROS 2 codebase. Shouldn't we start thinking about splitting the Gem into smaller pieces?
- ROS 2 - gem for the node and base sensor component to inherit from
- RobotImporter - gem for importing URDF/SDF and possibly more
- ROS 2 Sensors - gem with basic implementation of all sensors
etc.
This implementation might be the first one to think about it as you mention decoupling c++ and ROS 2 service interfaces.
There was a problem hiding this comment.
Regarding this topic, we have also considered moving feature implementations (Entity manager, Named pose manager, ...) into separate Gem, and placing their ROS 2 interfaces in ROS 2 Gem. If I remember correctly, this idea was not rejected explicitly, so we may also discuss it here.
There was a problem hiding this comment.
Keeping open for discussion. I think it should be a new gem
rfcs/RFC Simulation Interfaces.md
Outdated
| - A component called `SimulationInfoComponent` will be created and attached to the root entity. | ||
| That component will be used to cache information in `simulationinfo` to be easily accessible in the future. | ||
| - [TagComponent](https://www.docs.o3de.org/docs/user-guide/components/reference/gameplay/tag/) from LmbrCentral gem at root entity will be created (if not present in prefab). | ||
| - TagComponent will be updated with a list of new tags. |
There was a problem hiding this comment.
I do not fully get what is the source of new tags.
Co-authored-by: Paweł Liberadzki <pawel.liberadzki@gmail.com> Co-authored-by: Jan Hanca <jan.hanca@robotec.ai> Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
rfcs/RFC Simulation Interfaces.md
Outdated
| The implementation will be split into three (or more system components): | ||
| - ROS 2 Entities manager \ | ||
| It will be responsible for the lifetime of spawned objects, it will cache initial positions. | ||
| - ROS 2 Named poses manager \ | ||
| It will be responsible for aggregating information in the Named Pose Game Component. | ||
| - ROS 2 Simulator manager \ | ||
| It will be responsible for modifying the global state of the simulation (e.g., pausing, reloading). | ||
|
|
||
| We will decouple the implementation of those managers from the ROS 2 service. | ||
| Every manager will need to expose public methods that will be callable both from C++ and ROS 2. | ||
| The purpose of that approach is to enable testability without the need for a ROS domain. |
There was a problem hiding this comment.
Regarding this topic, we have also considered moving feature implementations (Entity manager, Named pose manager, ...) into separate Gem, and placing their ROS 2 interfaces in ROS 2 Gem. If I remember correctly, this idea was not rejected explicitly, so we may also discuss it here.
rfcs/RFC Simulation Interfaces.md
Outdated
| Let us investigate moving a `Simulated PhysX Rigid Body'. | ||
| We cannot simply call : | ||
| ```cpp | ||
| AZ::TransformBus::Event(entityId, &AZ::TransformBus::Events::SetWorldTM, transform); |
There was a problem hiding this comment.
There is no need to support robots that are incorrectly made
I would follow this idea everywhere in this implementation. We rather have to make some assumptions, or put some requirements on what is supported with this implementation. Fortunately, in most places we do like that.
Co-authored-by: Paweł Liberadzki <pawel.liberadzki@gmail.com> Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai> Co-authored-by: Michał Pełka <michal.pelka@robotec.ai> Co-authored-by: Paweł Liberadzki <pawel.liberadzki@robotec.ai> Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
|
Comments that should be addressed - noted in #2
|
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
michalpelka
force-pushed
the
rb/sim_interfaces
branch
from
357ff6d to
b8cb61c
Compare
|
Closed in favour of o3de#95 |
No description provided.