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

feat(spawning): functionality for spawning parking vehicles #354

Closed

Conversation

FunKuchen
Copy link
Contributor

@FunKuchen FunKuchen commented Oct 16, 2023

Description

This pull request adds necessary functionality that allows phabmacs to spawn vehicles anywhere on a map. For this new feature it became necessary that all in the mapping configured prototypes are also converted and saved as vehicle types.
Unit Tests that then failed because of this new implementation have been adjusted.

Issue(s) related to this PR

  • Resolves issue #mosaic-extended/697-configure-parking-vehicles

Affected parts of the online documentation

No changes in online documentation required.

Definition of Done

Prerequisites

  • You have read CONTRIBUTING.md carefully.
  • You have signed the Contributor License Agreement.
  • Your GitHub user id is linked with your Eclipse Account.

Required

  • The title of this merge request follows the scheme type(scope): description (in the style of Conventional Commits)
  • You have assigned a suitable label to this pull request (e.g., enhancement, or bugfix)
  • origin/main has been merged into your Fork.
  • Coding guidelines have been followed (see CONTRIBUTING.md).
  • All checks on GitHub pass.
  • All tests on Jenkins pass.

Requested (can be enforced by maintainers)

  • New functionality is covered by unit tests or integration tests. Code coverage must not decrease.
  • If a bug has been fixed, a new unit test has been written (beforehand) to prove misbehavior
  • There are no new SpotBugs warnings.

Special notes to reviewer

@@ -412,6 +413,8 @@ private void completeSpawnerDefinitions() {
VehicleTypesInitialization generateVehicleTypesInitialization() {
HashMap<String, VehicleType> types = new HashMap<>();

// TODO maybe find a better solution, this is just a first test to see if anything breaks
prototypeConfigurations.forEach(p -> types.put(p.name, new VehicleTypeSpawner(p).convertType()));
Copy link
Contributor

Choose a reason for hiding this comment

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

@FunKuchen Main problem now comes with predefined SUMO scenarios which cause the failing tests. When we use predefined SUMO scenarios which bring their own vehicle type definitions, we explicitly forbid that a vehicle spawner in mapping_config.json uses a vehicle type which is already defined in the loaded SUMO scenario. The main problem is, that we cannot know from SumoAmbassador perspective which vehicle types are configured in these predefined scenarios. Therefore even merging them somehow would be very difficult, because you'd need to parse all loaded route files and also additional files for vehicle type definitions. So we may need to find a better solution I guess. However, my goal was to see if anything breaks with this line, and luckily we found this already with our integration tests!

A quick (and not so clean) solution would be to add a new property in the MappingConfiguration class called propagateAllTypes which we set to false in the default case, and only do this line of code when this new property is set to true. As an alternative we could completely close this PR and see if we could fix the problem of missing prototypes in the PHABMACS federate somehow differently. As a third option we could go the full route of defining parking vehicles in the mapping configuration. However, this would require some additional discussions. Therefore we may go the proposed "quick'n'dirty' path and try to find a clean solution in follow up work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you somehow revert just this commit but keep everything else? The changed tests now fail again and have to be reverted to the old ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just close this PR and you'd switch to main as we don't need any changes, do we?

@kschrab
Copy link
Contributor

kschrab commented Oct 20, 2023

No required anymore

@kschrab kschrab closed this Oct 20, 2023
@FunKuchen FunKuchen deleted the 697-configure-parking-vehicles branch November 19, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants