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

Further expression parsing optimization and improvements. #60

Closed
wants to merge 12 commits into from

Conversation

AbdielKavash
Copy link
Member

@AbdielKavash AbdielKavash commented Feb 7, 2024

This PR is a major cleanup and rework of expression parsing in class MathExpression.

The previous multi-pass algorithm has been completely replaced by a single-pass stack-based algorithm, which is a variant of "Shunting Yard" (https://en.wikipedia.org/wiki/Shunting_yard_algorithm) algorithm. Compared to the description on wikipedia, my algorithm uses one stack for both numbers and operators, and immediately evaluates the expression as it is parsed, instead of converting to RPN notation.

Additionally, the function will now exit early and not even invoke the parser at all if the input string consists of only one number. This will be very common when used to parse values from various UI elements, as those are usually converted to a single number even if the user types in a complex expression.

Parsing now supports an additional parameter, Context, which can be used to specify various extra details, such as default values, expected number format, and so on. Using this context, parsing of expressions can also be completely turned off, and the function will only parse the string as a simple number. Finally, if the parser encounters an error, details about what happened can be retrieved from the Context.

Concepts that the parser now supports:

  • Digits 0-9.
  • Decimal point, based on a specified locale. Defaults to the user's system locale, can be overridden by the context variable.
  • Thousands separators (optional), again based on a specified locale as above.
  • Arithmetic operations: +, -, *, /. Note that support for modulo (%) has been removed in favor of percentage expressions, see below. Does anybody really need to be able to do modulo here?
  • Unary minus anywhere within the expression. (The previous implementation only allowed unary minus at the beginning of the expression. Now 3+-2 is correctly parsed as 1.)
  • Parentheses to specify order of evaluation. (Completely new in this PR.)
  • Suffixes k, m, b, g, t to specify large values (see Add support for scientific notation (2e3 = 2000) and suffixes like k, m, b, ... #58 for full details).
  • Scientific notation, now supporting both positive and negative exponents. (3e-2 evaluates to 0.03.)
  • Suffix % to specify a percentage of a maximum amount.

To explain on the latter: Imagine that I have an energy detector cover. I want this cover to emit redstone signal when power falls below 30% of the maximum stored amount. Previously, I would need to know what the maximum energy storage capacity of the machine is, and manually calculate and type a number equal to 30% of that into the cover's UI. With this change, I can type 30% as an expression, and this will be evaluated to 30% of the maximum value. The % functions as a postfix unary operator, so even expressions like 100% - 1000 are valid. (This would evaluate to 1000 less than the maximum value.)

Note that this functionality is not implemented in actual covers and such yet, I will add this later once this PR is merged. But the foundations for the parser to allow this are there. The only place where you can currently play with this is the TestTile, which has an input widget that is hardcoded to have a maximum value of 1000.

For examples of what the new parser can do, see MathExpressionTest.

This PR fixes the issues from #59. I have closed the second PR in favor of this rework.

@AbdielKavash AbdielKavash marked this pull request as ready for review February 7, 2024 13:33
@AbdielKavash AbdielKavash requested review from Dream-Master and a team February 7, 2024 13:34
Copy link
Member

@miozune miozune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good feature, good documentation. Amazing work!

But what do you think about moving this to GTNHLib? There're some request about adding such a feature for other places, GTNewHorizons/GT-New-Horizons-Modpack#12618 / GTNewHorizons/GT-New-Horizons-Modpack#14632

Optional: Currently +5 resulting in syntax error is not intuitive. How about making it parsed as 5 just like -5?
Optional: Allowing use of _ as separator would be neat.
Optional: Implementing toString for StackElement would be useful for debug.
Optional: Making error messages translatable by returning unlocalized name would be great.

// TODO:
// There should be a config option to default the number format to either the user's computer locale, or to a
// locale specified in a config. This way, users who have their computer locale set to a different one still have an
// option to input numbers using English conventions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ultimate solution would be completely separating the value stored in server/client widget and the text shown on client. That way player can customize formatting however they want, while server still knowing the actual value without formatting. However, it's way beyond the scope of this PR, and I doubt it could be made possible without breaking a lot of things. Maybe it could be easier with MUI2.

@AbdielKavash
Copy link
Member Author

AbdielKavash commented Feb 11, 2024

But what do you think about moving this to GTNHLib? There're some request about adding such a feature for other places, GTNewHorizons/GT-New-Horizons-Modpack#12618 / GTNewHorizons/GT-New-Horizons-Modpack#14632

Yes, that makes sense to me. The only issue I could see is that this would add a dependency on GTNHLib for mods which would otherwise be viable outside of GTNH.

  • Optional: Currently +5 resulting in syntax error is not intuitive. How about making it parsed as 5 just like -5?
  • Optional: Allowing use of _ as separator would be neat.
  • Optional: Implementing toString for StackElement would be useful for debug.
  • Optional: Making error messages translatable by returning unlocalized name would be great.

Agreed with all of this, and with most of the individual feedback above. Will implement soon.

The ultimate solution would be completely separating the value stored in server/client widget and the text shown on client. That way player can customize formatting however they want, while server still knowing the actual value without formatting. However, it's way beyond the scope of this PR, and I doubt it could be made possible without breaking a lot of things. Maybe it could be easier with MUI2.

The plan is to implement a separate NumericWidget, which exposes only the numeric value to the code (machine, cover, etc.) that is using it; and handles the string<->number conversion completely by itself. However the issue is, if you display the value to the player in the GUI as, for example, 1,000,000; and the player deletes the last four characters and confirms, you still need to parse the string 1,000 as the value 1000. So some amount of parsing is required; and if we want to make this locale-aware, then that needs to be followed.

@eigenraven
Copy link
Member

GTNHLib is safe to use outside of GTNH fyi

@miozune
Copy link
Member

miozune commented Feb 11, 2024

The only issue I could see is that this would add a dependency on GTNHLib for mods which would otherwise be viable outside of GTNH.

I can see only one repository that uses MUI1 and not related to NH at all, so I'd say that's ok. https://github.com/search?q=com.github.GTNewHorizons%3AModularUI+NOT+is%3Afork&type=code

However the issue is, if you display the value to the player in the GUI as, for example, 1,000,000; and the player deletes the last four characters and confirms, you still need to parse the string 1,000 as the value 1000. So some amount of parsing is required; and if we want to make this locale-aware, then that needs to be followed.

I mean, making client parse input text and send the actual value to the server, that'll do the trick. But I'm not sure if it's viable with MUI1 design.

@AbdielKavash
Copy link
Member Author

AbdielKavash commented Feb 17, 2024

Minor changes added:

  • Optional: Currently +5 resulting in syntax error is not intuitive. How about making it parsed as 5 just like -5?
  • Optional: Allowing use of _ as separator would be neat.
  • Optional: Implementing toString for StackElement would be useful for debug.
  • Some minor cleanup/refactoring.

Re locale stuff: I have realized the problem with locales in a multiplayer setting: if this is used for a numeric UI element (let's say threshold amount on a cover), it is possible that one player using the US locale enters a string 1000 which gets formatted as 1,000. This would then get stored in the cover's GUI element as a string. Another player using the FR locale would then open the same UI, and see a string which their client parses as 1 (as in, one, decimal separator, zero zero zero). So yes, if we want to make this locale-aware this needs to be a higher level change.

For now I always default the locale to EN_US, and I have added a disclaimer in the javadoc warning about this. As I have said above, I intend to implement a special widget that will manage this correctly.

Re moving: Please review the new changes, but do not merge this PR. If everything here is okay, I will make an identical PR to GTNHLib (and also to move over everything using this in other mods).

Once this is moved I will also add localization for error messages (since they will have to go in a different place).

@miozune
Copy link
Member

miozune commented Feb 18, 2024

Looks good, nice work!

@Dream-Master Dream-Master requested a review from a team February 18, 2024 09:42
@AbdielKavash
Copy link
Member Author

Discontinued and added to GTNHLib here: GTNewHorizons/GTNHLib#31.

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

Successfully merging this pull request may close these issues.

4 participants