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(interaction): Added LidarUpdates interaction and LidarData object #391

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

FunKuchen
Copy link
Contributor

@FunKuchen FunKuchen commented May 27, 2024

Description

What is this PR about?

  • We want to seperate the updating of LidarFrames from the VehicleUpdates interaction.
  • Therefore we created a new type of interaction for the purpose of sending LidarFrames to the ApplicationAmbassador.
  • The necessary interaction and a data structure for storage are implemented in this PR.

Related to

  • Issue number #809 in mosaic-extended

Affected parts of the online documentation

I believe no change in online documentation are 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


import javax.annotation.Nonnull;

public interface LidarApplication extends Application {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class needs to be documented before being merged.

Copy link
Contributor

@schwepmo schwepmo left a comment

Choose a reason for hiding this comment

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

I think this PR goes in the right direction but needs some further fine-tuning. I left a sloppy initial review with some concerns.

import java.util.List;
import java.util.stream.Collectors;

public class LidarUpdates extends Interaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class needs some more documentation before being merged




public LidarUpdates(long time, List<LidarData> updated) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For me it isn't clear what exactly the list of updated LidarData is made-up of.

  • Is it lidar data for all ego-vehicles at time?
  • Is it lidar data for one ego-vehicle at time?

This kind of stuff should be documented.

As far as I get, each ego-vehicle has a LidarData-Object for each lidar-collection-time, which contains the measured point clouds, right?

private final Object lidarData;

private final long time;
private final String name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Document the fields and maybe rename name to vehicleId or something that better describes it.


public class LidarData implements Serializable {
@JsonAdapter(PolymorphismTypeAdapterFactory.class)
private final Object lidarData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an Object and why is it called lidarData inside of the LidarData-class? This adds unnecessary confusion.

Do we not have a class representing the point clouds instead of using a generic object?

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