NREL / floorspace.js

Other
66 stars 35 forks source link

Removing all points from a space renders it inoperable #398

Closed benfen closed 2 years ago

benfen commented 2 years ago

Problem

When all the points of a space have been removed, it can't be interacted with, since there are built-in assumptions that all spaces have enough points for a polygon. Once this happens to a space, it can only be deleted, as other operations will fail. It will also cause modifications of all other spaces to fail.

Reproduction

  1. Create a new floorplan
  2. Create two new spaces without overlap between them
  3. Select the eraser and delete one space entirely
  4. Attempt to recreate the space that was deleted or modify the other space; both operations should fail

This can also be done by encompassing one space within another and there may be other approaches, too.

Resolution

I believe there are two orthogonal ways to handle this:

  1. In the mutator for trimGeometry, remove any spaces that have no edges. This would also need to be done on import to protect against old plans with the issue
  2. Identify all the places in the code where these errors occur and handle them gracefully, so that a space can be recovered from this issue
benfen commented 2 years ago

@DavidGoldwasser Priority for fixing this?

DavidGoldwasser commented 2 years ago

Currently I can make a bunch of spaces, name them, set the color and then add geometry later. Maybe that isn't common, but the point is at least when spaces are made they exist without any edges. So if we take approach 1 need to make sure it doesn't impact making new spaces.

Also if we do remove the space that is probably fine, but maybe we could make a new one of the same name. I don't fully understand what makes a space that has never has edges different from one that had edges but has then had them removed, but I can see there is a difference in behavior.

Should we remove spaces that don't have 3 or more edges, or will that mess up the polygon tool function which would start with one edge?

DavidGoldwasser commented 2 years ago

@benfen this can be high priority. If you want we can start to use the labels for priority or what is in progress.

benfen commented 2 years ago

Forgot to drop a comment here - the implementation was simpler than expected. Whenever this situation would occur, the geometry passes through trimGeometry, meaning that the bad faces can be trimmed out there and completely remove the problem.