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

[PI Sprint 24/25 / PD-456] - [Feature] Implement Localization #34

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

mbsaloka
Copy link

@mbsaloka mbsaloka commented Jan 7, 2025

Jira Link:

https://ichiro-its.atlassian.net/browse/PD-456?atlOrigin=eyJpIjoiYTNmODg5MjBmNGY0NDQ5NWIzOTQwNTVmMGM3YTkwZDAiLCJwIjoiaiJ9

Description

Implementing MCL algorithm in robot class

Type of Change

  • Bugfix
  • Enhancement
  • New feature
  • Breaking change (fix or feature that would cause the existing functionality to not work as expected)

How Has This Been Tested?

  • New unit tests added.
  • Manual tested.

Checklist:

  • Using Branch Name Convention
    • feature/JIRA-ID-SHORT-DESCRIPTION if has a JIRA ticket
    • enhancement/SHORT-DESCRIPTION if has/has no JIRA ticket and contain enhancement
    • hotfix/SHORT-DESCRIPTION if the change doesn't need to be tested (urgent)
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have made the documentation for the corresponding changes.

Comment on lines +1 to +2
#ifndef SUIRYOKU__LOCOMOTION__MODEL__FIELD_HPP_
#define SUIRYOKU__LOCOMOTION__MODEL__FIELD_HPP_
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's the copyright? check other files as well.


} // namespace suiryoku

#endif // SUIRYOKU__LOCOMOTION__MODEL__FIELD_HPP_
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing EOL. Check other file as well.

Comment on lines 53 to 59
bool Robot::get_apply_localization() {
return apply_localization;
}

void Robot::set_apply_localization(bool apply_localization) {
this->apply_localization = apply_localization;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no spesific requirement for the set get, you can expose the apply_localization to public instead.

p.orientation = orientation;

// p.position.x += delta_position.x;
// p.position.y += delta_position.y;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this unused?

Comment on lines 168 to 170
for (auto & p : particles) {
sum_weight += p.weight;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional, but it is better to use constant reference to avoid modification.

@mbsaloka mbsaloka changed the title [WIP] [Feature] Implement Localization [WIP] [PI Sprint 24/25] [Feature] Implement Localization Jan 11, 2025
@mbsaloka mbsaloka changed the title [WIP] [PI Sprint 24/25] [Feature] Implement Localization [WIP] [PI Sprint 24/25 / PD-456] [Feature] Implement Localization Jan 11, 2025
@mbsaloka mbsaloka changed the title [WIP] [PI Sprint 24/25 / PD-456] [Feature] Implement Localization [WIP] [PI Sprint 24/25 / PD-456] - [Feature] Implement Localization Jan 11, 2025
@mbsaloka mbsaloka changed the title [WIP] [PI Sprint 24/25 / PD-456] - [Feature] Implement Localization [PI Sprint 24/25 / PD-456] - [Feature] Implement Localization Jan 18, 2025
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