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

Add level fiducials to reference drawing features #164

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

luca-della-vedova
Copy link
Member

Bug fix

Fixed bug

This PR fixes map alignment when importing legacy projects that include a list of features / constraints but no fiducials, a typical case is a building with only one level (hence no fiducials) but one or more robot maps (hence the sets of constraints and features).

Fix applied

Previously, no level anchors were spawned so the optimization ran during map alignment would also transform the reference drawing and find an average of all the drawings. With this PR, level anchors are spawned and added to the fiducials in the position where the reference drawing is. The result is that the reference drawing will be "sticky" and stay in its original position with only the imported robot layers being translated.

I had to increase the scope of the reference drawing site id which required wrapping it in an Option to handle the "no reference drawing" case. This added a few (safe) boilerplate unwraps.

Before this PR:

Screencast.from.2023-08-17.10-54-01.webm

After this PR:

Screencast.from.2023-08-17.12-41-16.webm

Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I just made two tweaks:

  • Keep primary_drawing_id as a u32 instead of Option<32> so we don't need to unwrap so much. Unwrapping should generally be avoided except in rare cases where we are absolutely sure from the immediate context of the code that it will not panic (or that panicking is the only acceptable response to the unwrap failing).
  • Store both primary_drawing_id and drawing_tf inside of primary_drawing_info so we don't need to recalculate the drawing_tf later.

@mxgrey mxgrey merged commit 0ef3d83 into main Aug 17, 2023
5 checks passed
@mxgrey mxgrey deleted the fix/feature_level_fiducials branch August 17, 2023 06:15
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