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.

Sign up for GitHub

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

Jump to bottom

Constructors with parameters in ROS2SensorComponents #433

Merged
merged 7 commits into from
Aug 8, 2023
Merged

Constructors with parameters in ROS2SensorComponents #433

merged 7 commits into from
Aug 8, 2023

Conversation

jhanca-robotecai
Copy link
Contributor

  • move configuration of ROS2GNSSSensorComponent, ROS2ImuSensorComponent, ROS2LidarSensorComponent, and ROS2Lidar2DSensorComponent to separate structs
  • add constructors for ROS2SensorComponents that take parameters

The goal of this PR is to simplify the implementation of methods for creating sensors (it will be used in RobotImporter).

@jhanca-robotecai jhanca-robotecai requested review from a team as code owners July 27, 2023 09:21
adamdbrw
adamdbrw reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@adamdbrw adamdbrw left a comment

Choose a reason for hiding this comment

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

Nice changes! How was this PR tested?

Gems/ROS2/Code/Source/Lidar/LidarSensorConfiguration.cpp Show resolved Hide resolved
@jhanca-robotecai jhanca-robotecai mentioned this pull request Jul 27, 2023
Closed
@jhanca-robotecai
Copy link
Contributor Author

Nice changes! How was this PR tested?

It was tested manually to ensure that all options are still available in Editor and with some unit tests (not pushed) to test the constructors. The unit tests were based on this code: RobotecAI#10 and can be found in closed #429 for ROS2CameraSensorComponent. I can refactor the test code and push it to this PR if necessary.

@adamdbrw
Copy link
Contributor

Test code would be great!

@jhanca-robotecai
Copy link
Contributor Author

Test code would be great!

Tests were added, but I am not sure if we want to keep them.

michalpelka
michalpelka reviewed Jul 28, 2023
View reviewed changes
Gems/ROS2/Code/Source/Imu/ImuSensorConfiguration.cpp

namespace ROS2
{
void ImuSensorConfiguration::Reflect(AZ::ReflectContext* context)
Copy link
Contributor

@michalpelka michalpelka Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I do not like that change. There are designs, projects, and Gems that use that component and this changes reflection.

michalpelka
michalpelka reviewed Jul 28, 2023
View reviewed changes
Gems/ROS2/Code/Source/Lidar/LidarSensorConfiguration.h
namespace ROS2
{
//! A structure capturing configuration of a lidar sensor (to be used with ROS2LidarSensorComponent).
class LidarSensorConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

It breaks all lidars in other gems and other projects.

michalpelka
michalpelka requested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@michalpelka michalpelka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Change from code point of view looks nice, but it will break all prefabs using touched components.

michalpelka
michalpelka reviewed Jul 28, 2023
View reviewed changes
Copy link
Contributor

@michalpelka michalpelka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also from UX point of view it looks unclean:
Pr:
image
Development:
image
Since all components in PR were released, we should introduce translation for versions.

@michalpelka michalpelka requested a review from adamdbrw July 28, 2023 12:12
@adamdbrw
Copy link
Contributor

@jhanca-robotecai the default view in Editor is simple to address can be fixed through an attribute (expand).
@michalpelka I believe following the pattern of configuration reflection is good. It is a shame for projects that have serialized previous versions. However, we also updated version of the Gem to 2.0 recently, since we had API breaking changes. We won't avoid doing that between versions. I believe we get interfaces into good place before the next release - and if that includes breaking changes, so be it. The next release will happen in October most likely.
@jhanca-robotecai could you make sure versions are updated in serialization of components?

@zakmat
Copy link
Contributor

zakmat commented Jul 28, 2023

Change management is an inherent part of the software engineering process. The sooner we are able to battle-test our approach to component migration the better. Version converters might be possibly used to change existing data in an unbreakable way, and developed idioms shall be kept as unit tests for further reference

@michalpelka michalpelka self-requested a review July 28, 2023 21:32
adamdbrw
adamdbrw reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@adamdbrw adamdbrw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Does anything else need to be changed with prefabs? I see changes for ROSBot but not Proteus.

@jhanca-robotecai
Copy link
Contributor Author

jhanca-robotecai commented Aug 4, 2023 •
edited
Loading

Based on the internal conversation it was decided to break the backward compatibility and fix the design issue. If decided otherwise, the sample implementation of the version converter (for one of the sensors) can be found here: jhanca-robotecai#1

The Editor's view was set to the same as before using the attributes:
image

Additionally, as the backward compatibility is broken, the parameter visualise was changed to visualize, to follow the coding guidelines (US English).

Finally, the prefab with Slamtec sensor was fixed (the configuration of 3D lidar sensor was requested for 2D lidar sensor), before:
image
after:
image

@jhanca-robotecai
Copy link
Contributor Author

The changes in:

@jhanca-robotecai jhanca-robotecai mentioned this pull request Aug 4, 2023
Closed
@adamdbrw
Copy link
Contributor

adamdbrw commented Aug 4, 2023

The demos need to be adjusted to development anyway, we should skip adjustments for this PR alone

@jhanca-robotecai
Copy link
Contributor Author

jhanca-robotecai commented Aug 4, 2023 •
edited
Loading

Looks good! Does anything else need to be changed with prefabs? I see changes for ROSBot but not Proteus.

There is no configuration stored in Proteus' prefab.

The demos need to be adjusted to development anyway, we should skip adjustments for this PR alone

It is quite easy to miss the change, as de-serialization simply ignores the unknown fields without notifying the user about problems. Therefore I would strongly recommend doing it as fast as possible. E.g. ROSConDemo docker configuration use development branch of o3de-extras repo, which means failure right after this PR is merged.

@adamdbrw
Copy link
Contributor

adamdbrw commented Aug 7, 2023

@michalpelka take a look please

@michalpelka michalpelka merged commit 76e6569 into o3de:development Aug 8, 2023
@jhanca-robotecai jhanca-robotecai mentioned this pull request Aug 9, 2023
Closed
@adamdbrw adamdbrw mentioned this pull request Sep 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamdbrw adamdbrw adamdbrw approved these changes

@michalpelka michalpelka michalpelka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jhanca-robotecai @adamdbrw @zakmat @michalpelka