Allowing deletion of lift cabin vertices

[TanJunKiat]

Allow deletion of lift cabin vertices

Background and motivation

At the current state, when lift cabin vertices are generated, users are unable to directly delete them from the traffic editor. The way to go around currently is to manually go into the building.yaml file to delete them, but that might lead to undesirable results like failed deletion of a dangling edge or indexing issues.

This PR introduces the ability to delete lift cabin vertices. Users will be able to directly delete the lift cabin vertices from the traffic editor itself and have the application automatically re-index all the vertices as well as take care of any potential dangling edges.

Feature implementation

The implementation is straightforward.

When a deletion command is issued, the algorithm will check if there are any lanes connected to the selected vertex at the viewport level. As usual, an error message will surface if the vertex is used in any edge or polygon.

If the vertex is free, the algorithm will then check if it is a lift cabin vertex, and if true, will proceed to purge all the vertices that share the same lift cabin parameter with the same value_string, in this case, will be the lift's name.

Additonal task

• To update deletion dialog to remove the warning of vertex potentially being a lift cabin vertex.

[luca-della-vedova]

Thanks for the PR! I tried out the behavior and I have a few questions / feedback.

The main one is about the behavior, the filtering to make sure the vertex is not used in any edge currently only happens on the level that is being rendered, other levels will not be checked and all edges will be cleaned up automatically.
I find this behavior a bit confusing, on one end we refuse to delete the waypoint if it is connected to an edge, and show a warning to the user, on the other as long as the user goes on a level that has no edge we have no problem we recursively deleting all the other edges on other levels.
Is this intentional? My instinct would have been to extend the check for can_delete_current_selection to make sure that, if the vertex is a lift vertex, all other lift vertices have no edges connected to them, or in alternative only delete the vertex on the current level (but I'm guessing that will come with its own challenges?)

Then a small nit that I didn't add in line because it happens in an unrelated part of the code, we should change the text in the dialog that spawns when trying to delete a vertex that cannot be deleted to remove mentions of lifts if we will be allowing deleting lift vertices.

[TanJunKiat]

Is this intentional? My instinct would have been to extend the check for can_delete_current_selection to make sure that, if the vertex is a lift vertex, all other lift vertices have no edges connected to them, or in alternative only delete the vertex on the current level (but I'm guessing that will come with its own challenges?)

I would say it depends on the intention of the user.

If the user truly intends to perform a deletion of a vertex, informing that the vertex is used and forcing users to delete all the connecting entities before allowing them to delete, the whole process can be a very tedious and roundabout way.

But at the same time, I can vaguely understand the original intent, to warn the users that the vertex is still being used and deleting it might cause irreversible damage.

The current implementation makes it convenient for users if they are working on a large amount of levels, so much that it will take a lot of time to delete all the connecting lanes. We can of course do what as suggested, meaning to check if other levels are also clear of connections before deleting.

Another option we can go about is to open a dialog when the user wants to delete a vertex that is currently in use (not limiting it to lift cabins), and give them an option to either interrupt the delete action, or to proceed and delete all connecting entities

[luca-della-vedova]

I can see where you come from but I would argue that at least the current behavior is consistent, specifically this:

If the user truly intends to perform a deletion of a vertex, informing that the vertex is used and forcing users to delete all the connecting entities before allowing them to delete, the whole process can be a very tedious and roundabout way.

With this PR in some cases we would still delete vertices that are used while in some cases we wouldn't.
That being said, I agree that it can be a bit tedious so I wonder how tricky it would be to implement your last option:

Another option we can go about is to open a dialog when the user wants to delete a vertex that is currently in use (not limiting it to lift cabins), and give them an option to either interrupt the delete action, or to proceed and delete all connecting entities

So at least we could have a consistent behavior. The dialog could even include a list of entities that currently use the vertex so the user knows exactly what is going to happen, but admittedly I'm not sure how big of a change that would be

[TanJunKiat]

I can spend some time to look into it.

The last option seems more sensible, in both ease of use and forewarning the consequences of deleting used entities.

I can spend some time to see how it can be implemented while keeping in mind to see if we can use this for normal vertices as well.

[TanJunKiat]

Hi @luca-della-vedova

I've tweaked the implementation and reverted some of the functions.

But the concept is as what we discussed.

Now when the user is deleting a vertex that is used (edges, polygons, or lift cabins), a warning dialog will appear to confirm the action.

When the user confirms the action, if it is a lift cabin, the purging will be done as per the previous commit.

If it is an edge or polygon, it will trigger a purging function to remove the selected entity and the vertex.

[luca-della-vedova]

Looks a lot better! And I feel it will definitely be a helpful feature. Sometimes deleting a short edge can be quite painful if not outright impossible if all you can select is the vertices at its ends.

[luca-della-vedova]

Few small nits (and a few notes to self on things we should address in the future but are out of the scope of this PR) then looks good to me!

[luca-della-vedova]

Note to self but let's leave it to a followup, it is actually possible to do Ctrl-Click to select multiple vertices but the codebase only finds the first occurrence, we should consider whether to disallow selecting multiple vertices or make sure deletion can operate on an array of selected elements.

[TanJunKiat]

I think the selection of multiple vertices is required to collinear the selected vertices, so disabling it might be tough.

[luca-della-vedova]

Note to self, this is broken and we should either fix it or disable it

[luca-della-vedova]

Last (hopefully) round, more into the details of the implementation / code and a corner case that causes a segfault in the current implementation

[TanJunKiat]

Another side note, is it okay to make the lift cabin property uneditable?

If the value is to be edited manually and incorrectly, the behaviour might be undesirable.

[luca-della-vedova]

Another side note, is it okay to make the lift cabin property uneditable?

If the value is to be edited manually and incorrectly, the behaviour might be undesirable.

No strong feelings about this, there seem to be a lot of other improvements that could be made there, we can leave them for a followup (for example I just noticed that renaming a lift will not rename its cabin waypoints which might also cause unintended side effects, the lift of lifts is not updated, etc)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@53928fa). Learn more about missing BASE report.

Additional details and impacted files

@@          Coverage Diff          @@
##             main   #518   +/-   ##
=====================================
  Coverage        ?      0           
=====================================
  Files           ?      0           
  Lines           ?      0           
  Branches        ?      0           
=====================================
  Hits            ?      0           
  Misses          ?      0           
  Partials        ?      0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TanJunKiat]

Another side note, is it okay to make the lift cabin property uneditable?
If the value is to be edited manually and incorrectly, the behaviour might be undesirable.

No strong feelings about this, there seem to be a lot of other improvements that could be made there, we can leave them for a followup (for example I just noticed that renaming a lift will not rename its cabin waypoints which might also cause unintended side effects, the lift of lifts is not updated, etc)

Yea, I will try to fix these minor bugs when I have the time.

Thanks for the review on this PR.