-
Notifications
You must be signed in to change notification settings - Fork 8
feat: multi-expression implemented #19
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
base: main
Are you sure you want to change the base?
feat: multi-expression implemented #19
Conversation
|
Also, implicit functions, polar functions and parametric relations do not work anymore (you have simply removed all the required code for those features). Is this intentional? |
|
@nsh07 I accidently removed the features of implicit function polar function and parametric relation. I would do a commit by tomorrow evening as I was doing some refactoring in code to remove the clutterinside main application.cpp file. |
|
Sure, take your time. Your implementation is really good, and it is exactly what I was looking for. |
| namespace App::Core { | ||
|
|
||
| struct Expression { | ||
| std::array<char, 1024> expr; // expression as char array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this instead be a std::string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying this but faced some issues back then. As I remember that the exprtk compile and expression function require array of character. Not sure about it.
I would try to reuse it and check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not correct. std::string is the primary input parameter for the string representation of an expression for use with the compile method.
https://www.partow.net/programming/exprtk/exprtk.hpp_.html#line_24640
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok got it, will change the implementation. There might be something else back when I was structuring it. Thanks for the help
| for (size_t i = 0; i < functions.size(); ++i) { | ||
| if (!functions[i].visible) continue; | ||
| expressions[i].register_symbol_table(symbolTable); | ||
| parser.compile(functions[i].expr.data(), expressions[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to check the return value of the compile method. if it is false the function should not be further evaluated.
| for (size_t i = 0; i < functions.size(); ++i) { | ||
| if (!functions[i].visible) continue; | ||
| expressions[i].register_symbol_table(symbolTable); | ||
| parser.compile(functions[i].expr.data(), expressions[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions should only be compiled when they change. The Expression struct should perhaps hold the exprtk::expression instance. As constructing/destructing exprtk::parser/symbol_table/expression types on a per frame basis will be very expensive.
|
Thank you so much for your review @ArashPartow @guptanshuman124 please also resolve these change requests while you're working on the PR |
@ArashPartow thank you for the valuable feedback.I will take care of it. @nsh07 my current progress
|


Description
Semver Changes
Issues
Checklist