search

NREL / floorspace.js

Other
68 stars 36 forks source link

defaults have specific units. #297

Closed bgschiller closed 6 years ago

bgschiller commented 6 years ago

convert them to si units when that is the project config setting.

A caveat here is that we're destroying any stories that have already been created (because they are using the defaults for the wrong unit system).

Also, we now use si and ip in the schema Project.config.units field, rather than ft and m.

Addresses #293

shorowit commented 6 years ago

Beyond stories, note that the user can change space properties (e.g., heights), create windows, etc. all before changing units. So more than stories may need to be updated.

It makes me wonder if we simply move the IP vs SI choice to the quick start menu? Or, for the time being, we could remove the ability to change units and require that it be done via the API? @macumber

macumber commented 6 years ago

My thought is that we need to go by the schema here, if there is missing markup in the schema we may have to update the schema for certain fields. As long as @bgschiller detects the ip_units and si_units for a field he can make sure to do the conversion, both for defaults and for user set values, and update those on open if the init units dont match the file units

shorowit commented 6 years ago

Sorry, I probably wasn't clear. I'm not referring to a file open action, I'm referring to a new project The app allows you to change the units before you start drawing anything -- but you can still create objects or change object properties before changing the units.

image

bgschiller commented 6 years ago

I'm happy to talk this over on Wednesday, but it seems like we have several options:

  1. Delete any models (window definitions, etc) upon switching unit systems
  2. Disallow changing unit systems if any models have been created (aside from Story and Space, which get automatically created).
  3. Just ignore it. A window that is 3ft tall will now be interpreted to be 3m tall.
  4. Apply unit conversion factors to any models that have been created. I've tried this in the past, and @shorowit found several reasons why it didn't work great: https://github.com/NREL/floorspace.js/issues/169#issuecomment-351847899
macumber commented 6 years ago

I won't be able to make the call Wednesday. I don't like deleting existing models or ignoring the unit change. If we do the feature, we should make it work right. I think you could get away with rounding numbers for display only, you could leave them full precision in the data store

shorowit commented 6 years ago

I agree with Dan. Of the above options, I like option 2 best. Though I still think there are 2 other possibilities:

  1. Move the unit conversion dialog to the Quickstart dialog; it applies when creating a new project.
  2. Remove the unit conversion capability altogether. It can only be set as part of the API when creating a new project.