Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default imperial units length parser does not accept string from default length formatter. #7596

Open
bbastings opened this issue Jan 23, 2025 · 9 comments · May be fixed by #7671
Open

Default imperial units length parser does not accept string from default length formatter. #7596

bbastings opened this issue Jan 23, 2025 · 9 comments · May be fixed by #7671
Assignees
Labels

Comments

@bbastings
Copy link
Contributor

Using:

IModelApp.quantityFormatter.setActiveUnitSystem("imperial")).
IModelApp.quantityFormatter.findFormatterSpecByQuantityType(QuantityType.Length);
IModelApp.quantityFormatter.findParserSpecByQuantityType(QuantityType.Length);

Formatter can return a string like: 20'-6" and parser won't accept the "-". Must instead enter 20'6" or use a different separator like 20':6", these will be successfully formatted as 20'-6".

If possible fix should be backported to 4.11.x...

@hl662 hl662 self-assigned this Jan 23, 2025
@hl662
Copy link
Contributor

hl662 commented Jan 23, 2025

just want to ask for more info - is the quantityFormatter using the default units provider? You can tell if you did not call quantityFormatter.setUnitsProvider anywhere

@bbastings
Copy link
Contributor Author

I assume it's the default units provider (don't know where that is setup). Didn't call anything except:

IModelApp.quantityFormatter.findFormatterSpecByQuantityType(QuantityType.Length);
IModelApp.quantityFormatter.findParserSpecByQuantityType(QuantityType.Length);

Since "imperial" seems to be the default, I don't actually call IModelApp.quantityFormatter.setActiveUnitSystem("imperial")) either.

@hl662
Copy link
Contributor

hl662 commented Jan 30, 2025

I figured out what's the problem.

The default format props for imperial LENGTH has it's spacer set to -. We recently added logic that allowed mathematical operations, and there are hardcoded constants for addition and subtraction. This logic meant, while parsing strings and creating parsing tokens, if we find a - in the string, it's considered not a spacer, but a mathematical operator
After generating the tokens, there would be a conditional that returns an error because the parser found an operator when math operations aren't enabled by default in the default format props mentioned above.

We need to figure out what can we do when a format has a spacer that conflicts with an operator...

@mathieu-fournier @rschili @ColinKerr

@rschili
Copy link
Contributor

rschili commented Jan 31, 2025

I'd say one possible smart fix is: If any of the spacers/separators is set to one of our hard coded mathematical operators, we disable that mathematical operator for that format.
However, this may be too pragmatic. Ideally, the maths tokenizer would work on top of the units tokenizer, so we'd use the units tokenizer to identify and parse the units, and then the maths tokenizer to combine those values into a single one.
Having this logic all baked into one tokenizer which also supports composite units really complicates things :(

@bbastings
Copy link
Contributor Author

bbastings commented Jan 31, 2025

@hl662 @rschili @ColinKerr

I'd say one possible smart fix is: If any of the spacers/separators is set to one of our hard coded mathematical operators, we disable that mathematical operator for that format.

Don't like that idea, ideally I'd want to be able to do this:

Image

Currently, with the default parser (which you say doesn't enable math operations), it's doing something with the /2...not sure what though, might be adding the 2? It's definitely not returning half of the distance quantity before the operator. Seems like this should return an error if math operations aren't enabled then?

You also need "-" to specify negative values and "/" for fractions...so you'd never be able to use these?

After generating the tokens, there would be a conditional that returns an error because the parser found an operator when math operations aren't enabled by default in the default format props mentioned above.

I don't understand why it's looking for math operations when tokenizing if math operations aren't enabled for the parser?!?

I understand that this is a difficult problem, but if the parser can't be smart enough to "just work" should probably just expunge checking for math operations in the tokenizer. Ideally you shouldn't need to explicitly enable math operations, they should be enabled by default (who doesn't want functional expression support?). You'd only want to conditionally disable them if they are causing serious issues that prevent folks from being able to get their work done. So it would need to be a configuration option that lets users affect the parser behavior until a fix is available.

NOTE: I added very limited support for "quantity +- quantity" and "quantity */ number" for the AccuDraw input fields because I saw that the default parsers were doing weird things otherwise. AccuDraw needs to work with the default length and angle parser...

EDIT: Just looked at the code and saw that Operator is only "+" and "-". You should definitely just expunge looking for operators IMO. I consider supporting "*" and "/" a minimal requirement, without them it's not worth bothering with "+" and "-" especially since the current implementation is problematic...

@hl662
Copy link
Contributor

hl662 commented Feb 5, 2025

@bbastings can you share more on how you added support for math operations for AccuDraw's input fields, prior to it being passed into the parser? Support for mathematical operation in core quantity was added as more experimental to support an internal product feature. To remove looking for operators would mean to remove this simple math operation calculations from the API, but we would want to find a replacement/extension for math operations in that case.

@mathieu-fournier could chime in more on this idea

@bbastings
Copy link
Contributor Author

bbastings commented Feb 5, 2025

@hl662 What I did for AccuDraw isn't really relevant because I have additional context, like the fact I know the input field currently contains a valid quantity string.

The current implementation and it's limitations are describe here, near the top you can find a topic called "Expression Support" that will jump you to the right section. AccuDraw Expression Support

@hl662 hl662 linked a pull request Feb 7, 2025 that will close this issue
@hl662
Copy link
Contributor

hl662 commented Feb 7, 2025

@bbastings I drafted a PR that will address the immediate bug, but it's not perfect - we'll have to define limitations for what the parser can and can't do for now, it'll take a lot more work to refactor quantity's parsing logic to fit in with everything (or strip things out).

I wanted to also make sure that a formatted value that the quantity package returns, when passed back in, will result in the same value, as that's something that could have caught the problem you raised before math support was introduced.

Please look at the limitations I wrote in the PR description, and if you have more quantity values you'd like explicitly tested you can leave a comment of the value and the expected output

@bbastings
Copy link
Contributor Author

I wanted to also make sure that a formatted value that the quantity package returns, when passed back in, will result in the same value

@hl662 I definitely approve of having these tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants