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

Introduce inspection area polygon #2028

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Introduce inspection area polygon #2028

wants to merge 1 commit into from
+1,696 −150

Conversation

Christdej
Copy link
Contributor

@Christdej Christdej commented Feb 17, 2025 •
edited
Loading

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like console.log, TODO, etc.
  • The PR has been tested locally
  • A test have been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that require new issues
  • The changes does not introduce dead code as unused imports, functions etc.

@Christdej Christdej added feature New feature or request backend Backend related functionality improvement Improvement to existing functionality database-change Will require migration labels Feb 17, 2025
@Christdej Christdej self-assigned this Feb 17, 2025
@github-actions GitHub Actions
Copy link

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

@Christdej Christdej marked this pull request as draft February 17, 2025 12:12
@Christdej Christdej force-pushed the polygon branch 4 times, most recently from b212863 to 2d6d405 Compare February 18, 2025 11:48
@Christdej
Copy link
Contributor Author

Remaining work could be to remove Plant of Installation, documented here:
#2032

@Christdej Christdej force-pushed the polygon branch 2 times, most recently from f5c9456 to 08aa643 Compare February 20, 2025 11:20
@Christdej Christdej marked this pull request as ready for review February 20, 2025 11:20
tsundvoll
tsundvoll reviewed Feb 20, 2025
View reviewed changes
backend/api.test/Services/InspectionAreaService.cs
Comment on lines +54 to +57
{ ""x"": 0, ""y"": 0 },
{ ""x"": 0, ""y"": 10 },
{ ""x"": 10, ""y"": 10 },
{ ""x"": 10, ""y"": 0 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we are just checking for a square polygon. Maybe sufficient for now, but an idea here could be to define a more complex polygon and use property-based testing to test it. Issue for another time probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, though that was good enough for the test without having to think to much

Eddasol
Eddasol reviewed Feb 21, 2025
View reviewed changes
backend/api/Controllers/MissionSchedulingController.cs
Comment on lines +370 to +372
return Conflict(
$"The tasks of the mission are not inside the inspection area of the robot"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is conflict correct error type or is bad request more descriptive? Conflict indicates that the request didn't success at the moment due to some conflict with the current state, but this request will never succeed with the given input.

Eddasol
Eddasol reviewed Feb 21, 2025
View reviewed changes
backend/api/Services/InspectionAreaService.cs

if (inspectionAreaPolygon == null)
{
return true; // Invalid polygon, so all tasks are inside
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this trigger a warning before returning?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eddasol Eddasol Eddasol left review comments

@tsundvoll tsundvoll tsundvoll left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Christdej Christdej

Labels
backend Backend related functionality database-change Will require migration feature New feature or request improvement Improvement to existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Christdej @Eddasol @tsundvoll