From 313d5e9deb6cf5c64f9e8035614204f8f8dfaf1f Mon Sep 17 00:00:00 2001 From: phkahler <14852918+phkahler@users.noreply.github.com> Date: Thu, 3 Aug 2023 13:16:30 -0400 Subject: [PATCH 01/30] Add named parameters via old Evil-Spritchanges. --- src/clipboard.cpp | 1 + src/constrainteq.cpp | 15 +++++++++++++-- src/drawconstraint.cpp | 3 +++ src/expr.cpp | 42 ++++++++++++++++++++++++++++++++++++++---- src/expr.h | 3 ++- src/mouse.cpp | 8 +++++++- src/sketch.h | 2 ++ 7 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/clipboard.cpp b/src/clipboard.cpp index 0275b1707..88993eff8 100644 --- a/src/clipboard.cpp +++ b/src/clipboard.cpp @@ -244,6 +244,7 @@ void GraphicsWindow::PasteClipboard(Vector trans, double theta, double scale) { c.reference = cc->reference; c.disp = cc->disp; c.comment = cc->comment; + c.expression = cc->expression; bool dontAddConstraint = false; switch(c.type) { case Constraint::Type::COMMENT: diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index c21d10721..b11add38f 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -248,6 +248,9 @@ void ConstraintBase::AddEq(IdList *l, const ExprVector &v, } void ConstraintBase::Generate(IdList *l) { + if(expression != "") { + Expr::From(expression.c_str(), false, l); + } switch(type) { case Type::PARALLEL: case Type::CUBIC_LINE_TANGENT: @@ -270,9 +273,17 @@ void ConstraintBase::Generate(IdList *l) { void ConstraintBase::GenerateEquations(IdList *l, bool forReference) const { - if(reference && !forReference) return; + Expr *exA = {}; + if(reference && !forReference) { + return; + } else { + if(expression != "") { + exA = Expr::From(expression.c_str(), false, &SK.param, NULL); + } else { + exA = Expr::From(valA); + } + } - Expr *exA = Expr::From(valA); switch(type) { case Type::PT_PT_DISTANCE: AddEq(l, Distance(workplane, ptA, ptB)->Minus(exA), 0); diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index e84c0ce48..acaf1460a 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -26,6 +26,9 @@ std::string Constraint::Label() const { // valA has units of distance result = SS.MmToStringSI(fabs(valA)); } + if(expression != "") { + result = expression; + } if(reference) { result += " REF"; } diff --git a/src/expr.cpp b/src/expr.cpp index d5ed87b58..9e0ee132b 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -621,6 +621,9 @@ class ExprParser { std::string::const_iterator it, end; std::vector stack; + std::set newParams; + IdList *params; + hConstraint hc; char ReadChar(); char PeekChar(); @@ -637,7 +640,8 @@ class ExprParser { bool Reduce(std::string *error); bool Parse(std::string *error, size_t reduceUntil = 0); - static Expr *Parse(const std::string &input, std::string *error); + static Expr *Parse(const std::string &input, std::string *error, IdList *params = NULL, int *paramsCount = 0, hConstraint hc = {0}); }; ExprParser::Token ExprParser::Token::From(TokenType type, Expr *expr) { @@ -735,6 +739,30 @@ ExprParser::Token ExprParser::Lex(std::string *error) { } else if(s == "pi") { t = Token::From(TokenType::OPERAND, Expr::Op::CONSTANT); t.expr->v = PI; + } else if(params != NULL) { + bool found = false; + for(const Param &p : *params) { + if(p.name != s) continue; + t = Token::From(TokenType::OPERAND, Expr::Op::PARAM); + t.expr->parh = p.h; + newParams.insert(p.h.v); + found = true; + } + if(!found) { + Param p = {}; + int count = 0; + + while(params->FindByIdNoOops(hc.param(count)) != NULL) { + count++; + } + + p.h = hc.param(count); + p.name = s; + params->Add(&p); + newParams.insert(p.h.v); + t = Token::From(TokenType::OPERAND, Expr::Op::PARAM); + t.expr->parh = p.h; + } } else { *error = "'" + s + "' is not a valid variable, function or constant"; } @@ -906,14 +934,19 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { return true; } -Expr *ExprParser::Parse(const std::string &input, std::string *error) { +Expr *ExprParser::Parse(const std::string &input, std::string *error, + IdList *params, int *paramsCount, hConstraint hc) { ExprParser parser; parser.it = input.cbegin(); parser.end = input.cend(); + parser.params = params; + parser.newParams.clear(); + parser.hc = hc; if(!parser.Parse(error)) return NULL; Token r = parser.PopOperand(error); if(r.IsError()) return NULL; + if(paramsCount != NULL) *paramsCount = parser.newParams.size(); return r.expr; } @@ -921,9 +954,10 @@ Expr *Expr::Parse(const std::string &input, std::string *error) { return ExprParser::Parse(input, error); } -Expr *Expr::From(const std::string &input, bool popUpError) { +Expr *Expr::From(const std::string &input, bool popUpError, + IdList *params, int *paramsCount, hConstraint hc) { std::string error; - Expr *e = ExprParser::Parse(input, &error); + Expr *e = ExprParser::Parse(input, &error, params, paramsCount, hc); if(!e) { dbp("Parse/lex error: %s", error.c_str()); if(popUpError) { diff --git a/src/expr.h b/src/expr.h index 7eef4e005..0e886c24e 100644 --- a/src/expr.h +++ b/src/expr.h @@ -98,7 +98,8 @@ class Expr { IdList *thenTry) const; static Expr *Parse(const std::string &input, std::string *error); - static Expr *From(const std::string &input, bool popUpError); + static Expr *From(const std::string &input, bool popUpError, + IdList *params = NULL, int *paramsCount = NULL, hConstraint hc = {0}); }; class ExprVector { diff --git a/src/mouse.cpp b/src/mouse.cpp index 0826713d0..7b68ba450 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1431,8 +1431,14 @@ void GraphicsWindow::EditControlDone(const std::string &s) { return; } - if(Expr *e = Expr::From(s, true)) { + int usedParams; + Expr *e = Expr::From(s, true, &SK.param, &usedParams); + + if(e) { SS.UndoRemember(); + if(usedParams > 0) { + c->expression = s; + } switch(c->type) { case Constraint::Type::PROJ_PT_DISTANCE: diff --git a/src/sketch.h b/src/sketch.h index 020f40815..dbf9ec4ed 100644 --- a/src/sketch.h +++ b/src/sketch.h @@ -621,6 +621,7 @@ class Param { double val; bool known; bool free; + std::string name; // Used only in the solver Param *substd; @@ -710,6 +711,7 @@ class ConstraintBase { bool reference; // a ref dimension, that generates no eqs std::string comment; // since comments are represented as constraints + std::string expression; bool Equals(const ConstraintBase &c) const { return type == c.type && group == c.group && workplane == c.workplane && From 5039a8501ec0fd8935721deee47208f6fc31bc97 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 7 Dec 2023 17:29:14 -0500 Subject: [PATCH 02/30] Save parameters to file, Edit parameters in GUI Edit parameterized constraints in the GUI, (instead of their computed values) and save those constraints to a file. --- src/file.cpp | 1 + src/mouse.cpp | 63 +++++++++++++++++++++++++++++++++++---------------- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/file.cpp b/src/file.cpp index c68ba1379..1b2c245b4 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -185,6 +185,7 @@ const SolveSpaceUI::SaveTable SolveSpaceUI::SAVED[] = { { 'c', "Constraint.other2", 'b', &(SS.sv.c.other2) }, { 'c', "Constraint.reference", 'b', &(SS.sv.c.reference) }, { 'c', "Constraint.comment", 'S', &(SS.sv.c.comment) }, + { 'c', "Constraint.expression", 'S', &(SS.sv.c.expression) }, { 'c', "Constraint.disp.offset.x", 'f', &(SS.sv.c.disp.offset.x) }, { 'c', "Constraint.disp.offset.y", 'f', &(SS.sv.c.disp.offset.y) }, { 'c', "Constraint.disp.offset.z", 'f', &(SS.sv.c.disp.offset.z) }, diff --git a/src/mouse.cpp b/src/mouse.cpp index 7b68ba450..5081a8474 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1373,26 +1373,49 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { default: { double value = fabs(c->valA); - // If displayed as radius, also edit as radius. - if(c->type == Constraint::Type::DIAMETER && c->other) - value /= 2; - - // Try showing value with default number of digits after decimal first. - if(c->type == Constraint::Type::LENGTH_RATIO || c->type == Constraint::Type::ARC_ARC_LEN_RATIO || c->type == Constraint::Type::ARC_LINE_LEN_RATIO) { - editValue = ssprintf("%.3f", value); - } else if(c->type == Constraint::Type::ANGLE) { - editValue = SS.DegreeToString(value); - } else { - editValue = SS.MmToString(value, true); - value /= SS.MmPerUnit(); - } - // If that's not enough to represent it exactly, show the value with as many - // digits after decimal as required, up to 10. - int digits = 0; - while(fabs(std::stod(editValue) - value) > 1e-10) { - editValue = ssprintf("%.*f", digits, value); - digits++; - } + // True if the quantity represented by this constraint is dimensionless (ratios, angles etc.) + bool dimless = c->type == Constraint::Type::LENGTH_RATIO || c->type == Constraint::Type::ARC_ARC_LEN_RATIO || c->type == Constraint::Type::ARC_LINE_LEN_RATIO || c->type == Constraint::Type::ANGLE; + + // Render a value, or render an expression + if(!c->expression.empty()) { + // Try showing value with default number of digits after decimal first. + if(dimless) { + // these ratios are dimensionless, so should not be scaled (not a length value) + if(c->type == Constraint::Type::ANGLE) { + editValue = SS.DegreeToString(value); + } else { + editValue = ssprintf("%.3f", value); + } + } else { + // If displayed as radius, also edit as radius. + if(c->type == Constraint::Type::DIAMETER && c->other) + value /= 2; + + editValue = SS.MmToString(value, true); + value /= SS.MmPerUnit(); + } + + // If that's not enough to represent it exactly, show the value with as many + // digits after decimal as required, up to 10. + int digits = 0; + while(fabs(std::stod(editValue) - value) > 1e-10) { + editValue = ssprintf("%.*f", digits, value); + digits++; + } + } else { + if(c->type == Constraint::Type::DIAMETER && c->other) { + // Edit as radius instead of diameter due to user config + editValue = ssprintf("%s/%d", c->expression.c_str(), 2*SS.MmPerUnit()); + + } else if(SS.MmPerUnit() == 1 || dimless) { + // Unit does not need scaling + editValue = c->expression; + } else { + // Unit needs dimension scaling + editValue = ssprintf("%s/%d", c->expression.c_str(), SS.MmPerUnit()); + } + } + editPlaceholder = "10.000000"; break; } From c9cb23fa7f73c9ad8ce0ffa30d7668ae5f5ddf65 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Wed, 13 Dec 2023 09:09:03 -0500 Subject: [PATCH 03/30] [dirty]: scale factors + geom inverse finder --- src/constrainteq.cpp | 7 ++- src/drawconstraint.cpp | 5 +- src/expr.cpp | 114 +++++++++++++++++++++++++++++++++++++++++ src/expr.h | 1 + src/mouse.cpp | 27 +++++++--- src/sketch.h | 7 +-- src/solvespace.cpp | 2 + 7 files changed, 149 insertions(+), 14 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index b11add38f..6bfa77f99 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -6,6 +6,7 @@ // Copyright 2008-2013 Jonathan Westhues. //----------------------------------------------------------------------------- #include "solvespace.h" +#include const hConstraint ConstraintBase::NO_CONSTRAINT = { 0 }; @@ -249,7 +250,8 @@ void ConstraintBase::AddEq(IdList *l, const ExprVector &v, void ConstraintBase::Generate(IdList *l) { if(expression != "") { - Expr::From(expression.c_str(), false, l); + //TODO order of ops/move to AST + Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, l); } switch(type) { case Type::PARALLEL: @@ -278,7 +280,8 @@ void ConstraintBase::GenerateEquations(IdList *l, return; } else { if(expression != "") { - exA = Expr::From(expression.c_str(), false, &SK.param, NULL); + //TODO order of ops/move to AST + exA = Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, &SK.param, NULL); } else { exA = Expr::From(valA); } diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index acaf1460a..c923c9109 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -27,7 +27,10 @@ std::string Constraint::Label() const { result = SS.MmToStringSI(fabs(valA)); } if(expression != "") { - result = expression; + result = expression ; + if(SS.MmPerUnit() != expr_scaling_to_base) { + result += "*" + std::to_string(expr_scaling_to_base/SS.MmPerUnit()); + } } if(reference) { result += " REF"; diff --git a/src/expr.cpp b/src/expr.cpp index 9e0ee132b..19f9f1981 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -7,6 +7,9 @@ // Copyright 2008-2013 Jonathan Westhues. //----------------------------------------------------------------------------- #include "solvespace.h" +#include +#include +#include ExprVector ExprVector::From(Expr *x, Expr *y, Expr *z) { ExprVector r = { x, y, z}; @@ -438,6 +441,113 @@ bool Expr::IsZeroConst() const { return op == Op::CONSTANT && EXACT(v == 0.0); } +//TODO naming +void remove_redundant(std::vector& exprs) { + for(long i=0; iv == 1/exprs[k]->v) { //TODO: tol + + exprs[k] = exprs[exprs.size()-1]; + exprs.pop_back(); + + exprs[i] = exprs[exprs.size()-1]; + exprs.pop_back(); + } + } + } +} + +void remove_inverses(std::vector& exprs1, std::vector& exprs2) { + for(long i=0; iv == exprs2[k]->v) { + exprs1[i] = exprs1[exprs1.size()-1]; + exprs1.pop_back(); + + exprs2[i] = exprs2[exprs2.size()-1]; + exprs2.pop_back(); + } + } + } +} + +// TODO: should this function be made part of FoldConstants? Seems be OK based on how FoldConstants is used +// This routine cancels constants that multiplicative inverses, that act at on the entire expression. +// Contract: this function may cancel ALL multiplicative inverses in the future, regardless of where they appear in tree +Expr* Expr::SimplifyInverses() { + //TODO: include string slices in expr to allow for smart string rebuilding + //intent: be able to switch between inches and mm a few times without getting huge daisy chains + std::vector numerators = std::vector(); + std::vector denominators = std::vector(); + + // bool indicates if we are in the numerator of the expression + std::stack> iter = std::stack>(); + + if(this->op == Op::TIMES || this->op == Op::DIV) { + iter.push(std::make_pair(this,true)); + } else { + return nullptr; + } + + while(!iter.empty()) { + Expr* head = iter.top().first; + bool is_numerator = iter.top().second; + iter.pop(); + + std::vector* local_numerators = is_numerator ? &numerators : &denominators; + std::vector* local_denominators = is_numerator ? &denominators : &numerators; + + switch(head->a->op) { + case Op::CONSTANT: + local_numerators->push_back(head->a); //TODO: this would be an opportune time to handle deletion + + // is there another numerator that is our inverse? + // is there a denominoator that is equal to us? + + // if sibling is to be removed... + + // if removed item is not our sibling... + // delete self, make sibling into parent + break; + case Op::DIV: + case Op::TIMES: + iter.push(std::make_pair(head->a, is_numerator)); + break; + default: + break; + } + + switch(head->b->op) { + case Op::CONSTANT: + if(head->op == Op::DIV) + local_denominators->push_back(head->b); + else + local_numerators->push_back(head->b); + break; + case Op::DIV: + case Op::TIMES: + if(head->op == Op::DIV) + iter.push(std::make_pair(head->b, !is_numerator)); + else + iter.push(std::make_pair(head->b, is_numerator)); + + break; + default: + break; + } + } + + remove_redundant(numerators); + remove_redundant(denominators); + remove_inverses(numerators,denominators); + + // delete constants in the tree (leaf nodes). + // All deleted constants have a parent that is TIMES or DIV that their non-deleted siblings should replace + // TODO: what if sibling is also deleted? + + return nullptr; +} + Expr *Expr::FoldConstants() { Expr *n = AllocExpr(); *n = *this; @@ -614,6 +724,10 @@ class ExprParser { TokenType type; Expr *expr; + // string indicies to show where the token started and ended + int slice_start; + int slice_end; + static Token From(TokenType type = TokenType::ERROR, Expr *expr = NULL); static Token From(TokenType type, Expr::Op op); bool IsError() const { return type == TokenType::ERROR; } diff --git a/src/expr.h b/src/expr.h index 0e886c24e..d4d3ac326 100644 --- a/src/expr.h +++ b/src/expr.h @@ -74,6 +74,7 @@ class Expr { bool DependsOn(hParam p) const; static bool Tol(double a, double b); bool IsZeroConst() const; + Expr *SimplifyInverses(); Expr *FoldConstants(); void Substitute(hParam oldh, hParam newh); diff --git a/src/mouse.cpp b/src/mouse.cpp index 5081a8474..18c8982a5 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -927,6 +927,7 @@ bool GraphicsWindow::MouseEvent(Platform::MouseEvent event) { } void GraphicsWindow::MouseLeftDown(double mx, double my, bool shiftDown, bool ctrlDown) { + //TODO: put scaling factor back if abandon editing constraint orig.mouseDown = true; if(window->IsEditorVisible()) { @@ -1377,7 +1378,7 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { bool dimless = c->type == Constraint::Type::LENGTH_RATIO || c->type == Constraint::Type::ARC_ARC_LEN_RATIO || c->type == Constraint::Type::ARC_LINE_LEN_RATIO || c->type == Constraint::Type::ANGLE; // Render a value, or render an expression - if(!c->expression.empty()) { + if(c->expression.empty()) { // Try showing value with default number of digits after decimal first. if(dimless) { // these ratios are dimensionless, so should not be scaled (not a length value) @@ -1403,16 +1404,17 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { digits++; } } else { + // Appears to be the wrong approach + // TODO: Simplify so that we don't have *25.4/25.4 in a bunch of expressions if(c->type == Constraint::Type::DIAMETER && c->other) { // Edit as radius instead of diameter due to user config - editValue = ssprintf("%s/%d", c->expression.c_str(), 2*SS.MmPerUnit()); - - } else if(SS.MmPerUnit() == 1 || dimless) { + editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); + } else if(dimless || c->expr_scaling_to_base == SS.MmPerUnit()) { // Unit does not need scaling editValue = c->expression; } else { // Unit needs dimension scaling - editValue = ssprintf("%s/%d", c->expression.c_str(), SS.MmPerUnit()); + editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/SS.MmPerUnit()); } } @@ -1443,6 +1445,7 @@ void GraphicsWindow::MouseLeftDoubleClick(double mx, double my) { } void GraphicsWindow::EditControlDone(const std::string &s) { + //TODO: convert inch expessions back into mm window->HideEditor(); window->Invalidate(); @@ -1460,7 +1463,13 @@ void GraphicsWindow::EditControlDone(const std::string &s) { if(e) { SS.UndoRemember(); if(usedParams > 0) { + // TODO: special cases for dimless, angles + c->expr_scaling_to_base = SS.MmPerUnit(); + // Suppose something was designed in inches but edited in mm. The click handler will put the scaling in + // The user will choose to leave the scaling alone. So we put the c->expression = s; + + e->SimplifyInverses(); } switch(c->type) { @@ -1471,14 +1480,15 @@ void GraphicsWindow::EditControlDone(const std::string &s) { case Constraint::Type::LENGTH_DIFFERENCE: case Constraint::Type::ARC_ARC_DIFFERENCE: case Constraint::Type::ARC_LINE_DIFFERENCE: { + // The sign is not displayed to the user, but this is a signed // distance internally. To flip the sign, the user enters a // negative distance. bool wasNeg = (c->valA < 0); if(wasNeg) { - c->valA = -SS.ExprToMm(e); + c->valA = -(e->Eval()); } else { - c->valA = SS.ExprToMm(e); + c->valA = (e->Eval()); } break; } @@ -1492,7 +1502,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { break; case Constraint::Type::DIAMETER: - c->valA = fabs(SS.ExprToMm(e)); + c->valA = fabs((e->Eval()) * c->expr_scaling_to_base); // If displayed and edited as radius, convert back // to diameter @@ -1502,6 +1512,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { default: // These are always positive, and they get the units conversion. + // Use ExprToMm since this unparameterized expressions simplify down c->valA = fabs(SS.ExprToMm(e)); break; } diff --git a/src/sketch.h b/src/sketch.h index dbf9ec4ed..59ea25449 100644 --- a/src/sketch.h +++ b/src/sketch.h @@ -709,9 +709,10 @@ class ConstraintBase { bool other; bool other2; - bool reference; // a ref dimension, that generates no eqs - std::string comment; // since comments are represented as constraints - std::string expression; + bool reference; // a ref dimension, that generates no eqs + std::string comment; // since comments are represented as constraints + std::string expression; // user-defined, may not be in mm (if entered in inch mode for example) + double expr_scaling_to_base; // scales expression to put in solvespace base units (like mm for distance). bool Equals(const ConstraintBase &c) const { return type == c.type && group == c.group && workplane == c.workplane && diff --git a/src/solvespace.cpp b/src/solvespace.cpp index aa135eaff..7d2087ce3 100644 --- a/src/solvespace.cpp +++ b/src/solvespace.cpp @@ -482,9 +482,11 @@ std::string SolveSpaceUI::DegreeToString(double v) { } } double SolveSpaceUI::ExprToMm(Expr *e) { + //TODO: This function is wrong for constraints, remove all uses of it return (e->Eval()) * MmPerUnit(); } double SolveSpaceUI::StringToMm(const std::string &str) { + // we scale constants like this because we directly store values in Mm return std::stod(str) * MmPerUnit(); } double SolveSpaceUI::ChordTolMm() { From a9cc9e9ee39d460953642754fccab164ef76a84d Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 30 Dec 2023 23:48:13 -0500 Subject: [PATCH 04/30] Heap allocate tokens & metadata preservation setup In order to preserve token order, we keep a vector of pointers to original tokens, and avoid creating new tokens so that we can preseve metadata from the original string. Previously, I tried keeping pointers into the vector, but of course during re-allocation those pointers became dangling. --- src/expr.cpp | 141 ++++++++++++++++++++++++++------------------------- 1 file changed, 71 insertions(+), 70 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index 19f9f1981..dfcabe8c5 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -724,17 +724,14 @@ class ExprParser { TokenType type; Expr *expr; - // string indicies to show where the token started and ended - int slice_start; - int slice_end; - - static Token From(TokenType type = TokenType::ERROR, Expr *expr = NULL); - static Token From(TokenType type, Expr::Op op); + static Token* From(TokenType type = TokenType::ERROR, Expr *expr = NULL); + static Token* From(TokenType type, Expr::Op op); bool IsError() const { return type == TokenType::ERROR; } }; std::string::const_iterator it, end; - std::vector stack; + std::vector stack; + std::vector tokens; //TODO: free tokens when done - problem: this vector reallocates and the pointers all become trash. Use a destructor? std::set newParams; IdList *params; hConstraint hc; @@ -745,12 +742,12 @@ class ExprParser { std::string ReadWord(); void SkipSpace(); - Token PopOperator(std::string *error); - Token PopOperand(std::string *error); + Token* PopOperator(std::string *error); + Token* PopOperand(std::string *error); - int Precedence(Token token); - Token LexNumber(std::string *error); - Token Lex(std::string *error); + int Precedence(Token* token); + Token* LexNumber(std::string *error); + Token* Lex(std::string *error); bool Reduce(std::string *error); bool Parse(std::string *error, size_t reduceUntil = 0); @@ -758,18 +755,20 @@ class ExprParser { hParam> *params = NULL, int *paramsCount = 0, hConstraint hc = {0}); }; -ExprParser::Token ExprParser::Token::From(TokenType type, Expr *expr) { - Token t; - t.type = type; - t.expr = expr; +// allocates +ExprParser::Token* ExprParser::Token::From(TokenType type, Expr *expr) { + Token* t = new Token(); + t->type = type; + t->expr = expr; return t; } -ExprParser::Token ExprParser::Token::From(TokenType type, Expr::Op op) { - Token t; - t.type = type; - t.expr = Expr::AllocExpr(); - t.expr->op = op; +// allocates +ExprParser::Token* ExprParser::Token::From(TokenType type, Expr::Op op) { + Token* t = new Token(); + t->type = type; + t->expr = Expr::AllocExpr(); + t->expr->op = op; return t; } @@ -803,7 +802,7 @@ void ExprParser::SkipSpace() { } } -ExprParser::Token ExprParser::LexNumber(std::string *error) { +ExprParser::Token* ExprParser::LexNumber(std::string *error) { std::string s; while(char c = PeekChar()) { @@ -818,20 +817,20 @@ ExprParser::Token ExprParser::LexNumber(std::string *error) { char *endptr; double d = strtod(s.c_str(), &endptr); - Token t = Token::From(); + Token* t = Token::From(); if(endptr == s.c_str() + s.size()) { t = Token::From(TokenType::OPERAND, Expr::Op::CONSTANT); - t.expr->v = d; + t->expr->v = d; } else { *error = "'" + s + "' is not a valid number"; } return t; } -ExprParser::Token ExprParser::Lex(std::string *error) { +ExprParser::Token* ExprParser::Lex(std::string *error) { SkipSpace(); - Token t = Token::From(); + Token* t = nullptr; char c = PeekChar(); if(isupper(c)) { std::string n = ReadWord(); @@ -852,13 +851,13 @@ ExprParser::Token ExprParser::Lex(std::string *error) { t = Token::From(TokenType::UNARY_OP, Expr::Op::ACOS); } else if(s == "pi") { t = Token::From(TokenType::OPERAND, Expr::Op::CONSTANT); - t.expr->v = PI; + t->expr->v = PI; } else if(params != NULL) { bool found = false; for(const Param &p : *params) { if(p.name != s) continue; t = Token::From(TokenType::OPERAND, Expr::Op::PARAM); - t.expr->parh = p.h; + t->expr->parh = p.h; newParams.insert(p.h.v); found = true; } @@ -875,7 +874,7 @@ ExprParser::Token ExprParser::Lex(std::string *error) { params->Add(&p); newParams.insert(p.h.v); t = Token::From(TokenType::OPERAND, Expr::Op::PARAM); - t.expr->parh = p.h; + t->expr->parh = p.h; } } else { *error = "'" + s + "' is not a valid variable, function or constant"; @@ -908,9 +907,9 @@ ExprParser::Token ExprParser::Lex(std::string *error) { return t; } -ExprParser::Token ExprParser::PopOperand(std::string *error) { - Token t = Token::From(); - if(stack.empty() || stack.back().type != TokenType::OPERAND) { +ExprParser::Token* ExprParser::PopOperand(std::string *error) { + Token* t = nullptr; + if(stack.empty() || stack.back()->type != TokenType::OPERAND) { *error = "Expected an operand"; } else { t = stack.back(); @@ -919,10 +918,10 @@ ExprParser::Token ExprParser::PopOperand(std::string *error) { return t; } -ExprParser::Token ExprParser::PopOperator(std::string *error) { - Token t = Token::From(); - if(stack.empty() || (stack.back().type != TokenType::UNARY_OP && - stack.back().type != TokenType::BINARY_OP)) { +ExprParser::Token* ExprParser::PopOperator(std::string *error) { + Token* t = nullptr; + if(stack.empty() || (stack.back()->type != TokenType::UNARY_OP && + stack.back()->type != TokenType::BINARY_OP)) { *error = "Expected an operator"; } else { t = stack.back(); @@ -931,44 +930,45 @@ ExprParser::Token ExprParser::PopOperator(std::string *error) { return t; } -int ExprParser::Precedence(Token t) { - ssassert(t.type == TokenType::BINARY_OP || - t.type == TokenType::UNARY_OP || - t.type == TokenType::OPERAND, +int ExprParser::Precedence(Token* t) { + ssassert(t->type == TokenType::BINARY_OP || + t->type == TokenType::UNARY_OP || + t->type == TokenType::OPERAND, "Unexpected token type"); - if(t.type == TokenType::UNARY_OP) { + if(t->type == TokenType::UNARY_OP) { return 30; - } else if(t.expr->op == Expr::Op::TIMES || - t.expr->op == Expr::Op::DIV) { + } else if(t->expr->op == Expr::Op::TIMES || + t->expr->op == Expr::Op::DIV) { return 20; - } else if(t.expr->op == Expr::Op::PLUS || - t.expr->op == Expr::Op::MINUS) { + } else if(t->expr->op == Expr::Op::PLUS || + t->expr->op == Expr::Op::MINUS) { return 10; - } else if(t.type == TokenType::OPERAND) { + } else if(t->type == TokenType::OPERAND) { return 0; } else ssassert(false, "Unexpected operator"); } bool ExprParser::Reduce(std::string *error) { - Token a = PopOperand(error); - if(a.IsError()) return false; + Token* a = PopOperand(error); + if(a->IsError()) return false; - Token op = PopOperator(error); - if(op.IsError()) return false; + Token* op = PopOperator(error); + if(op->IsError()) return false; - Token r = Token::From(TokenType::OPERAND); - switch(op.type) { + //Token r = Token::From(TokenType::OPERAND); + switch(op->type) { case TokenType::BINARY_OP: { - Token b = PopOperand(error); - if(b.IsError()) return false; - r.expr = b.expr->AnyOp(op.expr->op, a.expr); + Token* b = PopOperand(error); + if(b->IsError()) return false; + b->expr = b->expr->AnyOp(op->expr->op, a->expr); + stack.push_back(b); break; } case TokenType::UNARY_OP: { - Expr *e = a.expr; - switch(op.expr->op) { + Expr *e = a->expr; + switch(op->expr->op) { case Expr::Op::NEGATE: e = e->Negate(); break; case Expr::Op::SQRT: e = e->Sqrt(); break; case Expr::Op::SQUARE: e = e->Times(e); break; @@ -978,21 +978,22 @@ bool ExprParser::Reduce(std::string *error) { case Expr::Op::ACOS: e = e->ACos()->Times(Expr::From(180/PI)); break; default: ssassert(false, "Unexpected unary operator"); } - r.expr = e; + a->expr = e; + stack.push_back(a); break; } default: ssassert(false, "Unexpected operator"); } - stack.push_back(r); return true; } bool ExprParser::Parse(std::string *error, size_t reduceUntil) { while(true) { - Token t = Lex(error); - switch(t.type) { + Token* t = Lex(error); + tokens.push_back(t); + switch(t->type) { case TokenType::ERROR: return false; @@ -1002,7 +1003,7 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { if(!Reduce(error)) return false; } - if(t.type == TokenType::PAREN_RIGHT) { + if(t->type == TokenType::PAREN_RIGHT) { stack.push_back(t); } return true; @@ -1011,7 +1012,7 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { // sub-expression if(!Parse(error, /*reduceUntil=*/stack.size())) return false; - if(stack.empty() || stack.back().type != TokenType::PAREN_RIGHT) { + if(stack.empty() || stack.back()->type != TokenType::PAREN_RIGHT) { *error = "Expected ')'"; return false; } @@ -1020,11 +1021,11 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { } case TokenType::BINARY_OP: - if((stack.size() > reduceUntil && stack.back().type != TokenType::OPERAND) || + if((stack.size() > reduceUntil && stack.back()->type != TokenType::OPERAND) || stack.size() == reduceUntil) { - if(t.expr->op == Expr::Op::MINUS) { - t.type = TokenType::UNARY_OP; - t.expr->op = Expr::Op::NEGATE; + if(t->expr->op == Expr::Op::MINUS) { + t->type = TokenType::UNARY_OP; + t->expr->op = Expr::Op::NEGATE; stack.push_back(t); break; } @@ -1058,10 +1059,10 @@ Expr *ExprParser::Parse(const std::string &input, std::string *error, parser.hc = hc; if(!parser.Parse(error)) return NULL; - Token r = parser.PopOperand(error); - if(r.IsError()) return NULL; + Token* r = parser.PopOperand(error); + if(r->IsError()) return NULL; if(paramsCount != NULL) *paramsCount = parser.newParams.size(); - return r.expr; + return r->expr; } Expr *Expr::Parse(const std::string &input, std::string *error) { From 68945e442892feed5753875ee198a6b27451c6c7 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sun, 31 Dec 2023 00:48:47 -0500 Subject: [PATCH 05/30] Tracability between ordered tokens and expr nodes --- src/expr.cpp | 25 ++++++++++++++++--------- src/expr.h | 2 ++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index dfcabe8c5..5e3458b63 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -447,10 +447,10 @@ void remove_redundant(std::vector& exprs) { for(long k=i+1; k < exprs.size(); k++) { if(exprs[i]->v == 1/exprs[k]->v) { //TODO: tol - exprs[k] = exprs[exprs.size()-1]; + exprs[k]->to_delete = true; exprs.pop_back(); - exprs[i] = exprs[exprs.size()-1]; + exprs[i]->to_delete = true; exprs.pop_back(); } } @@ -461,10 +461,10 @@ void remove_inverses(std::vector& exprs1, std::vector& exprs2) { for(long i=0; iv == exprs2[k]->v) { - exprs1[i] = exprs1[exprs1.size()-1]; + exprs1[i]->to_delete = true; exprs1.pop_back(); - exprs2[i] = exprs2[exprs2.size()-1]; + exprs2[k]->to_delete = true; exprs2.pop_back(); } } @@ -956,13 +956,16 @@ bool ExprParser::Reduce(std::string *error) { Token* op = PopOperator(error); if(op->IsError()) return false; - //Token r = Token::From(TokenType::OPERAND); switch(op->type) { case TokenType::BINARY_OP: { Token* b = PopOperand(error); if(b->IsError()) return false; - b->expr = b->expr->AnyOp(op->expr->op, a->expr); - stack.push_back(b); + + // gives the operand children: + // semantically this subtree represents an operand, so we change the token type accordingly + op->type = TokenType::OPERAND; + op->expr = b->expr->AnyOp(op->expr->op, a->expr); + stack.push_back(op); break; } @@ -978,8 +981,9 @@ bool ExprParser::Reduce(std::string *error) { case Expr::Op::ACOS: e = e->ACos()->Times(Expr::From(180/PI)); break; default: ssassert(false, "Unexpected unary operator"); } - a->expr = e; - stack.push_back(a); + op->type = TokenType::OPERAND; + op->expr = e; + stack.push_back(op); break; } @@ -1062,6 +1066,9 @@ Expr *ExprParser::Parse(const std::string &input, std::string *error, Token* r = parser.PopOperand(error); if(r->IsError()) return NULL; if(paramsCount != NULL) *paramsCount = parser.newParams.size(); + + r->expr->SimplifyInverses(); + return r->expr; } diff --git a/src/expr.h b/src/expr.h index d4d3ac326..30974b918 100644 --- a/src/expr.h +++ b/src/expr.h @@ -45,6 +45,8 @@ class Expr { Expr *b; }; + bool to_delete; + Expr() = default; Expr(double val) : op(Op::CONSTANT) { v = val; } From 9991b2a709532b66b5f49ffc8cc7e8fedda0cb25 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sun, 31 Dec 2023 01:42:44 -0500 Subject: [PATCH 06/30] Preserve original token types --- src/expr.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index 5e3458b63..5b948b36e 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -909,7 +909,12 @@ ExprParser::Token* ExprParser::Lex(std::string *error) { ExprParser::Token* ExprParser::PopOperand(std::string *error) { Token* t = nullptr; - if(stack.empty() || stack.back()->type != TokenType::OPERAND) { + if(stack.empty() + || ( + stack.back()->type != TokenType::OPERAND + && (stack.back()->expr == nullptr) + ) + ) { *error = "Expected an operand"; } else { t = stack.back(); @@ -963,7 +968,6 @@ bool ExprParser::Reduce(std::string *error) { // gives the operand children: // semantically this subtree represents an operand, so we change the token type accordingly - op->type = TokenType::OPERAND; op->expr = b->expr->AnyOp(op->expr->op, a->expr); stack.push_back(op); break; @@ -981,7 +985,6 @@ bool ExprParser::Reduce(std::string *error) { case Expr::Op::ACOS: e = e->ACos()->Times(Expr::From(180/PI)); break; default: ssassert(false, "Unexpected unary operator"); } - op->type = TokenType::OPERAND; op->expr = e; stack.push_back(op); break; From 0b9f32e33dc6f22986f7b03ec5df14f270b2be42 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sun, 31 Dec 2023 02:19:08 -0500 Subject: [PATCH 07/30] bubble up deletion --- src/expr.cpp | 18 +++++++++++++++++- src/expr.h | 10 +++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index 5b948b36e..7f4ae608b 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -441,7 +441,7 @@ bool Expr::IsZeroConst() const { return op == Op::CONSTANT && EXACT(v == 0.0); } -//TODO naming +//TODO naming/scope void remove_redundant(std::vector& exprs) { for(long i=0; i& exprs1, std::vector& exprs2) { } } +void bubble_delete(Expr* root) { + if(root->a != nullptr) { + bubble_delete(root->a); + root->to_delete = true; + } + + // this checks for enum variants that imply the union in Expr has Expr* b + if((uint32_t)root->op >= 100 && (uint32_t)root->op <=103) { + bubble_delete(root->b); + root->to_delete = true; + } + + // just because a parent is deleted doesn't mean its children are deleted +} + // TODO: should this function be made part of FoldConstants? Seems be OK based on how FoldConstants is used // This routine cancels constants that multiplicative inverses, that act at on the entire expression. // Contract: this function may cancel ALL multiplicative inverses in the future, regardless of where they appear in tree @@ -540,6 +555,7 @@ Expr* Expr::SimplifyInverses() { remove_redundant(numerators); remove_redundant(denominators); remove_inverses(numerators,denominators); + bubble_delete(this); // delete constants in the tree (leaf nodes). // All deleted constants have a parent that is TIMES or DIV that their non-deleted siblings should replace diff --git a/src/expr.h b/src/expr.h index 30974b918..68c126763 100644 --- a/src/expr.h +++ b/src/expr.h @@ -37,12 +37,12 @@ class Expr { }; Op op; - Expr *a; + Expr *a; //nullptr if this is a PARAM, PARAM_PTR, CONSTANT, VARIABLE union { - double v; - hParam parh; - Param *parp; - Expr *b; + double v; // this variant if CONSTANT + hParam parh; // this variant if PARAM + Param *parp; // this variant if PARAM_PTR + Expr *b; // nullptr if this is a unary op, }; bool to_delete; From de0572fad2f3fd7227a124adc536259a77b76bbe Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sun, 31 Dec 2023 13:24:36 -0500 Subject: [PATCH 08/30] proper deletion and parentheticals --- src/expr.cpp | 102 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 12 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index 7f4ae608b..a0a242091 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -441,6 +441,7 @@ bool Expr::IsZeroConst() const { return op == Op::CONSTANT && EXACT(v == 0.0); } + //TODO naming/scope void remove_redundant(std::vector& exprs) { for(long i=0; i& exprs1, std::vector& exprs2) { } } -void bubble_delete(Expr* root) { - if(root->a != nullptr) { - bubble_delete(root->a); - root->to_delete = true; - } +//TODO: fix parenthesis edge case: 2*c/(2*3*4) because 2*c/3*4 +void bubble_delete(Expr** parent) { + Expr* a_ptr = nullptr; + bool del_a = false; + Expr* b_ptr = nullptr; + bool del_b = false; + + if((*parent)->a != nullptr) { + bubble_delete(&(*parent)->a); + if(!(*parent)->a->to_delete) { + a_ptr = (*parent)->a; + } else { + del_a = true; + } + } // this checks for enum variants that imply the union in Expr has Expr* b - if((uint32_t)root->op >= 100 && (uint32_t)root->op <=103) { - bubble_delete(root->b); - root->to_delete = true; + if((uint32_t)(*parent)->op >= 100 && (uint32_t)(*parent)->op <=103) { + bubble_delete(&(*parent)->b); + if(!(*parent)->b->to_delete) { + b_ptr = (*parent)->b; + } else { + del_b = true; + } + } + + if(del_a && del_b) { + (*parent)->to_delete = true; + //TODO: conditionally free a & b + } else if(del_a && b_ptr != nullptr) { //TODO: check if uni-operato + (*parent)->to_delete = true; + (*parent) = b_ptr; + } else if(del_b && a_ptr != nullptr) { + (*parent)->to_delete = true; + (*parent) = a_ptr; } - - // just because a parent is deleted doesn't mean its children are deleted } // TODO: should this function be made part of FoldConstants? Seems be OK based on how FoldConstants is used @@ -555,13 +579,15 @@ Expr* Expr::SimplifyInverses() { remove_redundant(numerators); remove_redundant(denominators); remove_inverses(numerators,denominators); - bubble_delete(this); + + Expr* new_expr = this; + bubble_delete(&new_expr); // delete constants in the tree (leaf nodes). // All deleted constants have a parent that is TIMES or DIV that their non-deleted siblings should replace // TODO: what if sibling is also deleted? - return nullptr; + return new_expr; } Expr *Expr::FoldConstants() { @@ -1072,6 +1098,56 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { return true; } +//TODO: clean this up, make this auto edit the original string, fix auto-added scale factors in the expressions in tokens so they print properly or are fully suppressed depending on mode +void print_tokens(std::vector tokens) { + std::string* str = new std::string(); + + for(ExprParser::Token* token : tokens) { + + if(token->type == ExprParser::TokenType::PAREN_LEFT) { + str->append("("); + continue; + } else if(token->type == ExprParser::TokenType::PAREN_RIGHT) { + str->append(")"); + continue; + } else if(token->expr != nullptr) { + if(token->expr->to_delete) { + continue; + } + + + switch(token->expr->op) { + case Expr::Op::PARAM: + case Expr::Op::PARAM_PTR: + case Expr::Op::VARIABLE: + str->append("param"); + break; + case Expr::Op::CONSTANT: + str->append(std::to_string(token->expr->v)); + break; + case Expr::Op::PLUS: + str->append("+"); + break; + case Expr::Op::MINUS: + str->append("-"); + break; + case Expr::Op::TIMES: + str->append("*"); + break; + case Expr::Op::DIV: + str->append("/"); + break; + //TODO: UNARY OPS + default: + str->append("?"); + } + } + } + + dbp("%s", str->c_str()); + delete str; +} + Expr *ExprParser::Parse(const std::string &input, std::string *error, IdList *params, int *paramsCount, hConstraint hc) { ExprParser parser; @@ -1088,6 +1164,8 @@ Expr *ExprParser::Parse(const std::string &input, std::string *error, r->expr->SimplifyInverses(); + print_tokens(parser.tokens); + return r->expr; } From 6bb6610eae94d717556b9bac4a502ec85472f5fb Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 23 Mar 2024 18:14:18 -0700 Subject: [PATCH 09/30] debug printing and whitespace changes --- src/expr.cpp | 9 ++++++--- src/expr.h | 4 ++-- src/mouse.cpp | 13 ++++++------- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index a0a242091..d5deb980c 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -792,6 +792,7 @@ class ExprParser { Token* Lex(std::string *error); bool Reduce(std::string *error); bool Parse(std::string *error, size_t reduceUntil = 0); + void PrintTokens(); static Expr *Parse(const std::string &input, std::string *error, IdList *params = NULL, int *paramsCount = 0, hConstraint hc = {0}); @@ -1099,7 +1100,7 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { } //TODO: clean this up, make this auto edit the original string, fix auto-added scale factors in the expressions in tokens so they print properly or are fully suppressed depending on mode -void print_tokens(std::vector tokens) { +void ExprParser::PrintTokens() { std::string* str = new std::string(); for(ExprParser::Token* token : tokens) { @@ -1118,9 +1119,11 @@ void print_tokens(std::vector tokens) { switch(token->expr->op) { case Expr::Op::PARAM: + str->append(this->params->FindById(token->expr->parh)->name); + break; case Expr::Op::PARAM_PTR: case Expr::Op::VARIABLE: - str->append("param"); + str->append("?param"); break; case Expr::Op::CONSTANT: str->append(std::to_string(token->expr->v)); @@ -1164,7 +1167,7 @@ Expr *ExprParser::Parse(const std::string &input, std::string *error, r->expr->SimplifyInverses(); - print_tokens(parser.tokens); + parser.PrintTokens(); return r->expr; } diff --git a/src/expr.h b/src/expr.h index 68c126763..4a0c7206d 100644 --- a/src/expr.h +++ b/src/expr.h @@ -40,8 +40,8 @@ class Expr { Expr *a; //nullptr if this is a PARAM, PARAM_PTR, CONSTANT, VARIABLE union { double v; // this variant if CONSTANT - hParam parh; // this variant if PARAM - Param *parp; // this variant if PARAM_PTR + hParam parh; + Param *parp; Expr *b; // nullptr if this is a unary op, }; diff --git a/src/mouse.cpp b/src/mouse.cpp index 18c8982a5..2eda34f8e 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1463,13 +1463,12 @@ void GraphicsWindow::EditControlDone(const std::string &s) { if(e) { SS.UndoRemember(); if(usedParams > 0) { - // TODO: special cases for dimless, angles - c->expr_scaling_to_base = SS.MmPerUnit(); - // Suppose something was designed in inches but edited in mm. The click handler will put the scaling in - // The user will choose to leave the scaling alone. So we put the - c->expression = s; - - e->SimplifyInverses(); + // TODO: special cases for dimless, angles + c->expr_scaling_to_base = SS.MmPerUnit(); + // Suppose something was designed in inches but edited in mm. The click handler will put the scaling in + // The user will choose to leave the scaling alone. So we put the + c->expression = s; + e->SimplifyInverses(); } switch(c->type) { From 1539229d3f69b1f2122fbe73bcd1286c74028b21 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 6 Apr 2024 01:46:41 -0700 Subject: [PATCH 10/30] stubs for relation constraint --- CMakeLists.txt | 4 +++- cmake/c_flag_overrides.cmake | 2 ++ cmake/cxx_flag_overrides.cmake | 2 ++ src/constraint.cpp | 18 ++++++++++++++++++ src/constrainteq.cpp | 9 +++++++++ src/drawconstraint.cpp | 1 + src/expr.cpp | 3 +++ src/graphicswin.cpp | 1 + src/mouse.cpp | 16 +++++++++++++++- src/sketch.h | 10 ++++++---- src/ui.h | 1 + 11 files changed, 61 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5a012553f..3f8c2acdd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -309,7 +309,9 @@ if(ENABLE_GUI) "${CMAKE_SOURCE_DIR}/extlib/si/siapp.lib") endif() elseif(APPLE) - find_package(OpenGL REQUIRED) + #find_package(OpenGL REQUIRED) + set(CMAKE_C_FLAGS "-framework OpenGL ${CMAKE_C_FLAGS}") + set(CMAKE_CXX_FLAGS "-framework OpenGL ${CMAKE_CXX_FLAGS}") find_library(APPKIT_LIBRARY AppKit REQUIRED) elseif(EMSCRIPTEN) # Everything is built in diff --git a/cmake/c_flag_overrides.cmake b/cmake/c_flag_overrides.cmake index 978b6b659..014d7ada7 100644 --- a/cmake/c_flag_overrides.cmake +++ b/cmake/c_flag_overrides.cmake @@ -8,3 +8,5 @@ endif() if(EMSCRIPTEN) set(CMAKE_C_FLAGS_DEBUG_INIT "-g4") endif() + +set(CMAKE_C_FLAGS_DEBUG_INIT "-g") diff --git a/cmake/cxx_flag_overrides.cmake b/cmake/cxx_flag_overrides.cmake index 9c8d15fe6..ec43ae189 100644 --- a/cmake/cxx_flag_overrides.cmake +++ b/cmake/cxx_flag_overrides.cmake @@ -8,3 +8,5 @@ endif() if(EMSCRIPTEN) set(CMAKE_CXX_FLAGS_DEBUG_INIT "-g4") endif() + +set(CMAKE_CXX_FLAGS_DEBUG_INIT "-g") diff --git a/src/constraint.cpp b/src/constraint.cpp index 705feec6b..4a9e7c6e0 100644 --- a/src/constraint.cpp +++ b/src/constraint.cpp @@ -48,6 +48,7 @@ std::string Constraint::DescriptionString() const { case Type::EQUAL_LINE_ARC_LEN: s = C_("constr-name", "eq-line-len-arc-len"); break; case Type::WHERE_DRAGGED: s = C_("constr-name", "lock-where-dragged"); break; case Type::COMMENT: s = C_("constr-name", "comment"); break; + case Type::RELATION: s = C_("constr-name", "relation"); break; default: s = "???"; break; } @@ -889,6 +890,23 @@ void Constraint::MenuConstrain(Command id) { } break; + case Command::RELATION: + if(gs.points == 1 && gs.n == 1) { + c.type = Type::RELATION; + c.ptA = gs.point[0]; + c.group = SS.GW.activeGroup; + c.workplane = SS.GW.ActiveWorkplane(); + c.expression = _("x"); + AddConstraint(&c); + newcons.push_back(c); + } else { + SS.GW.pending.operation = GraphicsWindow::Pending::COMMAND; + SS.GW.pending.command = Command::RELATION; + SS.GW.pending.description = _("click center of comment text"); + SS.ScheduleShowTW(); + } + break; + default: ssassert(false, "Unexpected menu ID"); } for (auto nc:newcons){ diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 6bfa77f99..157ddcbb9 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -6,6 +6,7 @@ // Copyright 2008-2013 Jonathan Westhues. //----------------------------------------------------------------------------- #include "solvespace.h" +#include #include const hConstraint ConstraintBase::NO_CONSTRAINT = { 0 }; @@ -26,6 +27,7 @@ bool ConstraintBase::HasLabel() const { case Type::ARC_LINE_DIFFERENCE: case Type::ANGLE: case Type::COMMENT: + case Type::RELATION: return true; default: @@ -61,6 +63,7 @@ bool ConstraintBase::IsProjectible() const { case Type::PERPENDICULAR: case Type::WHERE_DRAGGED: case Type::COMMENT: + case Type::RELATION: return true; case Type::PT_PLANE_DISTANCE: @@ -1067,6 +1070,12 @@ void ConstraintBase::GenerateEquations(IdList *l, case Type::COMMENT: return; + + case Type::RELATION: + std::fprintf(stderr, "adding relation with expr! %s", exA->Print().c_str()); + AddEq(l, exA, 0); //TODO + return; + } ssassert(false, "Unexpected constraint ID"); } diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index c923c9109..2792dbcd9 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -1293,6 +1293,7 @@ void Constraint::DoLayout(DrawAs how, Canvas *canvas, } return; + case Type::RELATION: case Type::COMMENT: { Vector u, v; if(workplane == Entity::FREE_IN_3D) { diff --git a/src/expr.cpp b/src/expr.cpp index d5deb980c..6d762a2e2 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -1042,6 +1042,9 @@ bool ExprParser::Reduce(std::string *error) { bool ExprParser::Parse(std::string *error, size_t reduceUntil) { while(true) { Token* t = Lex(error); + if(error != NULL && error->length() != 0) { + printf("Error %s", error); + } tokens.push_back(t); switch(t->type) { case TokenType::ERROR: diff --git a/src/graphicswin.cpp b/src/graphicswin.cpp index 38095c8d0..e2a252db2 100644 --- a/src/graphicswin.cpp +++ b/src/graphicswin.cpp @@ -165,6 +165,7 @@ const MenuEntry Menu[] = { { 1, N_("Lock Point Where &Dragged"), Command::WHERE_DRAGGED, ']', KN, mCon }, { 1, NULL, Command::NONE, 0, KN, NULL }, { 1, N_("Comment"), Command::COMMENT, ';', KN, mCon }, +{ 1, N_("Relation"), Command::RELATION, ':', KN, mCon }, { 0, N_("&Analyze"), Command::NONE, 0, KN, mAna }, { 1, N_("Measure &Volume"), Command::VOLUME, C|S|'v', KN, mAna }, diff --git a/src/mouse.cpp b/src/mouse.cpp index 2eda34f8e..b2c5bb6a9 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1162,6 +1162,19 @@ void GraphicsWindow::MouseLeftDown(double mx, double my, bool shiftDown, bool ct hc = Constraint::AddConstraint(&c); break; } + + case Command::RELATION: { + //TODO + ClearSuper(); + Constraint c = {}; + c.group = SS.GW.activeGroup; + c.workplane = SS.GW.ActiveWorkplane(); + c.type = Constraint::Type::RELATION; + c.disp.offset = v; + c.expression = _("x"); + hc = Constraint::AddConstraint(&c); + break; + } default: ssassert(false, "Unexpected pending menu id"); } break; @@ -1492,6 +1505,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { break; } case Constraint::Type::ANGLE: + case Constraint::Type::RELATION: case Constraint::Type::LENGTH_RATIO: case Constraint::Type::ARC_ARC_LEN_RATIO: case Constraint::Type::ARC_LINE_LEN_RATIO: @@ -1511,7 +1525,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { default: // These are always positive, and they get the units conversion. - // Use ExprToMm since this unparameterized expressions simplify down + // Use ExprToMm since this unparameterized expressions simplify down c->valA = fabs(SS.ExprToMm(e)); break; } diff --git a/src/sketch.h b/src/sketch.h index 59ea25449..c90bb0a22 100644 --- a/src/sketch.h +++ b/src/sketch.h @@ -689,7 +689,9 @@ class ConstraintBase { ARC_LINE_LEN_RATIO = 211, ARC_ARC_DIFFERENCE = 212, ARC_LINE_DIFFERENCE = 213, - COMMENT = 1000 + COMMENT = 1000, + //like COMMENT, but its contents are an equation (in particular, a relation). + RELATION = 1001 }; Type type; @@ -709,9 +711,9 @@ class ConstraintBase { bool other; bool other2; - bool reference; // a ref dimension, that generates no eqs - std::string comment; // since comments are represented as constraints - std::string expression; // user-defined, may not be in mm (if entered in inch mode for example) + bool reference; // a ref dimension, that generates no eqs + std::string comment; // since comments are represented as constraints + std::string expression; // user-defined, may not be in mm (if entered in inch mode for example) double expr_scaling_to_base; // scales expression to put in solvespace base units (like mm for distance). bool Equals(const ConstraintBase &c) const { diff --git a/src/ui.h b/src/ui.h index 49c1be64f..a3dd6164f 100644 --- a/src/ui.h +++ b/src/ui.h @@ -158,6 +158,7 @@ enum class Command : uint32_t { ORIENTED_SAME, WHERE_DRAGGED, COMMENT, + RELATION, // Analyze VOLUME, AREA, From bba05a68dcd07e85dfe07157ca9ff6ec08901efe Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 6 Apr 2024 02:47:44 -0700 Subject: [PATCH 11/30] relations without eq operator --- src/drawconstraint.cpp | 1 + src/mouse.cpp | 90 +++++++++++++++++++++--------------------- 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index 2792dbcd9..3facb662f 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -1362,6 +1362,7 @@ hStyle Constraint::GetStyle() const { bool Constraint::HasLabel() const { switch(type) { case Type::COMMENT: + case Type::RELATION: case Type::PT_PT_DISTANCE: case Type::PT_PLANE_DISTANCE: case Type::PT_LINE_DISTANCE: diff --git a/src/mouse.cpp b/src/mouse.cpp index b2c5bb6a9..c2417f8ba 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1164,14 +1164,13 @@ void GraphicsWindow::MouseLeftDown(double mx, double my, bool shiftDown, bool ct } case Command::RELATION: { - //TODO ClearSuper(); Constraint c = {}; c.group = SS.GW.activeGroup; c.workplane = SS.GW.ActiveWorkplane(); c.type = Constraint::Type::RELATION; c.disp.offset = v; - c.expression = _("x"); + c.expression = _("x"); hc = Constraint::AddConstraint(&c); break; } @@ -1366,6 +1365,7 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { Constraint *c = SK.GetConstraint(constraintBeingEdited); if(!c->HasLabel()) { // Not meaningful to edit a constraint without a dimension + fprintf(stderr, "Wont edit without label"); return; } if(c->reference) { @@ -1387,49 +1387,49 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { default: { double value = fabs(c->valA); - // True if the quantity represented by this constraint is dimensionless (ratios, angles etc.) - bool dimless = c->type == Constraint::Type::LENGTH_RATIO || c->type == Constraint::Type::ARC_ARC_LEN_RATIO || c->type == Constraint::Type::ARC_LINE_LEN_RATIO || c->type == Constraint::Type::ANGLE; - - // Render a value, or render an expression - if(c->expression.empty()) { - // Try showing value with default number of digits after decimal first. - if(dimless) { - // these ratios are dimensionless, so should not be scaled (not a length value) - if(c->type == Constraint::Type::ANGLE) { - editValue = SS.DegreeToString(value); - } else { - editValue = ssprintf("%.3f", value); - } - } else { - // If displayed as radius, also edit as radius. - if(c->type == Constraint::Type::DIAMETER && c->other) - value /= 2; - - editValue = SS.MmToString(value, true); - value /= SS.MmPerUnit(); - } - - // If that's not enough to represent it exactly, show the value with as many - // digits after decimal as required, up to 10. - int digits = 0; - while(fabs(std::stod(editValue) - value) > 1e-10) { - editValue = ssprintf("%.*f", digits, value); - digits++; - } - } else { - // Appears to be the wrong approach - // TODO: Simplify so that we don't have *25.4/25.4 in a bunch of expressions - if(c->type == Constraint::Type::DIAMETER && c->other) { - // Edit as radius instead of diameter due to user config - editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); - } else if(dimless || c->expr_scaling_to_base == SS.MmPerUnit()) { - // Unit does not need scaling - editValue = c->expression; - } else { - // Unit needs dimension scaling - editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/SS.MmPerUnit()); - } - } + // True if the quantity represented by this constraint is dimensionless (ratios, angles etc.) + bool dimless = c->type == Constraint::Type::LENGTH_RATIO || c->type == Constraint::Type::ARC_ARC_LEN_RATIO || c->type == Constraint::Type::ARC_LINE_LEN_RATIO || c->type == Constraint::Type::ANGLE || c->type == Constraint::Type::RELATION; + + // Render a value, or render an expression + if(c->expression.empty()) { + // Try showing value with default number of digits after decimal first. + if(dimless) { + // these ratios are dimensionless, so should not be scaled (not a length value) + if(c->type == Constraint::Type::ANGLE) { + editValue = SS.DegreeToString(value); + } else { + editValue = ssprintf("%.3f", value); + } + } else { + // If displayed as radius, also edit as radius. + if(c->type == Constraint::Type::DIAMETER && c->other) + value /= 2; + + editValue = SS.MmToString(value, true); + value /= SS.MmPerUnit(); + } + + // If that's not enough to represent it exactly, show the value with as many + // digits after decimal as required, up to 10. + int digits = 0; + while(fabs(std::stod(editValue) - value) > 1e-10) { + editValue = ssprintf("%.*f", digits, value); + digits++; + } + } else { + // Appears to be the wrong approach + // TODO: Simplify so that we don't have *25.4/25.4 in a bunch of expressions + if(c->type == Constraint::Type::DIAMETER && c->other) { + // Edit as radius instead of diameter due to user config + editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); + } else if(dimless || c->expr_scaling_to_base == SS.MmPerUnit()) { + // Unit does not need scaling + editValue = c->expression; + } else { + // Unit needs dimension scaling + editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/SS.MmPerUnit()); + } + } editPlaceholder = "10.000000"; break; From 1ad0fcdda0edd057cd10dadaf916369272bbe9a1 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 6 Apr 2024 04:22:05 -0700 Subject: [PATCH 12/30] expressions with eq --- src/constraint.cpp | 2 +- src/constrainteq.cpp | 14 +++++++++++--- src/expr.h | 1 + src/mouse.cpp | 41 +++++++++++++++++++++++++++++++++++++---- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/constraint.cpp b/src/constraint.cpp index 4a9e7c6e0..7d96b8705 100644 --- a/src/constraint.cpp +++ b/src/constraint.cpp @@ -896,7 +896,7 @@ void Constraint::MenuConstrain(Command id) { c.ptA = gs.point[0]; c.group = SS.GW.activeGroup; c.workplane = SS.GW.ActiveWorkplane(); - c.expression = _("x"); + c.expression = _("x=5"); AddConstraint(&c); newcons.push_back(c); } else { diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 157ddcbb9..a04dc245e 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -252,7 +252,11 @@ void ConstraintBase::AddEq(IdList *l, const ExprVector &v, } void ConstraintBase::Generate(IdList *l) { - if(expression != "") { + if(type == Constraint::Type::RELATION) { + size_t eqpos = expression.find_first_of("="); + //TODO: validation that only one equals sign appears, or give up on having this pattern plastered everywhere and move into expression parser etc. + Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); + } else if(expression != "") { //TODO order of ops/move to AST Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, l); } @@ -282,8 +286,12 @@ void ConstraintBase::GenerateEquations(IdList *l, if(reference && !forReference) { return; } else { - if(expression != "") { - //TODO order of ops/move to AST + if(type == Constraint::Type::RELATION) { + size_t eqpos = expression.find_first_of("="); + //TODO: validation that only one equals sign appears, or give up on having this pattern plastered everywhere and move into expression parser etc. + exA = Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); + } else if(expression != "") { + //TODO order of ops/move to AST exA = Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, &SK.param, NULL); } else { exA = Expr::From(valA); diff --git a/src/expr.h b/src/expr.h index 4a0c7206d..364827823 100644 --- a/src/expr.h +++ b/src/expr.h @@ -26,6 +26,7 @@ class Expr { MINUS = 101, TIMES = 102, DIV = 103, + // Unary ops NEGATE = 104, SQRT = 105, diff --git a/src/mouse.cpp b/src/mouse.cpp index c2417f8ba..9f6dadb76 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1170,7 +1170,7 @@ void GraphicsWindow::MouseLeftDown(double mx, double my, bool shiftDown, bool ct c.workplane = SS.GW.ActiveWorkplane(); c.type = Constraint::Type::RELATION; c.disp.offset = v; - c.expression = _("x"); + c.expression = _("x=5"); hc = Constraint::AddConstraint(&c); break; } @@ -1458,7 +1458,7 @@ void GraphicsWindow::MouseLeftDoubleClick(double mx, double my) { } void GraphicsWindow::EditControlDone(const std::string &s) { - //TODO: convert inch expessions back into mm + //TODO: convert inch expessions back into mm window->HideEditor(); window->Invalidate(); @@ -1470,8 +1470,37 @@ void GraphicsWindow::EditControlDone(const std::string &s) { return; } + // decided not to parse equals signs in expressions since + // 1) A relation isn't an expression since it has no meaningful reducable value + // 2) There can only be one occurrence of an assignment in an expression, and this may only be in a relation + // To enforce #2 after future changes and to to not break implicit invariant #1, we process the assignment operation here + // Since "equations" are just expressions = 0, we just "subtract everything after the equals sign from both sides of the equation" + // + // TODO: + // if this doesn't work out, figure out alternative strategy, modify ExprParser::Precendence on line 981, add the operator, and have it basically be a lazy minus + Expr *e = NULL; int usedParams; - Expr *e = Expr::From(s, true, &SK.param, &usedParams); + if(c->type == Constraint::Type::RELATION) { + size_t eqpos = s.find_first_of("="); + if(eqpos != -1 && eqpos != s.find_last_of("=")) { + fprintf(stderr, "reject change 1"); + return; + } + else if(eqpos == 0) { + fprintf(stderr, "reject change2"); + return; + } else if(eqpos == s.length() - 1) { + fprintf(stderr, "reject change3"); + return; + } + + // (left_side) - (right_side) (... implicitly = 0) + e = Expr::From(s.substr(0, eqpos), true, &SK.param, &usedParams)->Minus(Expr::From(s.substr(eqpos+1, SIZE_T_MAX), true, &SK.param, &usedParams)); + + } else { + e = Expr::From(s, true, &SK.param, &usedParams); + } + if(e) { SS.UndoRemember(); @@ -1485,6 +1514,11 @@ void GraphicsWindow::EditControlDone(const std::string &s) { } switch(c->type) { + case Constraint::Type::RELATION: + c->expr_scaling_to_base = 1; + c->expression = s; + break; + case Constraint::Type::PROJ_PT_DISTANCE: case Constraint::Type::PT_LINE_DISTANCE: case Constraint::Type::PT_FACE_DISTANCE: @@ -1505,7 +1539,6 @@ void GraphicsWindow::EditControlDone(const std::string &s) { break; } case Constraint::Type::ANGLE: - case Constraint::Type::RELATION: case Constraint::Type::LENGTH_RATIO: case Constraint::Type::ARC_ARC_LEN_RATIO: case Constraint::Type::ARC_LINE_LEN_RATIO: From ce4712e7367a5b8025ea6f50e7c959b99da13a19 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 6 Apr 2024 13:24:59 -0700 Subject: [PATCH 13/30] relation in toolbar, menu, and keybind --- res/CMakeLists.txt | 1 + res/icons/graphics-window/relation.png | Bin 0 -> 151 bytes src/drawconstraint.cpp | 6 +++--- src/graphicswin.cpp | 2 +- src/toolbar.cpp | 2 ++ 5 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 res/icons/graphics-window/relation.png diff --git a/res/CMakeLists.txt b/res/CMakeLists.txt index 2862b5f41..204a29819 100644 --- a/res/CMakeLists.txt +++ b/res/CMakeLists.txt @@ -255,6 +255,7 @@ add_resources( icons/graphics-window/pointonx.png icons/graphics-window/point.png icons/graphics-window/rectangle.png + icons/graphics-window/relation.png icons/graphics-window/ref.png icons/graphics-window/revolve.png icons/graphics-window/same-orientation.png diff --git a/res/icons/graphics-window/relation.png b/res/icons/graphics-window/relation.png new file mode 100644 index 0000000000000000000000000000000000000000..e60900c1ce1cd4aeebd2cb562e3f89a13e086073 GIT binary patch literal 151 zcmeAS@N?(olHy`uVBq!ia0vp^5+KaM1|%Pp+x`GjoCO|{#S9GG!XV7ZFl!D-1!HlL zyA#8@b22Z19F}xPUq=Rpjs4tz5?O(K9Zwg>kP61P7Z(aLC@>s8Fy(K#Uf Date: Sat, 6 Apr 2024 15:57:42 -0700 Subject: [PATCH 14/30] fix: save expr scaling to base to prevent solver pushing params to zero --- src/constrainteq.cpp | 8 ++++++-- src/file.cpp | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index a04dc245e..34c63b2d4 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -256,9 +256,11 @@ void ConstraintBase::Generate(IdList *l) { size_t eqpos = expression.find_first_of("="); //TODO: validation that only one equals sign appears, or give up on having this pattern plastered everywhere and move into expression parser etc. Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); - } else if(expression != "") { + } else if(expression != "" && expr_scaling_to_base != 0) { //TODO order of ops/move to AST Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, l); + } else if(expression != "") { + Expr::From(expression.c_str(), false, l); } switch(type) { case Type::PARALLEL: @@ -290,9 +292,11 @@ void ConstraintBase::GenerateEquations(IdList *l, size_t eqpos = expression.find_first_of("="); //TODO: validation that only one equals sign appears, or give up on having this pattern plastered everywhere and move into expression parser etc. exA = Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); - } else if(expression != "") { + } else if(expression != "" && expr_scaling_to_base != 0) { //TODO order of ops/move to AST exA = Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, &SK.param, NULL); + } else if(expression != "") { + exA = Expr::From(expression.c_str(), false, &SK.param, NULL); } else { exA = Expr::From(valA); } diff --git a/src/file.cpp b/src/file.cpp index 1b2c245b4..18c006cb1 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -122,6 +122,7 @@ const SolveSpaceUI::SaveTable SolveSpaceUI::SAVED[] = { { 'p', "Param.h.v.", 'x', &(SS.sv.p.h.v) }, { 'p', "Param.val", 'f', &(SS.sv.p.val) }, + { 'p', "Param.name", 'S', &(SS.sv.p.name) }, { 'r', "Request.h.v", 'x', &(SS.sv.r.h.v) }, { 'r', "Request.type", 'd', &(SS.sv.r.type) }, @@ -186,6 +187,7 @@ const SolveSpaceUI::SaveTable SolveSpaceUI::SAVED[] = { { 'c', "Constraint.reference", 'b', &(SS.sv.c.reference) }, { 'c', "Constraint.comment", 'S', &(SS.sv.c.comment) }, { 'c', "Constraint.expression", 'S', &(SS.sv.c.expression) }, + { 'c', "Constraint.expr_scaling_to_base", 'd', &(SS.sv.c.expr_scaling_to_base) }, { 'c', "Constraint.disp.offset.x", 'f', &(SS.sv.c.disp.offset.x) }, { 'c', "Constraint.disp.offset.y", 'f', &(SS.sv.c.disp.offset.y) }, { 'c', "Constraint.disp.offset.z", 'f', &(SS.sv.c.disp.offset.z) }, From 96f500a70c2a7125a9ce1b77219645645439bc33 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 6 Apr 2024 17:57:21 -0700 Subject: [PATCH 15/30] fix: some code cleanup --- src/constrainteq.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 34c63b2d4..1b8312e73 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -252,16 +252,19 @@ void ConstraintBase::AddEq(IdList *l, const ExprVector &v, } void ConstraintBase::Generate(IdList *l) { + // apparently necessary to get params used into l, though we just have to "parse" everything, regardless of whether we use it or not. if(type == Constraint::Type::RELATION) { - size_t eqpos = expression.find_first_of("="); - //TODO: validation that only one equals sign appears, or give up on having this pattern plastered everywhere and move into expression parser etc. - Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); + size_t eqpos = expression.find_first_of("="); + ssassert(eqpos == expression.find_last_of("="), "There is at most one equals sign in the relation \"expression\""); + ssassert(eqpos != std::string::npos, "There is at least one equals sign in the relation \"expression\""); + Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { - //TODO order of ops/move to AST - Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, l); + //TODO order of ops/move to AST + Expr::From(expression.c_str(), false, l, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, l, NULL)); } else if(expression != "") { Expr::From(expression.c_str(), false, l); } + switch(type) { case Type::PARALLEL: case Type::CUBIC_LINE_TANGENT: @@ -294,7 +297,7 @@ void ConstraintBase::GenerateEquations(IdList *l, exA = Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { //TODO order of ops/move to AST - exA = Expr::From((expression+"*"+std::to_string(expr_scaling_to_base)).c_str(), false, &SK.param, NULL); + exA = Expr::From(expression.c_str(), false, &SK.param, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, &SK.param, NULL)); } else if(expression != "") { exA = Expr::From(expression.c_str(), false, &SK.param, NULL); } else { From be693548b4083065bfa12476857a6b0dba861b6a Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sat, 6 Apr 2024 19:18:05 -0700 Subject: [PATCH 16/30] fix savefile format and some zero guards --- src/file.cpp | 2 +- src/mouse.cpp | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/file.cpp b/src/file.cpp index 18c006cb1..ecc568c9a 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -187,7 +187,7 @@ const SolveSpaceUI::SaveTable SolveSpaceUI::SAVED[] = { { 'c', "Constraint.reference", 'b', &(SS.sv.c.reference) }, { 'c', "Constraint.comment", 'S', &(SS.sv.c.comment) }, { 'c', "Constraint.expression", 'S', &(SS.sv.c.expression) }, - { 'c', "Constraint.expr_scaling_to_base", 'd', &(SS.sv.c.expr_scaling_to_base) }, + { 'c', "Constraint.expr_scaling_to_base", 'f', &(SS.sv.c.expr_scaling_to_base) }, { 'c', "Constraint.disp.offset.x", 'f', &(SS.sv.c.disp.offset.x) }, { 'c', "Constraint.disp.offset.y", 'f', &(SS.sv.c.disp.offset.y) }, { 'c', "Constraint.disp.offset.z", 'f', &(SS.sv.c.disp.offset.z) }, diff --git a/src/mouse.cpp b/src/mouse.cpp index 9f6dadb76..9e71ef6bf 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1419,15 +1419,15 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { } else { // Appears to be the wrong approach // TODO: Simplify so that we don't have *25.4/25.4 in a bunch of expressions - if(c->type == Constraint::Type::DIAMETER && c->other) { + if(c->type == Constraint::Type::DIAMETER && c->other && c->expr_scaling_to_base != 0) { // Edit as radius instead of diameter due to user config editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); - } else if(dimless || c->expr_scaling_to_base == SS.MmPerUnit()) { - // Unit does not need scaling - editValue = c->expression; - } else { + } else if(!dimless && c->expr_scaling_to_base != SS.MmPerUnit() && c->expr_scaling_to_base != 0) { // Unit needs dimension scaling editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/SS.MmPerUnit()); + } else { + // Unit does not need scaling + editValue = c->expression; } } From d6cede037f8d2347904468333fa3ccf65d965ce0 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sun, 7 Apr 2024 21:05:35 -0700 Subject: [PATCH 17/30] mark dirty for dof computation --- src/constrainteq.cpp | 2 +- src/mouse.cpp | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 1b8312e73..81c55f19e 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -1088,7 +1088,7 @@ void ConstraintBase::GenerateEquations(IdList *l, case Type::RELATION: std::fprintf(stderr, "adding relation with expr! %s", exA->Print().c_str()); - AddEq(l, exA, 0); //TODO + AddEq(l, exA, 0); return; } diff --git a/src/mouse.cpp b/src/mouse.cpp index 9e71ef6bf..2df4b9ac1 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -4,6 +4,7 @@ // Copyright 2008-2013 Jonathan Westhues. //----------------------------------------------------------------------------- #include "solvespace.h" +#include "ui.h" void GraphicsWindow::UpdateDraggedPoint(hEntity hp, double mx, double my) { Entity *p = SK.GetEntity(hp); @@ -1482,17 +1483,14 @@ void GraphicsWindow::EditControlDone(const std::string &s) { int usedParams; if(c->type == Constraint::Type::RELATION) { size_t eqpos = s.find_first_of("="); - if(eqpos != -1 && eqpos != s.find_last_of("=")) { - fprintf(stderr, "reject change 1"); + if(eqpos == std::string::npos || eqpos != s.find_last_of("=")) { + dbp("Only exactly one equals sing allowed"); return; } else if(eqpos == 0) { - fprintf(stderr, "reject change2"); + dbp("Nothing left on the left-hand side of the = sign"); return; - } else if(eqpos == s.length() - 1) { - fprintf(stderr, "reject change3"); - return; - } + } // (left_side) - (right_side) (... implicitly = 0) e = Expr::From(s.substr(0, eqpos), true, &SK.param, &usedParams)->Minus(Expr::From(s.substr(eqpos+1, SIZE_T_MAX), true, &SK.param, &usedParams)); @@ -1562,7 +1560,9 @@ void GraphicsWindow::EditControlDone(const std::string &s) { c->valA = fabs(SS.ExprToMm(e)); break; } + SK.GetGroup(c->group)->dofCheckOk = false; // if an named param was used in the constraint, the DoF may have changed SS.MarkGroupDirty(c->group); + SS.TW.HideEditControl(); } } From bab8ad2aa7cf6eb99f37f326146d39f86d5445d3 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 11 Apr 2024 20:07:18 -0700 Subject: [PATCH 18/30] several remove/address some todos fix SimplifyInverses, which is still broken and has no way to go back into the expression string --- src/constrainteq.cpp | 3 --- src/drawconstraint.cpp | 2 +- src/expr.cpp | 2 +- src/mouse.cpp | 22 ++++++++++++---------- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 81c55f19e..7f84f3aeb 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -293,10 +293,8 @@ void ConstraintBase::GenerateEquations(IdList *l, } else { if(type == Constraint::Type::RELATION) { size_t eqpos = expression.find_first_of("="); - //TODO: validation that only one equals sign appears, or give up on having this pattern plastered everywhere and move into expression parser etc. exA = Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { - //TODO order of ops/move to AST exA = Expr::From(expression.c_str(), false, &SK.param, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, &SK.param, NULL)); } else if(expression != "") { exA = Expr::From(expression.c_str(), false, &SK.param, NULL); @@ -1087,7 +1085,6 @@ void ConstraintBase::GenerateEquations(IdList *l, return; case Type::RELATION: - std::fprintf(stderr, "adding relation with expr! %s", exA->Print().c_str()); AddEq(l, exA, 0); return; diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index a62e6c019..898065621 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -29,7 +29,7 @@ std::string Constraint::Label() const { if(expression != "") { result = expression ; if(SS.MmPerUnit() != expr_scaling_to_base && expr_scaling_to_base != 0) { - result += "*" + std::to_string(expr_scaling_to_base/SS.MmPerUnit()); + result = "("+result+")" + "*" + std::to_string(expr_scaling_to_base/SS.MmPerUnit()); } } if(reference) { diff --git a/src/expr.cpp b/src/expr.cpp index 6d762a2e2..da343971a 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -525,7 +525,7 @@ Expr* Expr::SimplifyInverses() { if(this->op == Op::TIMES || this->op == Op::DIV) { iter.push(std::make_pair(this,true)); } else { - return nullptr; + return this; } while(!iter.empty()) { diff --git a/src/mouse.cpp b/src/mouse.cpp index 2df4b9ac1..4a2161874 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -928,7 +928,6 @@ bool GraphicsWindow::MouseEvent(Platform::MouseEvent event) { } void GraphicsWindow::MouseLeftDown(double mx, double my, bool shiftDown, bool ctrlDown) { - //TODO: put scaling factor back if abandon editing constraint orig.mouseDown = true; if(window->IsEditorVisible()) { @@ -1422,10 +1421,10 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { // TODO: Simplify so that we don't have *25.4/25.4 in a bunch of expressions if(c->type == Constraint::Type::DIAMETER && c->other && c->expr_scaling_to_base != 0) { // Edit as radius instead of diameter due to user config - editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); + editValue = ssprintf("(%s)*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); } else if(!dimless && c->expr_scaling_to_base != SS.MmPerUnit() && c->expr_scaling_to_base != 0) { // Unit needs dimension scaling - editValue = ssprintf("%s*%f", c->expression.c_str(), c->expr_scaling_to_base/SS.MmPerUnit()); + editValue = ssprintf("(%s)*%f", c->expression.c_str(), c->expr_scaling_to_base/SS.MmPerUnit()); } else { // Unit does not need scaling editValue = c->expression; @@ -1459,7 +1458,6 @@ void GraphicsWindow::MouseLeftDoubleClick(double mx, double my) { } void GraphicsWindow::EditControlDone(const std::string &s) { - //TODO: convert inch expessions back into mm window->HideEditor(); window->Invalidate(); @@ -1470,15 +1468,17 @@ void GraphicsWindow::EditControlDone(const std::string &s) { c->comment = s; return; } + + // after a user finishes editing an expression in a constraint that has a different scaling_to_base, we can assume that they intend for the expression to be in the currently selected units + if(c->expr_scaling_to_base != SS.MmPerUnit()) { + c->expr_scaling_to_base = SS.MmPerUnit(); + } - // decided not to parse equals signs in expressions since + // decided not to parse equals signs in expressions for now since // 1) A relation isn't an expression since it has no meaningful reducable value // 2) There can only be one occurrence of an assignment in an expression, and this may only be in a relation - // To enforce #2 after future changes and to to not break implicit invariant #1, we process the assignment operation here + // To enforce #2 after future changes and to to not break implicit invariant #1, we process the assignment operation as a special case // Since "equations" are just expressions = 0, we just "subtract everything after the equals sign from both sides of the equation" - // - // TODO: - // if this doesn't work out, figure out alternative strategy, modify ExprParser::Precendence on line 981, add the operator, and have it basically be a lazy minus Expr *e = NULL; int usedParams; if(c->type == Constraint::Type::RELATION) { @@ -1508,11 +1508,13 @@ void GraphicsWindow::EditControlDone(const std::string &s) { // Suppose something was designed in inches but edited in mm. The click handler will put the scaling in // The user will choose to leave the scaling alone. So we put the c->expression = s; - e->SimplifyInverses(); + //e = e->SimplifyInverses(); // this causes null deref later + //c->expression = e->Print(); } switch(c->type) { case Constraint::Type::RELATION: + // on relation, expr_scaling_to_base should be ignored, since the scaling is done on constraints that directly constrain some entity c->expr_scaling_to_base = 1; c->expression = s; break; From 7b4225c61ef1e4f637fb6de23888782c2e131108 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 11 Apr 2024 20:12:02 -0700 Subject: [PATCH 19/30] Remove SimplifyInverses and PrintTokens It won't be useful until an overhaul of the expression handling is done, in particular we need to structure is so that expressions can be stably re-serialized in the same infix order from before the expression was parsed --- src/expr.cpp | 338 ++++++++++---------------------------------------- src/expr.h | 1 - src/mouse.cpp | 2 - 3 files changed, 65 insertions(+), 276 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index da343971a..5d7847953 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -441,155 +441,6 @@ bool Expr::IsZeroConst() const { return op == Op::CONSTANT && EXACT(v == 0.0); } - -//TODO naming/scope -void remove_redundant(std::vector& exprs) { - for(long i=0; iv == 1/exprs[k]->v) { //TODO: tol - - exprs[k]->to_delete = true; - exprs.pop_back(); - - exprs[i]->to_delete = true; - exprs.pop_back(); - } - } - } -} - -void remove_inverses(std::vector& exprs1, std::vector& exprs2) { - for(long i=0; iv == exprs2[k]->v) { - exprs1[i]->to_delete = true; - exprs1.pop_back(); - - exprs2[k]->to_delete = true; - exprs2.pop_back(); - } - } - } -} - -//TODO: fix parenthesis edge case: 2*c/(2*3*4) because 2*c/3*4 -void bubble_delete(Expr** parent) { - Expr* a_ptr = nullptr; - bool del_a = false; - Expr* b_ptr = nullptr; - bool del_b = false; - - if((*parent)->a != nullptr) { - bubble_delete(&(*parent)->a); - if(!(*parent)->a->to_delete) { - a_ptr = (*parent)->a; - } else { - del_a = true; - } - } - - // this checks for enum variants that imply the union in Expr has Expr* b - if((uint32_t)(*parent)->op >= 100 && (uint32_t)(*parent)->op <=103) { - bubble_delete(&(*parent)->b); - if(!(*parent)->b->to_delete) { - b_ptr = (*parent)->b; - } else { - del_b = true; - } - } - - if(del_a && del_b) { - (*parent)->to_delete = true; - //TODO: conditionally free a & b - } else if(del_a && b_ptr != nullptr) { //TODO: check if uni-operato - (*parent)->to_delete = true; - (*parent) = b_ptr; - } else if(del_b && a_ptr != nullptr) { - (*parent)->to_delete = true; - (*parent) = a_ptr; - } -} - -// TODO: should this function be made part of FoldConstants? Seems be OK based on how FoldConstants is used -// This routine cancels constants that multiplicative inverses, that act at on the entire expression. -// Contract: this function may cancel ALL multiplicative inverses in the future, regardless of where they appear in tree -Expr* Expr::SimplifyInverses() { - //TODO: include string slices in expr to allow for smart string rebuilding - //intent: be able to switch between inches and mm a few times without getting huge daisy chains - std::vector numerators = std::vector(); - std::vector denominators = std::vector(); - - // bool indicates if we are in the numerator of the expression - std::stack> iter = std::stack>(); - - if(this->op == Op::TIMES || this->op == Op::DIV) { - iter.push(std::make_pair(this,true)); - } else { - return this; - } - - while(!iter.empty()) { - Expr* head = iter.top().first; - bool is_numerator = iter.top().second; - iter.pop(); - - std::vector* local_numerators = is_numerator ? &numerators : &denominators; - std::vector* local_denominators = is_numerator ? &denominators : &numerators; - - switch(head->a->op) { - case Op::CONSTANT: - local_numerators->push_back(head->a); //TODO: this would be an opportune time to handle deletion - - // is there another numerator that is our inverse? - // is there a denominoator that is equal to us? - - // if sibling is to be removed... - - // if removed item is not our sibling... - // delete self, make sibling into parent - break; - case Op::DIV: - case Op::TIMES: - iter.push(std::make_pair(head->a, is_numerator)); - break; - default: - break; - } - - switch(head->b->op) { - case Op::CONSTANT: - if(head->op == Op::DIV) - local_denominators->push_back(head->b); - else - local_numerators->push_back(head->b); - break; - case Op::DIV: - case Op::TIMES: - if(head->op == Op::DIV) - iter.push(std::make_pair(head->b, !is_numerator)); - else - iter.push(std::make_pair(head->b, is_numerator)); - - break; - default: - break; - } - } - - remove_redundant(numerators); - remove_redundant(denominators); - remove_inverses(numerators,denominators); - - Expr* new_expr = this; - bubble_delete(&new_expr); - - // delete constants in the tree (leaf nodes). - // All deleted constants have a parent that is TIMES or DIV that their non-deleted siblings should replace - // TODO: what if sibling is also deleted? - - return new_expr; -} - Expr *Expr::FoldConstants() { Expr *n = AllocExpr(); *n = *this; @@ -766,14 +617,13 @@ class ExprParser { TokenType type; Expr *expr; - static Token* From(TokenType type = TokenType::ERROR, Expr *expr = NULL); - static Token* From(TokenType type, Expr::Op op); + static Token From(TokenType type = TokenType::ERROR, Expr *expr = NULL); + static Token From(TokenType type, Expr::Op op); bool IsError() const { return type == TokenType::ERROR; } }; std::string::const_iterator it, end; - std::vector stack; - std::vector tokens; //TODO: free tokens when done - problem: this vector reallocates and the pointers all become trash. Use a destructor? + std::vector stack; std::set newParams; IdList *params; hConstraint hc; @@ -784,34 +634,33 @@ class ExprParser { std::string ReadWord(); void SkipSpace(); - Token* PopOperator(std::string *error); - Token* PopOperand(std::string *error); + Token PopOperator(std::string *error); + Token PopOperand(std::string *error); - int Precedence(Token* token); - Token* LexNumber(std::string *error); - Token* Lex(std::string *error); + int Precedence(Token token); + Token LexNumber(std::string *error); + Token Lex(std::string *error); bool Reduce(std::string *error); bool Parse(std::string *error, size_t reduceUntil = 0); - void PrintTokens(); static Expr *Parse(const std::string &input, std::string *error, IdList *params = NULL, int *paramsCount = 0, hConstraint hc = {0}); }; // allocates -ExprParser::Token* ExprParser::Token::From(TokenType type, Expr *expr) { - Token* t = new Token(); - t->type = type; - t->expr = expr; +ExprParser::Token ExprParser::Token::From(TokenType type, Expr *expr) { + Token t; + t.type = type; + t.expr = expr; return t; } // allocates -ExprParser::Token* ExprParser::Token::From(TokenType type, Expr::Op op) { - Token* t = new Token(); - t->type = type; - t->expr = Expr::AllocExpr(); - t->expr->op = op; +ExprParser::Token ExprParser::Token::From(TokenType type, Expr::Op op) { + Token t; + t.type = type; + t.expr = Expr::AllocExpr(); + t.expr->op = op; return t; } @@ -845,7 +694,7 @@ void ExprParser::SkipSpace() { } } -ExprParser::Token* ExprParser::LexNumber(std::string *error) { +ExprParser::Token ExprParser::LexNumber(std::string *error) { std::string s; while(char c = PeekChar()) { @@ -860,20 +709,20 @@ ExprParser::Token* ExprParser::LexNumber(std::string *error) { char *endptr; double d = strtod(s.c_str(), &endptr); - Token* t = Token::From(); + Token t = Token::From(); if(endptr == s.c_str() + s.size()) { t = Token::From(TokenType::OPERAND, Expr::Op::CONSTANT); - t->expr->v = d; + t.expr->v = d; } else { *error = "'" + s + "' is not a valid number"; } return t; } -ExprParser::Token* ExprParser::Lex(std::string *error) { +ExprParser::Token ExprParser::Lex(std::string *error) { SkipSpace(); - Token* t = nullptr; + Token t; char c = PeekChar(); if(isupper(c)) { std::string n = ReadWord(); @@ -894,13 +743,13 @@ ExprParser::Token* ExprParser::Lex(std::string *error) { t = Token::From(TokenType::UNARY_OP, Expr::Op::ACOS); } else if(s == "pi") { t = Token::From(TokenType::OPERAND, Expr::Op::CONSTANT); - t->expr->v = PI; + t.expr->v = PI; } else if(params != NULL) { bool found = false; for(const Param &p : *params) { if(p.name != s) continue; t = Token::From(TokenType::OPERAND, Expr::Op::PARAM); - t->expr->parh = p.h; + t.expr->parh = p.h; newParams.insert(p.h.v); found = true; } @@ -917,7 +766,7 @@ ExprParser::Token* ExprParser::Lex(std::string *error) { params->Add(&p); newParams.insert(p.h.v); t = Token::From(TokenType::OPERAND, Expr::Op::PARAM); - t->expr->parh = p.h; + t.expr->parh = p.h; } } else { *error = "'" + s + "' is not a valid variable, function or constant"; @@ -950,12 +799,12 @@ ExprParser::Token* ExprParser::Lex(std::string *error) { return t; } -ExprParser::Token* ExprParser::PopOperand(std::string *error) { - Token* t = nullptr; +ExprParser::Token ExprParser::PopOperand(std::string *error) { + Token t; if(stack.empty() || ( - stack.back()->type != TokenType::OPERAND - && (stack.back()->expr == nullptr) + stack.back().type != TokenType::OPERAND + && (stack.back().expr == nullptr) ) ) { *error = "Expected an operand"; @@ -966,10 +815,10 @@ ExprParser::Token* ExprParser::PopOperand(std::string *error) { return t; } -ExprParser::Token* ExprParser::PopOperator(std::string *error) { - Token* t = nullptr; - if(stack.empty() || (stack.back()->type != TokenType::UNARY_OP && - stack.back()->type != TokenType::BINARY_OP)) { +ExprParser::Token ExprParser::PopOperator(std::string *error) { + Token t; + if(stack.empty() || (stack.back().type != TokenType::UNARY_OP && + stack.back().type != TokenType::BINARY_OP)) { *error = "Expected an operator"; } else { t = stack.back(); @@ -978,47 +827,47 @@ ExprParser::Token* ExprParser::PopOperator(std::string *error) { return t; } -int ExprParser::Precedence(Token* t) { - ssassert(t->type == TokenType::BINARY_OP || - t->type == TokenType::UNARY_OP || - t->type == TokenType::OPERAND, +int ExprParser::Precedence(Token t) { + ssassert(t.type == TokenType::BINARY_OP || + t.type == TokenType::UNARY_OP || + t.type == TokenType::OPERAND, "Unexpected token type"); - if(t->type == TokenType::UNARY_OP) { + if(t.type == TokenType::UNARY_OP) { return 30; - } else if(t->expr->op == Expr::Op::TIMES || - t->expr->op == Expr::Op::DIV) { + } else if(t.expr->op == Expr::Op::TIMES || + t.expr->op == Expr::Op::DIV) { return 20; - } else if(t->expr->op == Expr::Op::PLUS || - t->expr->op == Expr::Op::MINUS) { + } else if(t.expr->op == Expr::Op::PLUS || + t.expr->op == Expr::Op::MINUS) { return 10; - } else if(t->type == TokenType::OPERAND) { + } else if(t.type == TokenType::OPERAND) { return 0; } else ssassert(false, "Unexpected operator"); } bool ExprParser::Reduce(std::string *error) { - Token* a = PopOperand(error); - if(a->IsError()) return false; + Token a = PopOperand(error); + if(a.IsError()) return false; - Token* op = PopOperator(error); - if(op->IsError()) return false; + Token op = PopOperator(error); + if(op.IsError()) return false; - switch(op->type) { + switch(op.type) { case TokenType::BINARY_OP: { - Token* b = PopOperand(error); - if(b->IsError()) return false; + Token b = PopOperand(error); + if(b.IsError()) return false; // gives the operand children: // semantically this subtree represents an operand, so we change the token type accordingly - op->expr = b->expr->AnyOp(op->expr->op, a->expr); + op.expr = b.expr->AnyOp(op.expr->op, a.expr); stack.push_back(op); break; } case TokenType::UNARY_OP: { - Expr *e = a->expr; - switch(op->expr->op) { + Expr *e = a.expr; + switch(op.expr->op) { case Expr::Op::NEGATE: e = e->Negate(); break; case Expr::Op::SQRT: e = e->Sqrt(); break; case Expr::Op::SQUARE: e = e->Times(e); break; @@ -1028,7 +877,7 @@ bool ExprParser::Reduce(std::string *error) { case Expr::Op::ACOS: e = e->ACos()->Times(Expr::From(180/PI)); break; default: ssassert(false, "Unexpected unary operator"); } - op->expr = e; + op.expr = e; stack.push_back(op); break; } @@ -1041,12 +890,11 @@ bool ExprParser::Reduce(std::string *error) { bool ExprParser::Parse(std::string *error, size_t reduceUntil) { while(true) { - Token* t = Lex(error); + Token t = Lex(error); if(error != NULL && error->length() != 0) { printf("Error %s", error); } - tokens.push_back(t); - switch(t->type) { + switch(t.type) { case TokenType::ERROR: return false; @@ -1056,7 +904,7 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { if(!Reduce(error)) return false; } - if(t->type == TokenType::PAREN_RIGHT) { + if(t.type == TokenType::PAREN_RIGHT) { stack.push_back(t); } return true; @@ -1065,7 +913,7 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { // sub-expression if(!Parse(error, /*reduceUntil=*/stack.size())) return false; - if(stack.empty() || stack.back()->type != TokenType::PAREN_RIGHT) { + if(stack.empty() || stack.back().type != TokenType::PAREN_RIGHT) { *error = "Expected ')'"; return false; } @@ -1074,11 +922,11 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { } case TokenType::BINARY_OP: - if((stack.size() > reduceUntil && stack.back()->type != TokenType::OPERAND) || + if((stack.size() > reduceUntil && stack.back().type != TokenType::OPERAND) || stack.size() == reduceUntil) { - if(t->expr->op == Expr::Op::MINUS) { - t->type = TokenType::UNARY_OP; - t->expr->op = Expr::Op::NEGATE; + if(t.expr->op == Expr::Op::MINUS) { + t.type = TokenType::UNARY_OP; + t.expr->op = Expr::Op::NEGATE; stack.push_back(t); break; } @@ -1102,58 +950,6 @@ bool ExprParser::Parse(std::string *error, size_t reduceUntil) { return true; } -//TODO: clean this up, make this auto edit the original string, fix auto-added scale factors in the expressions in tokens so they print properly or are fully suppressed depending on mode -void ExprParser::PrintTokens() { - std::string* str = new std::string(); - - for(ExprParser::Token* token : tokens) { - - if(token->type == ExprParser::TokenType::PAREN_LEFT) { - str->append("("); - continue; - } else if(token->type == ExprParser::TokenType::PAREN_RIGHT) { - str->append(")"); - continue; - } else if(token->expr != nullptr) { - if(token->expr->to_delete) { - continue; - } - - - switch(token->expr->op) { - case Expr::Op::PARAM: - str->append(this->params->FindById(token->expr->parh)->name); - break; - case Expr::Op::PARAM_PTR: - case Expr::Op::VARIABLE: - str->append("?param"); - break; - case Expr::Op::CONSTANT: - str->append(std::to_string(token->expr->v)); - break; - case Expr::Op::PLUS: - str->append("+"); - break; - case Expr::Op::MINUS: - str->append("-"); - break; - case Expr::Op::TIMES: - str->append("*"); - break; - case Expr::Op::DIV: - str->append("/"); - break; - //TODO: UNARY OPS - default: - str->append("?"); - } - } - } - - dbp("%s", str->c_str()); - delete str; -} - Expr *ExprParser::Parse(const std::string &input, std::string *error, IdList *params, int *paramsCount, hConstraint hc) { ExprParser parser; @@ -1164,15 +960,11 @@ Expr *ExprParser::Parse(const std::string &input, std::string *error, parser.hc = hc; if(!parser.Parse(error)) return NULL; - Token* r = parser.PopOperand(error); - if(r->IsError()) return NULL; + Token r = parser.PopOperand(error); + if(r.IsError()) return NULL; if(paramsCount != NULL) *paramsCount = parser.newParams.size(); - r->expr->SimplifyInverses(); - - parser.PrintTokens(); - - return r->expr; + return r.expr; } Expr *Expr::Parse(const std::string &input, std::string *error) { diff --git a/src/expr.h b/src/expr.h index 364827823..3837c8982 100644 --- a/src/expr.h +++ b/src/expr.h @@ -77,7 +77,6 @@ class Expr { bool DependsOn(hParam p) const; static bool Tol(double a, double b); bool IsZeroConst() const; - Expr *SimplifyInverses(); Expr *FoldConstants(); void Substitute(hParam oldh, hParam newh); diff --git a/src/mouse.cpp b/src/mouse.cpp index 4a2161874..b0d3a5f8f 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1508,8 +1508,6 @@ void GraphicsWindow::EditControlDone(const std::string &s) { // Suppose something was designed in inches but edited in mm. The click handler will put the scaling in // The user will choose to leave the scaling alone. So we put the c->expression = s; - //e = e->SimplifyInverses(); // this causes null deref later - //c->expression = e->Print(); } switch(c->type) { From faf4423d25b054a836004416a1577d7938a322da Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 11 Apr 2024 20:39:19 -0700 Subject: [PATCH 20/30] chore: address several todos --- src/constrainteq.cpp | 1 - src/mouse.cpp | 12 +++++------- src/solvespace.cpp | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 7f84f3aeb..a15897333 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -259,7 +259,6 @@ void ConstraintBase::Generate(IdList *l) { ssassert(eqpos != std::string::npos, "There is at least one equals sign in the relation \"expression\""); Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { - //TODO order of ops/move to AST Expr::From(expression.c_str(), false, l, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, l, NULL)); } else if(expression != "") { Expr::From(expression.c_str(), false, l); diff --git a/src/mouse.cpp b/src/mouse.cpp index b0d3a5f8f..1be3f0453 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1417,8 +1417,6 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { digits++; } } else { - // Appears to be the wrong approach - // TODO: Simplify so that we don't have *25.4/25.4 in a bunch of expressions if(c->type == Constraint::Type::DIAMETER && c->other && c->expr_scaling_to_base != 0) { // Edit as radius instead of diameter due to user config editValue = ssprintf("(%s)*%f", c->expression.c_str(), c->expr_scaling_to_base/2*SS.MmPerUnit()); @@ -1484,11 +1482,14 @@ void GraphicsWindow::EditControlDone(const std::string &s) { if(c->type == Constraint::Type::RELATION) { size_t eqpos = s.find_first_of("="); if(eqpos == std::string::npos || eqpos != s.find_last_of("=")) { - dbp("Only exactly one equals sing allowed"); + Error("Relation constraints must have exactly one '=': '%s'", s.c_str()); return; } else if(eqpos == 0) { - dbp("Nothing left on the left-hand side of the = sign"); + Error("Relation constraints must have a left-hand-side (\?\?%s)", s.c_str()); + return; + } else if(eqpos == s.length() - 1) { + Error("Relation constraints must have a right-hand-side (%s\?\?)", s.c_str()); return; } @@ -1503,10 +1504,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { if(e) { SS.UndoRemember(); if(usedParams > 0) { - // TODO: special cases for dimless, angles c->expr_scaling_to_base = SS.MmPerUnit(); - // Suppose something was designed in inches but edited in mm. The click handler will put the scaling in - // The user will choose to leave the scaling alone. So we put the c->expression = s; } diff --git a/src/solvespace.cpp b/src/solvespace.cpp index 7d2087ce3..7be4a6f1e 100644 --- a/src/solvespace.cpp +++ b/src/solvespace.cpp @@ -482,7 +482,7 @@ std::string SolveSpaceUI::DegreeToString(double v) { } } double SolveSpaceUI::ExprToMm(Expr *e) { - //TODO: This function is wrong for constraints, remove all uses of it + //TODO: This function is wrong for constraints, since constraints with expressions may have their own scaling to base (for example since they were specified in a different system) return (e->Eval()) * MmPerUnit(); } double SolveSpaceUI::StringToMm(const std::string &str) { From f2db469468f7520229185a62631f5911708b80cd Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 11 Apr 2024 20:55:02 -0700 Subject: [PATCH 21/30] protect invariant for constraint expr evaluation semantics --- src/confscreen.cpp | 12 ++++++------ src/mouse.cpp | 4 ++-- src/solvespace.cpp | 5 +++-- src/solvespace.h | 2 +- src/textscreens.cpp | 4 ++-- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/confscreen.cpp b/src/confscreen.cpp index b4be1f82a..7141b4e5d 100644 --- a/src/confscreen.cpp +++ b/src/confscreen.cpp @@ -484,7 +484,7 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { case Edit::EXPORT_OFFSET: { Expr *e = Expr::From(s, /*popUpError=*/true); if(e) { - double ev = SS.ExprToMm(e); + double ev = SS.NonConstraintExprToMm(e); if(IsReasonable(ev) || ev < 0) { Error(_("Cutter radius offset must not be negative!")); } else { @@ -498,7 +498,7 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { if(!e) { break; } - float d = (float)SS.ExprToMm(e); + float d = (float)SS.NonConstraintExprToMm(e); switch(edit.i) { case 0: SS.exportMargin.left = d; break; case 1: SS.exportMargin.right = d; break; @@ -514,12 +514,12 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { } case Edit::G_CODE_DEPTH: { Expr *e = Expr::From(s, /*popUpError=*/true); - if(e) SS.gCode.depth = (float)SS.ExprToMm(e); + if(e) SS.gCode.depth = (float)SS.NonConstraintExprToMm(e); break; } case Edit::G_CODE_SAFE_HEIGHT: { Expr *e = Expr::From(s, /*popUpError=*/true); - if(e) SS.gCode.safeHeight = (float)SS.ExprToMm(e); + if(e) SS.gCode.safeHeight = (float)SS.NonConstraintExprToMm(e); break; } case Edit::G_CODE_PASSES: { @@ -530,12 +530,12 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { } case Edit::G_CODE_FEED: { Expr *e = Expr::From(s, /*popUpError=*/true); - if(e) SS.gCode.feed = (float)SS.ExprToMm(e); + if(e) SS.gCode.feed = (float)SS.NonConstraintExprToMm(e); break; } case Edit::G_CODE_PLUNGE_FEED: { Expr *e = Expr::From(s, /*popUpError=*/true); - if(e) SS.gCode.plungeFeed = (float)SS.ExprToMm(e); + if(e) SS.gCode.plungeFeed = (float)SS.NonConstraintExprToMm(e); break; } case Edit::AUTOSAVE_INTERVAL: { diff --git a/src/mouse.cpp b/src/mouse.cpp index 1be3f0453..7cdde34a3 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1554,8 +1554,8 @@ void GraphicsWindow::EditControlDone(const std::string &s) { default: // These are always positive, and they get the units conversion. - // Use ExprToMm since this unparameterized expressions simplify down - c->valA = fabs(SS.ExprToMm(e)); + // Use ExprToMm since this unparameterized expression will simplify immediately, and the dimension transformation is irrelevant + c->valA = fabs(SS.NonConstraintExprToMm(e)); break; } SK.GetGroup(c->group)->dofCheckOk = false; // if an named param was used in the constraint, the DoF may have changed diff --git a/src/solvespace.cpp b/src/solvespace.cpp index 7be4a6f1e..849f7cd5d 100644 --- a/src/solvespace.cpp +++ b/src/solvespace.cpp @@ -481,8 +481,9 @@ std::string SolveSpaceUI::DegreeToString(double v) { return ssprintf("%.0f", v); } } -double SolveSpaceUI::ExprToMm(Expr *e) { - //TODO: This function is wrong for constraints, since constraints with expressions may have their own scaling to base (for example since they were specified in a different system) + +// Invariant: This function is wrong for constraints, since constraints with expressions may have their own scaling to base (for example since they were specified in a different system) +double SolveSpaceUI::NonConstraintExprToMm(Expr *e) { return (e->Eval()) * MmPerUnit(); } double SolveSpaceUI::StringToMm(const std::string &str) { diff --git a/src/solvespace.h b/src/solvespace.h index 6c3daa737..b306b10ea 100644 --- a/src/solvespace.h +++ b/src/solvespace.h @@ -623,7 +623,7 @@ class SolveSpaceUI { std::string MmToString(double v, bool editable=false); std::string MmToStringSI(double v, int dim = 0); std::string DegreeToString(double v); - double ExprToMm(Expr *e); + double NonConstraintExprToMm(Expr *e); double StringToMm(const std::string &s); const char *UnitName(); double MmPerUnit(); diff --git a/src/textscreens.cpp b/src/textscreens.cpp index c73361f92..66e7675ad 100644 --- a/src/textscreens.cpp +++ b/src/textscreens.cpp @@ -889,7 +889,7 @@ void TextWindow::EditControlDone(std::string s) { case Edit::STEP_DIM_FINISH: if(Expr *e = Expr::From(s, /*popUpError=*/true)) { if(stepDim.isDistance) { - stepDim.finish = SS.ExprToMm(e); + stepDim.finish = SS.NonConstraintExprToMm(e); } else { stepDim.finish = e->Eval(); } @@ -906,7 +906,7 @@ void TextWindow::EditControlDone(std::string s) { Error(_("Radius cannot be zero or negative.")); break; } - SS.tangentArcRadius = SS.ExprToMm(e); + SS.tangentArcRadius = SS.NonConstraintExprToMm(e); } break; From 64480859564444d50a589eeb38d096cd5fc72394 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 11 Apr 2024 21:25:19 -0700 Subject: [PATCH 22/30] feat: multiplication shorthand + eager error checks --- src/expr.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/expr.cpp b/src/expr.cpp index 5d7847953..c7183d39e 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -848,14 +848,25 @@ int ExprParser::Precedence(Token t) { bool ExprParser::Reduce(std::string *error) { Token a = PopOperand(error); + if(error != NULL && !error->empty()) return false; if(a.IsError()) return false; Token op = PopOperator(error); + + // support for multiplicaiton shorthand (i.e. 2x instead of 2*x) + if(op.IsError()) { + op = Token::From(TokenType::BINARY_OP, Expr::Op::TIMES); + error = NULL; + } + //redundant in current setup, since if there's an error op is likely to be null. keeping to preseve error handing in future changes + else if(error != NULL && !error->empty()) return false; + if(op.IsError()) return false; switch(op.type) { case TokenType::BINARY_OP: { Token b = PopOperand(error); + if(error != NULL && !error->empty()) return false; if(b.IsError()) return false; // gives the operand children: From ea489ea55022f4f6c2d1783370272128dd09e664 Mon Sep 17 00:00:00 2001 From: ruevs Date: Mon, 15 Apr 2024 16:56:03 +0300 Subject: [PATCH 23/30] Small clean up to align with dgramop/parameters --- CMakeLists.txt | 4 +--- cmake/c_flag_overrides.cmake | 2 -- cmake/cxx_flag_overrides.cmake | 2 -- src/constraint.cpp | 2 +- src/constrainteq.cpp | 2 -- src/expr.cpp | 9 +-------- src/expr.h | 3 --- src/mouse.cpp | 6 +++--- 8 files changed, 6 insertions(+), 24 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3f8c2acdd..5a012553f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -309,9 +309,7 @@ if(ENABLE_GUI) "${CMAKE_SOURCE_DIR}/extlib/si/siapp.lib") endif() elseif(APPLE) - #find_package(OpenGL REQUIRED) - set(CMAKE_C_FLAGS "-framework OpenGL ${CMAKE_C_FLAGS}") - set(CMAKE_CXX_FLAGS "-framework OpenGL ${CMAKE_CXX_FLAGS}") + find_package(OpenGL REQUIRED) find_library(APPKIT_LIBRARY AppKit REQUIRED) elseif(EMSCRIPTEN) # Everything is built in diff --git a/cmake/c_flag_overrides.cmake b/cmake/c_flag_overrides.cmake index 014d7ada7..978b6b659 100644 --- a/cmake/c_flag_overrides.cmake +++ b/cmake/c_flag_overrides.cmake @@ -8,5 +8,3 @@ endif() if(EMSCRIPTEN) set(CMAKE_C_FLAGS_DEBUG_INIT "-g4") endif() - -set(CMAKE_C_FLAGS_DEBUG_INIT "-g") diff --git a/cmake/cxx_flag_overrides.cmake b/cmake/cxx_flag_overrides.cmake index ec43ae189..9c8d15fe6 100644 --- a/cmake/cxx_flag_overrides.cmake +++ b/cmake/cxx_flag_overrides.cmake @@ -8,5 +8,3 @@ endif() if(EMSCRIPTEN) set(CMAKE_CXX_FLAGS_DEBUG_INIT "-g4") endif() - -set(CMAKE_CXX_FLAGS_DEBUG_INIT "-g") diff --git a/src/constraint.cpp b/src/constraint.cpp index 7d96b8705..31cbca228 100644 --- a/src/constraint.cpp +++ b/src/constraint.cpp @@ -902,7 +902,7 @@ void Constraint::MenuConstrain(Command id) { } else { SS.GW.pending.operation = GraphicsWindow::Pending::COMMAND; SS.GW.pending.command = Command::RELATION; - SS.GW.pending.description = _("click center of comment text"); + SS.GW.pending.description = _("click center of relation text"); SS.ScheduleShowTW(); } break; diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index a15897333..0622808a2 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -6,8 +6,6 @@ // Copyright 2008-2013 Jonathan Westhues. //----------------------------------------------------------------------------- #include "solvespace.h" -#include -#include const hConstraint ConstraintBase::NO_CONSTRAINT = { 0 }; diff --git a/src/expr.cpp b/src/expr.cpp index c7183d39e..63c61df41 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -7,9 +7,6 @@ // Copyright 2008-2013 Jonathan Westhues. //----------------------------------------------------------------------------- #include "solvespace.h" -#include -#include -#include ExprVector ExprVector::From(Expr *x, Expr *y, Expr *z) { ExprVector r = { x, y, z}; @@ -722,7 +719,7 @@ ExprParser::Token ExprParser::LexNumber(std::string *error) { ExprParser::Token ExprParser::Lex(std::string *error) { SkipSpace(); - Token t; + Token t = Token::From(); char c = PeekChar(); if(isupper(c)) { std::string n = ReadWord(); @@ -902,9 +899,6 @@ bool ExprParser::Reduce(std::string *error) { bool ExprParser::Parse(std::string *error, size_t reduceUntil) { while(true) { Token t = Lex(error); - if(error != NULL && error->length() != 0) { - printf("Error %s", error); - } switch(t.type) { case TokenType::ERROR: return false; @@ -974,7 +968,6 @@ Expr *ExprParser::Parse(const std::string &input, std::string *error, Token r = parser.PopOperand(error); if(r.IsError()) return NULL; if(paramsCount != NULL) *paramsCount = parser.newParams.size(); - return r.expr; } diff --git a/src/expr.h b/src/expr.h index 3837c8982..609850638 100644 --- a/src/expr.h +++ b/src/expr.h @@ -26,7 +26,6 @@ class Expr { MINUS = 101, TIMES = 102, DIV = 103, - // Unary ops NEGATE = 104, SQRT = 105, @@ -46,8 +45,6 @@ class Expr { Expr *b; // nullptr if this is a unary op, }; - bool to_delete; - Expr() = default; Expr(double val) : op(Op::CONSTANT) { v = val; } diff --git a/src/mouse.cpp b/src/mouse.cpp index 7cdde34a3..5242501ba 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1365,7 +1365,7 @@ void GraphicsWindow::EditConstraint(hConstraint constraint) { Constraint *c = SK.GetConstraint(constraintBeingEdited); if(!c->HasLabel()) { // Not meaningful to edit a constraint without a dimension - fprintf(stderr, "Wont edit without label"); + dbp("Wont edit without label"); return; } if(c->reference) { @@ -1528,9 +1528,9 @@ void GraphicsWindow::EditControlDone(const std::string &s) { // negative distance. bool wasNeg = (c->valA < 0); if(wasNeg) { - c->valA = -(e->Eval()); + c->valA = -(e->Eval()) * c->expr_scaling_to_base; } else { - c->valA = (e->Eval()); + c->valA = (e->Eval()) * c->expr_scaling_to_base; } break; } From 1f83cd8065929d3b43de8ab3bd0526aed66ad0cb Mon Sep 17 00:00:00 2001 From: ruevs Date: Mon, 15 Apr 2024 17:48:56 +0300 Subject: [PATCH 24/30] SIZE_MAX instead of "SIZE_T_MAX"... ... which is not standard and does not exist in Visual C++. --- src/constrainteq.cpp | 4 ++-- src/mouse.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 0622808a2..8cea1508e 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -255,7 +255,7 @@ void ConstraintBase::Generate(IdList *l) { size_t eqpos = expression.find_first_of("="); ssassert(eqpos == expression.find_last_of("="), "There is at most one equals sign in the relation \"expression\""); ssassert(eqpos != std::string::npos, "There is at least one equals sign in the relation \"expression\""); - Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); + Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_MAX), false, &SK.param, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { Expr::From(expression.c_str(), false, l, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, l, NULL)); } else if(expression != "") { @@ -290,7 +290,7 @@ void ConstraintBase::GenerateEquations(IdList *l, } else { if(type == Constraint::Type::RELATION) { size_t eqpos = expression.find_first_of("="); - exA = Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_T_MAX), false, &SK.param, NULL)); + exA = Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_MAX), false, &SK.param, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { exA = Expr::From(expression.c_str(), false, &SK.param, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, &SK.param, NULL)); } else if(expression != "") { diff --git a/src/mouse.cpp b/src/mouse.cpp index 5242501ba..f0d4e516d 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1494,7 +1494,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { } // (left_side) - (right_side) (... implicitly = 0) - e = Expr::From(s.substr(0, eqpos), true, &SK.param, &usedParams)->Minus(Expr::From(s.substr(eqpos+1, SIZE_T_MAX), true, &SK.param, &usedParams)); + e = Expr::From(s.substr(0, eqpos), true, &SK.param, &usedParams)->Minus(Expr::From(s.substr(eqpos+1, SIZE_MAX), true, &SK.param, &usedParams)); } else { e = Expr::From(s, true, &SK.param, &usedParams); From 15c221a98ecf09466def43b0c7f99a77e36ee3b3 Mon Sep 17 00:00:00 2001 From: ruevs Date: Mon, 15 Apr 2024 18:38:47 +0300 Subject: [PATCH 25/30] UI: Adjust the toolbar height to accommodate the new "relation" icon. --- src/drawentity.cpp | 2 +- src/graphicswin.cpp | 2 +- src/toolbar.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/drawentity.cpp b/src/drawentity.cpp index 7ac03386d..56abc34e7 100644 --- a/src/drawentity.cpp +++ b/src/drawentity.cpp @@ -661,7 +661,7 @@ void Entity::Draw(DrawAs how, Canvas *canvas) { double w = 60 - camera.width / 2.0; // Shift the axis to the right if they would overlap with the toolbar. if(SS.showToolbar) { - if(h + 30 > -(32*18 + 3*16 + 8) / 2) + if(h + 30 > -(32*19 + 3*16 + 8) / 2) w += 60; } tail = camera.projRight.ScaledBy(w/s).Plus( diff --git a/src/graphicswin.cpp b/src/graphicswin.cpp index 7f793d163..4deb269bc 100644 --- a/src/graphicswin.cpp +++ b/src/graphicswin.cpp @@ -432,7 +432,7 @@ void GraphicsWindow::Init() { using namespace std::placeholders; // Do this first, so that if it causes an onRender event we don't try to paint without // a canvas. - window->SetMinContentSize(720, /*ToolbarDrawOrHitTest 636*/ 32 * 18 + 3 * 16 + 8 + 4); + window->SetMinContentSize(720, /*ToolbarDrawOrHitTest 636*/ 32 * 19 + 3 * 16 + 8 + 4); window->onClose = std::bind(&SolveSpaceUI::MenuFile, Command::EXIT); window->onContextLost = [&] { canvas = NULL; diff --git a/src/toolbar.cpp b/src/toolbar.cpp index 6152a541d..0e5187bba 100644 --- a/src/toolbar.cpp +++ b/src/toolbar.cpp @@ -160,7 +160,7 @@ bool GraphicsWindow::ToolbarDrawOrHitTest(int mx, int my, UiCanvas *canvas, // When changing these values, also change the asReference drawing code in drawentity.cpp // as well as the "window->SetMinContentSize(720, 636);" in graphicswin.cpp int fudge = 8; - int h = 32*18 + 3*16 + fudge; // Toolbar height = 18 icons * 32 pixels + 3 dividers * 16 pixels + fudge + int h = 32*19 + 3*16 + fudge; // Toolbar height = 19 icons * 32 pixels + 3 dividers * 16 pixels + fudge if(h < y) { // If there is enough vertical space leave up to 32 pixels between the menu bar and the toolbar. From 296eff8a76ae64de25149d9bfd3a554593dd043f Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Tue, 16 Apr 2024 23:24:09 -0700 Subject: [PATCH 26/30] fix: allow underscores inside named parameters --- src/expr.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/expr.cpp b/src/expr.cpp index 63c61df41..2a1873bf2 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -677,7 +677,7 @@ std::string ExprParser::ReadWord() { std::string s; while(char c = PeekChar()) { - if(!isalnum(c)) break; + if(!isalnum(c) && c != '_') break; s.push_back(ReadChar()); } From 98ee73197fd01eb574088b352ab961aeb867ca23 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Sun, 2 Jun 2024 19:51:36 -0700 Subject: [PATCH 27/30] fix: address review items --- src/drawconstraint.cpp | 4 ++-- src/expr.cpp | 8 +++----- src/mouse.cpp | 7 +------ 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index 898065621..a736d486a 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -27,8 +27,8 @@ std::string Constraint::Label() const { result = SS.MmToStringSI(fabs(valA)); } if(expression != "") { - result = expression ; - if(SS.MmPerUnit() != expr_scaling_to_base && expr_scaling_to_base != 0) { + result = expression; + if((SS.MmPerUnit() != expr_scaling_to_base) && (expr_scaling_to_base != 0)) { result = "("+result+")" + "*" + std::to_string(expr_scaling_to_base/SS.MmPerUnit()); } } diff --git a/src/expr.cpp b/src/expr.cpp index 2a1873bf2..27761bb7f 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -644,7 +644,6 @@ class ExprParser { hParam> *params = NULL, int *paramsCount = 0, hConstraint hc = {0}); }; -// allocates ExprParser::Token ExprParser::Token::From(TokenType type, Expr *expr) { Token t; t.type = type; @@ -652,7 +651,6 @@ ExprParser::Token ExprParser::Token::From(TokenType type, Expr *expr) { return t; } -// allocates ExprParser::Token ExprParser::Token::From(TokenType type, Expr::Op op) { Token t; t.type = type; @@ -845,7 +843,7 @@ int ExprParser::Precedence(Token t) { bool ExprParser::Reduce(std::string *error) { Token a = PopOperand(error); - if(error != NULL && !error->empty()) return false; + if((error != NULL) && (!error->empty())) return false; if(a.IsError()) return false; Token op = PopOperator(error); @@ -856,14 +854,14 @@ bool ExprParser::Reduce(std::string *error) { error = NULL; } //redundant in current setup, since if there's an error op is likely to be null. keeping to preseve error handing in future changes - else if(error != NULL && !error->empty()) return false; + else if((error != NULL) && (!error->empty())) return false; if(op.IsError()) return false; switch(op.type) { case TokenType::BINARY_OP: { Token b = PopOperand(error); - if(error != NULL && !error->empty()) return false; + if((error != NULL) && (!error->empty())) return false; if(b.IsError()) return false; // gives the operand children: diff --git a/src/mouse.cpp b/src/mouse.cpp index f0d4e516d..c04106a7d 100644 --- a/src/mouse.cpp +++ b/src/mouse.cpp @@ -1467,11 +1467,6 @@ void GraphicsWindow::EditControlDone(const std::string &s) { return; } - // after a user finishes editing an expression in a constraint that has a different scaling_to_base, we can assume that they intend for the expression to be in the currently selected units - if(c->expr_scaling_to_base != SS.MmPerUnit()) { - c->expr_scaling_to_base = SS.MmPerUnit(); - } - // decided not to parse equals signs in expressions for now since // 1) A relation isn't an expression since it has no meaningful reducable value // 2) There can only be one occurrence of an assignment in an expression, and this may only be in a relation @@ -1504,6 +1499,7 @@ void GraphicsWindow::EditControlDone(const std::string &s) { if(e) { SS.UndoRemember(); if(usedParams > 0) { + // after a user finishes editing an expression in a constraint that has a different scaling_to_base, we can assume that they intend for the expression to be in the currently selected units c->expr_scaling_to_base = SS.MmPerUnit(); c->expression = s; } @@ -1560,7 +1556,6 @@ void GraphicsWindow::EditControlDone(const std::string &s) { } SK.GetGroup(c->group)->dofCheckOk = false; // if an named param was used in the constraint, the DoF may have changed SS.MarkGroupDirty(c->group); - SS.TW.HideEditControl(); } } From d5b2f533ed3480c72e96ccfd1a62bff243ee91a4 Mon Sep 17 00:00:00 2001 From: ruevs Date: Thu, 18 Apr 2024 20:36:17 +0300 Subject: [PATCH 28/30] Allow the "RELATION" constraint to be styled. Try to export it in DXFs. --- src/drawconstraint.cpp | 4 ++-- src/exportvector.cpp | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/drawconstraint.cpp b/src/drawconstraint.cpp index a736d486a..d1a85c74e 100644 --- a/src/drawconstraint.cpp +++ b/src/drawconstraint.cpp @@ -66,7 +66,7 @@ void Constraint::DoLabel(Canvas *canvas, Canvas::hStroke hcs, // By default, the reference is from the center; but the style could // specify otherwise if one is present, and it could also specify a // rotation. - if(type == Type::COMMENT && disp.style.v) { + if(((type == Type::COMMENT) || (type == Type::RELATION)) && disp.style.v) { Style *st = Style::Get(disp.style); // rotation first double rads = st->textAngle*PI/180; @@ -1350,7 +1350,7 @@ void Constraint::GetReferencePoints(const Camera &camera, std::vector *r } bool Constraint::IsStylable() const { - if(type == Type::COMMENT) return true; + if((type == Type::COMMENT) || (type == Type::RELATION)) return true; return false; } diff --git a/src/exportvector.cpp b/src/exportvector.cpp index 55abac685..268937a75 100644 --- a/src/exportvector.cpp +++ b/src/exportvector.cpp @@ -290,6 +290,7 @@ class DxfWriteInterface : public DRW_Interface { break; } + case Constraint::Type::RELATION: case Constraint::Type::COMMENT: { Style *st = SK.style.FindById(c.GetStyle()); writeText(xfrm(c.disp.offset), c.Label(), @@ -605,6 +606,7 @@ bool DxfFileWriter::NeedToOutput(Constraint *c) { case Constraint::Type::DIAMETER: case Constraint::Type::ANGLE: case Constraint::Type::COMMENT: + case Constraint::Type::RELATION: return c->IsVisible(); default: // See writeEntities(). From e52d37c3eb48873e45482e5c36b84461b0c6c3c2 Mon Sep 17 00:00:00 2001 From: ruevs Date: Wed, 26 Jun 2024 15:09:54 +0300 Subject: [PATCH 29/30] ExprParser: In case of error a "proper" `TokenType::ERROR` with `*expr = NULL` is returned. It probably does not matter but this was the old behaviour. --- src/expr.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/expr.cpp b/src/expr.cpp index 27761bb7f..382321517 100644 --- a/src/expr.cpp +++ b/src/expr.cpp @@ -795,7 +795,7 @@ ExprParser::Token ExprParser::Lex(std::string *error) { } ExprParser::Token ExprParser::PopOperand(std::string *error) { - Token t; + Token t = Token::From(); if(stack.empty() || ( stack.back().type != TokenType::OPERAND @@ -811,7 +811,7 @@ ExprParser::Token ExprParser::PopOperand(std::string *error) { } ExprParser::Token ExprParser::PopOperator(std::string *error) { - Token t; + Token t = Token::From(); if(stack.empty() || (stack.back().type != TokenType::UNARY_OP && stack.back().type != TokenType::BINARY_OP)) { *error = "Expected an operator"; From ad43f63988a5ed3ac5b6083ccfeab735619254a2 Mon Sep 17 00:00:00 2001 From: Dhruv Gramopadhye Date: Thu, 27 Mar 2025 11:29:08 -0700 Subject: [PATCH 30/30] Fix parameters "failed to solve" bug when adding a relation constraint (#1549) * add rel constraint expr params to same list as others > Using the windows build offered in the comments above, the solver will complain of an unsolvable constraint when adding named parameter-based constraints to multiple group's sketches after a first group's sketch already has constraints built out of named parameters. With regard to this unsolvable constraint bug, I have a fix (PR #1549 ). This is my bad. This happens because parameters created inside a relation constraint are not added to the correct list before being passed to the solver. This led to the solver being unable to solve an unused relation constraint. WLOG, say you use the default relation constraint x=5. The solver, in trying to solve x-5=0, then has 0 parameters to nudge, so it can't just set x = -5. Now if you added, say a line segment, and used this parameter, the expression in the line segment would have added the parameter to the correct list. Now the solver is actually passed x as a free variable and can solve for it. For cross-referencing, that's another can of worms and the discussion of how to approach is still ongoing iirc. --- src/constrainteq.cpp | 2 +- src/system.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/constrainteq.cpp b/src/constrainteq.cpp index 8cea1508e..5b4bb78f7 100644 --- a/src/constrainteq.cpp +++ b/src/constrainteq.cpp @@ -255,7 +255,7 @@ void ConstraintBase::Generate(IdList *l) { size_t eqpos = expression.find_first_of("="); ssassert(eqpos == expression.find_last_of("="), "There is at most one equals sign in the relation \"expression\""); ssassert(eqpos != std::string::npos, "There is at least one equals sign in the relation \"expression\""); - Expr::From(expression.substr(0, eqpos), false, &SK.param, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_MAX), false, &SK.param, NULL)); + Expr::From(expression.substr(0, eqpos), false, l, NULL)->Minus(Expr::From(expression.substr(eqpos+1, SIZE_MAX), false, l, NULL)); } else if(expression != "" && expr_scaling_to_base != 0) { Expr::From(expression.c_str(), false, l, NULL)->Times(Expr::From(std::to_string(expr_scaling_to_base).c_str(), false, l, NULL)); } else if(expression != "") { diff --git a/src/system.cpp b/src/system.cpp index c513796f6..3e812d174 100644 --- a/src/system.cpp +++ b/src/system.cpp @@ -425,7 +425,7 @@ SolveResult System::Solve(Group *g, int *rank, int *dof, List *bad, int x; dbp("%d equations", eq.n); for(x = 0; x < eq.n; x++) { - dbp(" %.3f = %s = 0", eq[x].e->Eval(), eq[x].e->Print()); + dbp(" %.3f = %s = 0", eq[x].e->Eval(), eq[x].e->Print().c_str()); } dbp("%d parameters", param.n); for(x = 0; x < param.n; x++) {