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

Introducing lift delete button #511

Merged

Conversation

TanJunKiat
Copy link
Contributor

Continuation of #509

Implementation summary

  • Delete button in lift_table to remove lift from map and yam
  • Delete any lift cabin way point and any edges connected to said way point

Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Interesting, I was thinking something a lot simpler (just iterating over waypoints and removing the lift_cabin property if it matches the deleted lift name, leaving everything else intact) but I can see why it could be helpful to automatically remove the lift vertices and edges that lead into the lift.
I'm very slightly concerned about this break, this will mean that as soon as one waypoint with the matching lift_cabin property is found it will stop iterating, which "might" behave unexpectedly if there are more waypoints with that property, which will not be cleaned up properly. Now this is usually not the case but other parts of the code dealing with such property don't have the assumption that there will never be more than one (i.e. here https://github.com/TanJunKiat/rmf_traffic_editor/blob/7a55fbd1d695059f3ee5bc40a4601cbb2c27c9f3/rmf_traffic_editor/gui/lift_dialog.cpp#L441-L449, you can see that a flag is set to denote it as found but the loop doesn't early exit).

@luca-della-vedova
Copy link
Member

The test failures seem mostly related to uncrustify, you should be able to reproduce with a local colcon test run

@TanJunKiat
Copy link
Contributor Author

Right now the implementation is a nested if , to check for a lift_cabin property, then check if the lift name matches.

I am also worried on the situation where the lift cabin name is changed manually via the property panel which results in unsuccessful deletion of the vertex after deleting the lift.

@luca-della-vedova
Copy link
Member

Yap I know there are two nested if statements to make sure the lift matches, it's more a "what would happen if there was more than one lift with the requested lift_name?", the main other code path I linked would still work but this wouldn't because only the first waypoint would be cleaned up

@TanJunKiat
Copy link
Contributor Author

Alright, what we can do is do remove the break and remove all the waypoints that fits the criteria.

Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Thanks for iterating!

@luca-della-vedova
Copy link
Member

Ah sorry last hurdle, we need commits to be signed https://docs.github.com/authentication/managing-commit-signature-verification/about-commit-signature-verification, can you do that and force-push to this PR?

@TanJunKiat
Copy link
Contributor Author

Sorry, still trying to fumble around with the signature. Will get to it asap.

Signed-off-by: tanjunkiat <tanjunkiat@outlook.sg>
@luca-della-vedova
Copy link
Member

Thanks for iterating! (Sign offs can be painful the first time :) )

@luca-della-vedova luca-della-vedova merged commit 71e419c into open-rmf:main Sep 19, 2024
6 checks passed
@TanJunKiat TanJunKiat deleted the feature/lift_delete_button branch October 8, 2024 04:50
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