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 LocalizationPose to Deck #987

Closed
wants to merge 1 commit into from

Conversation

oysand
Copy link
Contributor

@oysand oysand commented Aug 23, 2023

No description provided.

@github-actions
Copy link

🔔 Changes in database folder detected 🔔
Do these changes require adding new migrations? 🤔 In that case follow these steps.
If you are uncertain, ask a database admin on the team 😄

@oysand oysand force-pushed the add-localization-pose-to-deck branch from 6f36e02 to 24afc0a Compare August 23, 2023 12:05
@oysand oysand mentioned this pull request Aug 23, 2023
4 tasks
@oysand oysand self-assigned this Aug 23, 2023
@oysand oysand force-pushed the add-localization-pose-to-deck branch 4 times, most recently from 0d8a23e to c31cdad Compare August 24, 2023 07:24
@github-actions
Copy link

🔔 Migrations changes detected 🔔
📣 Remember to comment "/UpdateDatabase" after review approval for migrations to take effect!

@github-actions github-actions bot added the database-change Will require migration label Aug 24, 2023
Comment on lines 403 to +404

};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
};
};

@@ -144,7 +145,8 @@ private async Task<(string installationId, string plantId, string deckId, string
{
InstallationCode = installationCode,
PlantCode = plantCode,
Name = deckName
Name = deckName,
LocalizationPose = testPose
Copy link
Contributor

Choose a reason for hiding this comment

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

Area has DefaultLocalizationPose and deck has LocalizationPose. Consider renaming one of them for consistent naming

Comment on lines 387 to +388
};
var testLocalizationPose = new Pose
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
};
var testLocalizationPose = new Pose
};
var testLocalizationPose = new Pose

Comment on lines 387 to +388
};
var testLocalizationPose = new Pose
Copy link
Contributor

@Eddasol Eddasol Aug 31, 2023

Choose a reason for hiding this comment

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

Why not just use testPose instead of a separate testLocalizationPose? Alternatively, change area to also use testLocalizationPose and remove testPose

@oysand oysand force-pushed the add-localization-pose-to-deck branch from c31cdad to ae73818 Compare August 31, 2023 07:15
@oysand oysand marked this pull request as draft September 1, 2023 12:03
@oysand
Copy link
Contributor Author

oysand commented Sep 8, 2023

#1003 address this better

@oysand oysand closed this Sep 8, 2023
@oysand oysand deleted the add-localization-pose-to-deck branch September 8, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-change Will require migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants