iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
583 stars 211 forks source link

Formating quantities results in absolute value #3915

Closed ImVeryLost closed 1 year ago

ImVeryLost commented 2 years ago

Describe the bug I am trying to add a custom quantity type: Temperature. I'm providing a CustomQuantityTypeDefinitionas well as a UnitsProvider that creates the UnitConversion, but doing the actual conversions/formatting with ModelApp.quantityFormatter.formatQuantity.

I noticed that if the value in persistence units is positive, the result value will also be positive, even when it should not be.

E.g. Converting 1 Kelvin to Fahrenheit should result in a negative value: image But the actual value im getting is '457.87' instead.

image

To Reproduce I forked the "Add a toolbar button sample" and added a temperature unit. Now if you click the sample button, it will try converting a couple of different temperatures. The areas of interest will be CustomQuantityType(defines "temperature" in all unit systems, registers the quantity type), and CustomUnitsProvider (provides the actual conversions. Its based on the BasicUnitsProvider. Its a pity there is no better way to add additional UnitDefinition)

You will notice that in ToolbarButtonProvider:

  let formatter = await IModelApp.quantityFormatter.getFormatterSpecByQuantityType(KnownQuantityTypes.temperature.name);
  let negativeF = IModelApp.quantityFormatter.formatQuantity(1, formatter!); // 1K == -457.87F
  let positiveF = IModelApp.quantityFormatter.formatQuantity(509.744444, formatter!); //509.744444K == 457.87F

positiveF is converted correctly, but the negativeF results in the absolute value. I took a quick peek, and it looks like itwin.js takes the prefix of the original value, and attaches it on the result value. Which means that if the original value was negative, it would stay negative in all unit systems, and a positive value will stay positive. But as you can see, that's not how it should work for quantities with offset.

Expected behavior Conversions with offset should be correct. Let me know if there is a better approach to adding additional units to IModelApp.quantityFormatter.

Desktop (please complete the applicable information):

pmconne commented 2 years ago

@ColinKerr @NancyMcCallB see Formatter.formatQuantity where sign obtained from input is applied to output.

ColinKerr commented 2 years ago

Thanks for reporting this and providing a nice test case, @ImVeryLost. I have labeled this issue as a bug, I will provide a schedule update once we have done triage and we know what release a fix will be included in.

We provide another unit provider which can load unit definitions from an ECSchema: SchemaUnitProvider. We also provide a standard schema with many conversions, this schema can be downloaded as an npm package or loaded from an iModel that references it.

Here is the npm package and the source repo for the standard units schema.

The schema can be loaded from an iModel using the SchemaUnitProvider following this example: https://www.itwinjs.org/changehistory/3.1.0/#new-schemaunitprovider.

Note: You also need to register the ECSchemaRpcInterface on the client side which is not shown in this example.

ImVeryLost commented 2 years ago

@ColinKerr Thank you for letting me know about the SchemaUnitProvider. It is still missing some quantities we would like to use (e.g. measuring force in kips , and moment in Kip-meters, stress quantity is not in at all), but it is still useful to know. Just to verify - if I use IModelApp.quantityFormatter.setUnitsProvider, the supplied provider will be used instead of BasicUnitsProvider? Does the SchemaUnitProvider fully overlap the BasicUnitsProvider, and using regular itwin.js quantities (e.g. QuantityType.LengthEngineering) will still work as expected?

There is also the fact that we want to use these units in an extension/plugin/helper tool (that is published as a npm package and consumed by itwin viewer based applications, similar to itwin measure tools), rather than in a single application, which makes registering the RPCInterface a very involved task. I will see what we can do though.

Another question, and this might be just me overthinking - both providers define some units for the USSURVEY system.

  1. Are you aware that this system is to be deprecated by the end of the year? What are your plans for dropping the support?
  2. Whilst you have different length, area, volume definitions for US survey vs Us Customary systems, things like density, slope, and other quantities derived from length, only have the US Customary definitions. I realize that the US Survey system is only used in certain contexts, but wouldn't users expect everything to use the US survey foot if they choose that system? Admittedly, I always use the metric system myself, so again, I might be overthinking it.
ColinKerr commented 2 years ago

There should be 100% overlap between the units defined in the BasicUnitsProvider and the units defined in the standard Units schema. Other units could be defined in other schemas stored in an iModel or another schema provided outside of the Model. So the SchemaUnitsProvider will cover all units from the BasicUnitsProvider if it can find the standard Units schema. It can support any number of schemas that contain unit definitions.

We have no plans to deprecate US Survey unit definitions, we may still encounter data using these units after they are deprecated. Applications built on iTwin.js may choose to no longer show us survey as a choice of unit system.

WRT to units belonging to the US Survey unit system, we added the units to the standard schema requested by the developers working on different disciplines. The US Survey unit system is generally used in civil so I suspect density was not interesting to them. For slope, it is a ratio of lengths so there is no functional difference between units using US Survey and International Foot. So maybe it's a bit sloppy but we get by with only one definition.

The standard schema does have a kip unit but it has a different name, 'KPF'. There is a 'KPF_FT' moment unit but we do not have kip meter.

Stress is measured in units dimensionally equivalent to pressure, so while not technically correct I have seen some folks use our pressure units for stresses. It would be technically correct to define a new phenomenon with only those units you wish to use for stress. One limitation of the units system to note is that it does not differentiate between scalar and vector, so we get some clumsiness when working with phenomena where there is a vector component.

So you could use our standard units schema plus create your own schema with additional units defintions that reference the units defined in the standard schema. For example see the CifUnits schema used by some of the Bentley Civil Connectors: https://github.com/iTwin/bis-schemas/blob/master/Domains/4-Application/Connectors/CivilInfrastructureFramework/CifUnits.ecschema.xml.

ImVeryLost commented 2 years ago

The standard schema does have a kip unit but it has a different name, 'KPF'. There is a 'KPF_FT' moment unit but we do not have kip meter.

...My apologies, I 100% mixed that up with kip-feet.

Thank you for the detailed explanation! This definitely answered most of my questions. I will have to think a bit if we want to use the more technically correct approach and define separate conversions for "stress", but other than that it sounds like it is all covered in the existing units schema.

For US Survey vs Us custom units, I understand your approach of not bothering to define units when they arent technically needed. Lets hope it will be good enough until the whole US survey foot is finally dropped!

calebmshafer commented 1 year ago

closing this issue as resolved. please open a new one if you have any additional issues.