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.