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

feat: make position in world to be the vessels #10

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

feat: make position in world to be the vessels #10

wants to merge 17 commits into from
+360 −187

Conversation

eduardacoppo
Copy link
Contributor

@eduardacoppo eduardacoppo commented Jan 15, 2025 •
edited
Loading

Depends on:

What is this PR for

See shortcut story for details
[sc-35882]

How I did it

Results, How I tested

Checklist

  • Assign yourself in the PR
  • Write unit tests (when relevant)
  • Run rubocop and rake tests locally
  • If this PR initialize a CMAKE package please enable styling and linting

@eduardacoppo eduardacoppo self-assigned this Jan 15, 2025
@eduardacoppo eduardacoppo changed the title test: add tests for position correction feat: make position in world to be the vessels Jan 16, 2025
jhonasiv
jhonasiv reviewed Jan 17, 2025
View reviewed changes
Copy link

@jhonasiv jhonasiv left a comment

Choose a reason for hiding this comment

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

This is mostly alright IMO. Just add a flag that turns this feature on and off so we can test it in live.

tasks/AISTask.cpp Outdated Show resolved Hide resolved
tasks/AISTask.cpp Outdated Show resolved Hide resolved
@eduardacoppo eduardacoppo requested a review from jhonasiv January 17, 2025 18:13
@jhonasiv jhonasiv requested a review from wvmcastro January 17, 2025 19:19
jhonasiv
jhonasiv reviewed Jan 17, 2025
View reviewed changes
nmea0183.orogen Outdated Show resolved Hide resolved
@eduardacoppo eduardacoppo requested a review from doudou January 17, 2025 19:53
seixasxbr
seixasxbr reviewed Jan 22, 2025
View reviewed changes
tasks/AISTask.cpp Outdated Show resolved Hide resolved
tasks/AISTask.cpp Outdated Show resolved Hide resolved
tasks/AISTask.cpp Outdated Show resolved Hide resolved
doudou
doudou reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@doudou doudou left a comment

Choose a reason for hiding this comment

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

Some fixes in addition to the naming issue Mateus spotted.

Biggest change is: move all that code to the library.

tasks/AISTask.cpp Outdated Show resolved Hide resolved
@eduardacoppo
chore: give explicit name to property
5495483
@eduardacoppo
chore: rename variable name in sensor-world transform
02b5afa
@eduardacoppo
test: move test setup to a function
c311e32 
Keeping the test setup in the `before do` block makes it harder to make adjustments for the tests (e. g., changing the value of the `use_sensor_offset_correction` property).
This commit moves the setup logic into a function to reduce duplication.
@eduardacoppo
chore: move position correction code to lib
d9f0073
@eduardacoppo
fix: fix tests to use ref pos relative to center
7339e19 
and avoid using null values in converter
@eduardacoppo eduardacoppo mentioned this pull request Jan 29, 2025
Open
5 tasks
jhonasiv
jhonasiv reviewed Jan 30, 2025
View reviewed changes
tasks/AISTask.cpp Outdated Show resolved Hide resolved
@eduardacoppo
fix: always update vessels map with info
fa2785b 
and remove function that was previously doing that, as the code is now one line
@eduardacoppo
chore: document
a8ca7a3
@eduardacoppo eduardacoppo requested a review from jhonasiv January 30, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seixasxbr seixasxbr Awaiting requested review from seixasxbr

@doudou doudou Awaiting requested review from doudou

@wvmcastro wvmcastro Awaiting requested review from wvmcastro

@jhonasiv jhonasiv Awaiting requested review from jhonasiv

Assignees

@eduardacoppo eduardacoppo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eduardacoppo @doudou @wvmcastro @jhonasiv @seixasxbr