Skip to content

RJD-1057 (4/5): Remove non-API member functions: EntityManager’s member functions forwarded to EntityBase (2/2) #1334

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

Merged

Conversation

TauTheLepton
Copy link
Contributor

@TauTheLepton TauTheLepton commented Jul 31, 2024

Description

Important

Right before merging this PR, please change the target branch of #1337 to master.

Otherwise that PR (which is next in line to be merged) will be closed automatically.

Abstract

This pull request mainly consists of refactoring and introduces changes in the API and EntityManager.
This pull request was divided into some parts (#1552, #1543, #1540, #1539) to make it easier to review.

Background

This pull request is an extension of the #1473.
Due to the fact that these pull requests combined would have been too large for a review, they have been split into two.

As a result of the previous PR, changes have been made to the API and EntityManager classes.
These changes created a great opportunity to declutter / tidy up the code of these classes.
This pull request consists of the code cleanup and refactoring. Many of the changes introduced are just changes to the order of functions inside the implementation file to keep them grouped by functionality and consistent with the header file.

Tip

When reviewing the changes, it may be easier to see the actual modifications by opening both versions of each file side by side rather than looking at the diff generated by GitHub.
This is due to the fact that GitHub shows lots of changes where mostly copying and pasting the code has been done (for reordering purposes).

Cpp mock scenarios

The change mentioned above has been applied in the whole codebase, which includes most of the cpp mock scenarios.

Cpp mock scenarios have been all changed like the following:

Before:

api_.spawn(
"npc", traffic_simulator::helper::constructCanonicalizedLaneletPose(34741, 10.0, 0.0),
getVehicleParameters());
auto & npc_entity = api_.getEntity("npc");
npc_entity.setLinearVelocity(10);
npc_entity.requestSpeedChange(10, true);

After:

auto & npc_entity = api_.spawn(
"npc", traffic_simulator::helper::constructCanonicalizedLaneletPose(34741, 10.0, 0.0),
getVehicleParameters());
npc_entity.setLinearVelocity(10);
npc_entity.requestSpeedChange(10, true);

Details

Below are all changes made in this PR

API

  • Renamed member configuration to configuration_
  • Most of the constructor logic has been moved to API::init() member function
  • Added member function setSiulationStepTime
    auto API::setSimulationStepTime(const double step_time) -> bool
    {
    /**
    * @note Pausing the simulation by setting the realtime_factor_ value to 0 is not supported and causes the simulation crash.
    * For that reason, before performing the action, it needs to be ensured that the incoming request data is a positive number.
    */
    if (step_time >= 0.001) {
    clock_.realtime_factor = step_time;
    simulation_api_schema::UpdateStepTimeRequest request;
    request.set_simulation_step_time(clock_.getStepTime());
    return zeromq_client_.call(request).result().success();
    } else {
    return false;
    }
    }
  • Removed member function getZMQSocketPort in favor of using getROS2Parameter
  • 3 spawn functions have been combined into 1 spawn function (additional template parameter ParamsType was introduced for this reason)
    • Merged by #1543
  • The remaining member functions that were defined using the FORWARD_TO_ENTITY_MANAGER macro have been converted to standard member functions

Important

All other changes are minor refactoring or just reordering functions in the file, which GitHub shows as big changes.

EntityManager

Important

All other changes are minor refactoring or just reordering functions in the file, which GitHub shows as big changes.

EntityBase

MiscObjectEntity

  • Change getGoalPoses return type
    • auto getGoalPoses() -> std::vector<CanonicalizedLaneletPose>

      auto getGoalPoses() -> std::vector<geometry_msgs::msg::Pose>

PedestrianEntity

  • Change getGoalPoses return type
    • auto getGoalPoses() -> std::vector<CanonicalizedLaneletPose>

      auto getGoalPoses() -> std::vector<geometry_msgs::msg::Pose>
  • Added member function auto getParameters() const -> const traffic_simulator_msgs::msg::PedestrianParameters &;

VehicleEntity

  • Change getGoalPoses return type
    • auto getGoalPoses() -> std::vector<CanonicalizedLaneletPose>

      auto getGoalPoses() -> std::vector<geometry_msgs::msg::Pose>
  • Added member function auto getParameters() const -> const traffic_simulator_msgs::msg::VehicleParameters &;

Ostream helpers

Merged by #1539.

All existing operators std::ostream & operator<< have been moved from file helper.[ch]pp to a dedicated file ostream_helper.[ch]pp.
In addition many more operators have been added for the types

References

INTERNAL LINK 1
INTERNAL LINK 2

Destructive Changes

--

Known Limitations

--

dmoszynski and others added 13 commits July 2, 2024 13:09
…o RJD-1057-remove-functions-forwarded-to-entity-base

Signed-off-by: Mateusz Palczuk <[email protected]>
…ed-to-entity-base' into RJD-1057-remove-functions-forwarded-to-entity-base

Signed-off-by: Mateusz Palczuk <[email protected]>
…o RJD-1057-remove-functions-forwarded-to-entity-base

Signed-off-by: Mateusz Palczuk <[email protected]>
Signed-off-by: Mateusz Palczuk <[email protected]>
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-with-middle

Signed-off-by: Mateusz Palczuk <[email protected]>
@dmoszynski dmoszynski self-assigned this Jul 31, 2024
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-with-middle

Signed-off-by: Mateusz Palczuk <[email protected]>
@TauTheLepton TauTheLepton force-pushed the RJD-1057-remove-functions-forwarded-to-entity-base-refactor branch from c9057f9 to ff759fe Compare July 31, 2024 14:16
@yamacir-kit yamacir-kit deleted the branch master September 18, 2024 01:42
@yamacir-kit yamacir-kit deleted the RJD-1057-remove-functions-forwarded-to-entity-base-refactor branch September 18, 2024 01:43
@TauTheLepton TauTheLepton restored the RJD-1057-remove-functions-forwarded-to-entity-base-refactor branch October 2, 2024 07:32
@TauTheLepton TauTheLepton reopened this Oct 2, 2024
TauTheLepton and others added 3 commits October 3, 2024 16:43
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-refactor

Signed-off-by: Mateusz Palczuk <[email protected]>
…ded-to-entity-base-middle' into RJD-1057-remove-functions-forwarded-to-entity-base-refactor
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-refactor
@TauTheLepton TauTheLepton self-assigned this Oct 15, 2024
@dmoszynski dmoszynski changed the title [tmp] Remove non-API member functions: EntityManager’s member functions forwarded to EntityBase (2/2) [tmp] RJD-1057 (4/5): Remove non-API member functions: EntityManager’s member functions forwarded to EntityBase (2/2) Oct 15, 2024
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-refactor
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-refactor
…le' into RJD-1057-remove-functions-forwarded-to-entity-base-refactor
@f0reachARR f0reachARR requested review from hakuturu583 and removed request for yamacir-kit and HansRobo April 3, 2025 03:58
Copy link

github-actions bot commented Apr 9, 2025

Failure optional scenarios

Note

This is an experimental check and does not block merging the pull-request.
But, please be aware that this may indicate a regression.

scenario failed: execution_time_test
      <failure type="SimulationFailure" message="CustomCommandAction typed &quot;exitFailure&quot; was triggered by the named Conditions {&quot;update time checker&quot;, &quot;avoid startup&quot;}: {&quot;update time checker&quot;: Is the /simulation/interpreter/execution_time/update (= 0.005911999999999999720612375853) is greaterThan 0.005?}, {&quot;avoid startup&quot;: Is the simulation time (= 7.549999999999981170617502357345) is greaterThan 1.000000000000000000000000000000?}" />

Copy link

Failure optional scenarios

Note

This is an experimental check and does not block merging the pull-request.
But, please be aware that this may indicate a regression.

scenario failed: execution_time_test
      <failure type="SimulationFailure" message="CustomCommandAction typed &quot;exitFailure&quot; was triggered by the named Conditions {&quot;update time checker&quot;, &quot;avoid startup&quot;}: {&quot;update time checker&quot;: Is the /simulation/interpreter/execution_time/update (= 0.005399000000000000153155266247) is greaterThan 0.005?}, {&quot;avoid startup&quot;: Is the simulation time (= 6.549999999999984723331181157846) is greaterThan 1.000000000000000000000000000000?}" />

@TauTheLepton
Copy link
Contributor Author

@hakuturu583 (or anyone merging this PR)

When you decide to merge this PR, right before merging, please change the target branch of #1337 to master.
This will prevent #1337 from automatically closing.

Copy link

@hakuturu583 hakuturu583 merged commit 5122f5b into master Apr 17, 2025
13 checks passed
@github-actions github-actions bot deleted the RJD-1057-remove-functions-forwarded-to-entity-base-refactor branch April 17, 2025 02:45
@HansRobo HansRobo restored the RJD-1057-remove-functions-forwarded-to-entity-base-refactor branch April 17, 2025 04:23
@HansRobo HansRobo deleted the RJD-1057-remove-functions-forwarded-to-entity-base-refactor branch April 17, 2025 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump major If this pull request merged, bump major version of the scenario_simulator_v2 refactor wait for regression test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants