[parameters] Save parameters to file, Relation constraint #1433

dgramop · 2023-12-07T22:35:04Z

Edit parameterized constraints in the GUI, (instead of their computed values) and save those constraints to the file.

Continues work on #77 on the parameters branch

With these changes, solvespace can still open files created by older versions. Older versions cannot open files created by this version, however.

Edit parameterized constraints in the GUI, (instead of their computed values) and save those constraints to a file.

dgramop · 2023-12-08T01:53:11Z

Instead of working on this completely solo and then dumping a big PR with all my progress, figured I should check in with some small changes to get your feedback and make sure I'm on the right track

phkahler · 2023-12-08T14:53:13Z

Instead of working on this completely solo and then dumping a big PR with all my progress, figured I should check in with some small changes to get your feedback and make sure I'm on the right track

@dgramop Thanks. I really appreciate you getting into this. I'll look at what I can, but I've been a bit detached from Solvespace lately. @ruevs seems to be in a similar state ;-) but he might review some stuff too. And of course others pop in sometimes totally unexpected with feedback!

The approach of extending the parameters branch is probably good. I'll have a look at this this weekend.

dgramop · 2023-12-09T06:32:03Z

Based on our discussion in #77, we may end up overhauling the dimension system anyhow. I can clean up the scale factor solution currently in my PR so that we have something to play around with until the unit system can be overhauled (if that's what is decided in #77).

dgramop · 2023-12-09T07:35:21Z

Alright looks like everyone's good to do the scaling solution. I'm going to still mark this as WIP since there are few more changes that I'd like to add on now that I've tested this version a little more

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.

dgramop · 2024-04-06T11:17:45Z

Still in progress....

Screen.Recording.2024-04-06.at.4.14.42.AM.mov

Here is a proof of concept!

Code is in my parameters_messy branch, but have several TODOs, a bit of cleanup, file format changes etc.

I created a new "Relation" constraint. It's two expressions delimited by an equals sign. The equals sign isn't added as an Op, instead when the Relation is parsed into an equation, it should be split up into two expressions. This is to avoid unintended side effects with "real" expressions that should evaluate to something, and where an assignment operator could through things off

dgramop · 2024-04-07T03:39:44Z

The following is a snapshot of what still needs to be done before this is ready for review:

Correctly Update DOF calculations when finished editing relation, since variables mentioned could have changed
- Currently if you create a constraint that doesn't reduce DOF, DOF still goes down by one until you start dragging stuff (which seems to force an update)
General code quality cleanup
- Fix up the scaling factors
  - Make sure scaling factors are never applied on RELATIONs
- Rebase and squash
Testing & error handling (taken care of by existing no-solution cases)
- Erroneous input
  - x = 2x
  - x = y, y= 2x

~~Variable Scoping~~ (EDIT Apr 7: I think this should be made a separate PR into parameters
- Currently, to reference a variable in a sketch, for example in another group like an extrusion, I have to make another dimension around something that reflects that variable and give it a variable in the new scope to control
~~Simplify expressions with duplicate scaling factors (requires a little more overhaul on the parser side, and would make this PR a little fat)~~

dgramop · 2024-04-08T05:44:38Z

I decided to postpone variable scoping into a later PR. I think it's best to merge changes into our parameters branch since these changes have reached atomicity.

The next (edit: Apr 12th, big) PR into that branch will be variable scoping, by which point we should (IMO) have more than enough to start discussing merging to parameters to master.

remove/address some todos fix SimplifyInverses, which is still broken and has no way to go back into the expression string

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

dgramop · 2024-04-12T08:02:39Z

src/solvespace.h

@@ -621,7 +621,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);


I chose to rename this function, since using it to get the value of a constraint's expression in base Mm is a bad idea, considering scale factor stapled to each expression in a constraint, which encodes the scale factor that constraint was "saved" under, which itself was necessarily to correctly render constraint labels while applying the right scalings.

By renaming the function, I'm hoping to force callers to declare compliance with this invariant

dgramop · 2024-04-12T08:03:15Z

res/icons/graphics-window/relation.png

Picasso, I consider myself

That is a beautiful = sign :-) ;-)

ruevs · 2024-04-12T13:06:47Z

@dgramop I've been following your progress with great interest. Unfortunately only from a phone at odd times of day when I had some spare time.

I'll try to spend some time on my SolveSpace development system and will pull, review and try your changes.

I agree I should merge them into "parameters" first (no squashing so that we have the history of how things happened) - this way you will have a new "base" for further changes. When we decide that it is time to merge to master I'll do the rabasing and squashing (to a few "meaningful" commits).

dgramop · 2024-04-13T19:12:26Z

Nice! By squash I meant my own parameters_messy branch, just so the PR is a single commit. Looking forward to more parametric features in solvespace!

... which is not standard and does not exist in Visual C++.

dgramop · 2024-04-16T06:43:19Z

@ruevs just rewrote history so hard that authoritarian regimes are taking notice. in any case, I went ahead and undid my squash and got pushed the full history in all its glory.

ruevs · 2024-04-16T09:53:37Z

@ruevs just rewrote history so hard authoritarian regimes are taking notice.

Just for context so others know what we are talking about dgramop#1 ;-)

@dgramop I played with your branch a bit. It actually works surprisingly well. Impressively thorough job!

Here is my first file using parameters: first_dgramop_parameters_test.zip

I'm particularly impressed and (pleasantly) surprised that there can be two "distance" constraints on the same line segment when one is an "equation". This is beautiful and I'll definitely dig into the code until I figure out why it is possible. I did also notice some interesting corner cases but let me first go through the code more carefully and add "formal" review comments here.

Another "sad" finding is that opening the above file in previous versions causes them to "crash" after the:

---------------------------
SolveSpace - Error
---------------------------
Unrecognized data in file.

This file may be corrupt, or from a newer version of the program.
---------------------------
OK   
---------------------------

dialog.

1.9, 2.0, 2.1, 2.3 close with no explanation.
3.0.rc1, 3.0, 3.1 crash an show the proper diagnostic:

---------------------------
Fatal error â€” SolveSpace
---------------------------
File D:\a\solvespace\solvespace\src\constrainteq.cpp, line 1057, function SolveSpace::ConstraintBase::GenerateEquations:
Assertion failed: false.
Message: Unexpected constraint ID.

Generate debug report?
---------------------------
OK   Cancel   
---------------------------

Of course this is normal and "unavoidable" considering the new Type::RELATION constraint type.

dgramop · 2024-04-16T22:58:40Z

For the next release, should we change the file extension (slvs2)? We can still let the new release open older slvs files, but when saved they'd be converted to slvs2. Parametric constraints will probably also confuse older versions.

dgramop · 2024-04-17T03:06:28Z

Something I noticed is that underscores are interpreted as "operators" which triggers an unknown operator exception. I'm a big fan of snake case, so I think for better user of parameters we should change this in the expr parser

ruevs · 2024-04-17T06:10:07Z

Something I noticed is that underscores are interpreted as "operators" which triggers an unknown operator exception. I'm a big fan of snake case, so I think for better user of parameters we should change this in the expr parser

@dgramop do you mean this?

solvespace/src/expr.cpp

Line 692 in 7d379e7

 if(!((c >= '0' && c <= '9') || c == 'e' || c == 'E' || c == '.' || c == '_')) break; 

To me it looks like they are ignored as part of "numbers" here:

solvespace/src/expr.cpp

Line 693 in 7d379e7

if(c == '_') {

This is a question to @jwesthues. Jonathan what was your plan idea to ignore _? E.g. the following are accepted as "700000" in a distance constraint:

700000
7E5
7e5
7__e_5___
7_0_0_0_0_0_____

dgramop · 2024-04-17T06:16:56Z

@ruevs while LexNumber allows underscores, when the generic Lex(...) function is called, LexNumber(...) is only attempted if(isdigit(c) || c == '.'). So an underscore outside the context of a number ispunct is true, so Lex tries to parse it as an operator... (edit: to permit snake case, the read word function needed to allow underscores, which I just committed)

(Edit: also, how did you get those nice looking links to code with preview?)

dgramop · 2024-04-17T06:43:57Z

https://youtu.be/L8cyW_mXisE

There's another bug here with how constraints are taken care of, I haven't really done much with inter-group named parameters. I think taking care of this falls under variable scoping so maybe we visit that in the next PR... Just wanted to document this here so I can pick up after it

phkahler · 2024-04-17T14:33:14Z

Reference

This is exactly the scope issue I talked about previously. The test_test parameter is first created in the 2D sketch. Later, when it is used in the 3D extrusion, it should be treated as a constant value because it was defined and "solved" in a previous group - so the extrusion height should not be draggable. This should be equivalent to putting an equal length constraint on the height of the extrusion and the width of the extruded rectangle/square.

AFAICT part of the issue is that the named parameters are created during expression parsing and are not associated with any group. I'm not sure how such an association is normally created, but it seems to be related to their handles.

One thing I had considered is making a named parameters a "request" that gets displayed in the text window for a group. It should be possible to get the group connection established that way, and we need the functionality anyway. Perhaps adding this capability and creating the parameter prior to use would "fix" the problem. If so, one solution would be to require such a definition and NOT create named parameters on the fly during expression evaluation.

TLDR; Most parameters are created in response to entity creation. Somehow that gets them associated with the group that entity resides in. We need named parameters to have that same group association.

ruevs · 2024-04-17T15:08:12Z

(Edit: also, how did you get those nice looking links to code with preview?)

Browse through the project until you get to the file and line you want:
Click on the line number.
Click on the ellipsis that shows up
"Copy permalink"
Paste the link that is now in your clipboard.

ruevs · 2024-04-17T16:01:46Z

For the next release, should we change the file extension (slvs2)?

In my opinion - no. We have introduced new features that impact the file format many times since 2.0 (which was the first open source version). Here is a few that I can think of:

In this cases the old versions simply show the warning - they always do - @jwesthues was forward looking enough to ensure that back in 2009 b974a4a#diff-09121c6f65665acb5c49233fc2f9d269f6da3f97c924fc5ff34aec625f0b01f7R464

So let them show the warning and then "crash and burn" or do whatever the particular new features in the file will cause - many times the entities/constraints are simply missing.

ruevs · 2024-04-17T16:20:13Z

@dgramop do not forget the other loose end that brought you here ;-) Search for Op::VARIABLE.

solvespace/src/expr.h

Line 22 in 7d379e7

VARIABLE = 21,

As I get deeper into the parameters branch I'll also keep it in mind.

jwesthues · 2024-04-18T05:23:15Z

@ruevs I probably intended to allow underscores in literal numbers so that you could write stuff like 1_000_000, inspired by the corresponding behavior in C, and implemented it lazily and half-heartedly. There should be no problem fixing it in the obvious way.

And yeah, no change to the file extension. I'd avoid unnecessary breakage where possible (e.g., don't generate new stuff in the saved file if the new feature isn't actually used), but otherwise just let the users get that warning to upgrade.

dgramop · 2024-04-18T05:45:15Z

@phkahler

that the named parameters are created during expression parsing and are not associated with any group. I'm not sure how such an association is normally created, but it seems to be related to their handles.
making a named parameters a "request" that gets displayed in the text window for a group

I have some code in my other branch that lists parameters by group, but it does so in a roundabout way: by first collecting parameters by constraint in a set, then displaying those parameters.

TLDR; Most parameters are created in response to entity creation. Somehow that gets them associated with the group that entity resides in. We need named parameters to have that same group association.

@ruevs

We have introduced new features that impact the file format many times since 2.0

Fair, and the way the format is structured if they don't use the RELATION constraint, savefiles will be backwards compatible. I walk back that suggestion

dgramop · 2024-04-20T02:19:13Z

@dgramop do not forget the other loose end that brought you here ;-) Search for Op::VARIABLE.

solvespace/src/expr.h

Line 22 in 7d379e7

VARIABLE = 21,

Wow! That feels like an eternity ago!

ruevs · 2024-04-20T04:35:15Z

Wow! That feels like an eternity ago!

This is closer to an "eternity ago" ;-)
61f2488

ruevs

@dgramop I finally found time to review your changes carefully. My comments are mostly minor. Please take a look.

Also you may want to merge my PR.

src/drawconstraint.cpp

src/expr.cpp

ruevs · 2024-04-23T08:06:42Z

src/expr.cpp

@@ -795,8 +797,13 @@ ExprParser::Token ExprParser::Lex(std::string *error) {
 }

 ExprParser::Token ExprParser::PopOperand(std::string *error) {
- Token t = Token::From();
- if(stack.empty() || stack.back().type != TokenType::OPERAND) {
+ Token t;


Does the code that uses the returned t rely on some of the initialization that From does when we do not go through the else branch?

Don't think so, I think I was just explicitly calling constructor for readability. Should I revert this change?

ruevs · 2024-04-23T08:08:26Z

src/expr.cpp

@@ -806,7 +813,7 @@ ExprParser::Token ExprParser::PopOperand(std::string *error) {
 }

 ExprParser::Token ExprParser::PopOperator(std::string *error) {
- Token t = Token::From();
+ Token t;


Does the code that uses the returned t rely on some of the initialization that From does when we do not go through the else branch?

Don't think so, I think I was just explicitly calling constructor for readability. Should I revert this change?

ruevs · 2024-04-23T08:09:17Z

src/expr.cpp

@@ -838,17 +845,31 @@ 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()))
for clarity. An two more places below

src/mouse.cpp

ruevs · 2024-04-23T15:01:28Z

src/mouse.cpp

@@ -1429,32 +1465,71 @@ 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()) {


Isn't this redundant? See 1506 below.
It may also be wrong - if editing the "relation" does not succeed in the "equals checks" below.

ruevs · 2024-04-23T15:03:28Z

src/mouse.cpp

 }

 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; 


src/mouse.cpp

dgramop · 2024-04-30T21:16:48Z

sorry folks! I got busy with work and was at hackathon but will be reviewing this ASAP.

dgramop · 2024-06-03T02:51:14Z

@ruevs addressed some of the items in your review & asked some follow-up questions... will commit review changes soon

Save parameters to file, Edit parameters in GUI

b27ab59

Edit parameterized constraints in the GUI, (instead of their computed values) and save those constraints to a file.

dgramop changed the title ~~Save parameters to file, Edit parameters in GUI~~ [parameters] Save parameters to file, Edit parameters in GUI Dec 8, 2023

ruevs marked this pull request as draft December 9, 2023 16:14

ruevs linked an issue Dec 9, 2023 that may be closed by this pull request

Parametric sketches #77

Open

ruevs removed a link to an issue Dec 9, 2023

Parametric sketches #77

Open

dgramop added 9 commits December 13, 2023 09:09

[dirty]: scale factors + geom inverse finder

7bbd084

Tracability between ordered tokens and expr nodes

4b56e1b

Preserve original token types

d8b0d71

bubble up deletion

12ae65c

proper deletion and parentheticals

adef204

debug printing and whitespace changes

5e1bac8

stubs for relation constraint

360e8ae

relations without eq operator

61e2269

dgramop added 5 commits April 6, 2024 04:22

expressions with eq

b8b76b6

relation in toolbar, menu, and keybind

92f5a40

fix: save expr scaling to base to prevent solver pushing params to zero

23d14a3

fix: some code cleanup

8749f75

fix savefile format and some zero guards

84c556b

mark dirty for dof computation

23ff427

dgramop changed the title ~~[parameters] Save parameters to file, Edit parameters in GUI~~ [parameters] Save parameters to file, Relation constraint Apr 8, 2024

dgramop added 2 commits April 11, 2024 20:07

several

72fadaa

remove/address some todos fix SimplifyInverses, which is still broken and has no way to go back into the expression string

Remove SimplifyInverses and PrintTokens

7cb7448

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

dgramop commented Apr 12, 2024

View reviewed changes

ruevs self-requested a review April 12, 2024 13:10

ruevs added 3 commits April 15, 2024 16:56

Small clean up to align with dgramop/parameters

afd74fd

SIZE_MAX instead of "SIZE_T_MAX"...

4f8984b

... which is not standard and does not exist in Visual C++.

UI: Adjust the toolbar height to accommodate the new "relation" icon.

10eafda

dgramop force-pushed the parameters branch from f0ce253 to 10eafda Compare April 16, 2024 06:40

fix: allow underscores inside named parameters

11811c7

ruevs mentioned this pull request Apr 17, 2024

Path to hierarchical parametric sketches #1436

Open

ruevs reviewed Apr 23, 2024

View reviewed changes

fix: address review items

2aadd13

[parameters] Save parameters to file, Relation constraint #1433

Are you sure you want to change the base?

[parameters] Save parameters to file, Relation constraint #1433

Conversation

dgramop commented Dec 7, 2023

dgramop commented Dec 8, 2023

phkahler commented Dec 8, 2023

dgramop commented Dec 9, 2023

dgramop commented Dec 9, 2023

dgramop commented Apr 6, 2024 • edited

dgramop commented Apr 7, 2024 • edited

dgramop commented Apr 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruevs commented Apr 12, 2024

dgramop commented Apr 13, 2024

dgramop commented Apr 16, 2024 • edited

ruevs commented Apr 16, 2024

dgramop commented Apr 16, 2024

dgramop commented Apr 17, 2024

ruevs commented Apr 17, 2024

dgramop commented Apr 17, 2024 • edited

dgramop commented Apr 17, 2024

phkahler commented Apr 17, 2024

ruevs commented Apr 17, 2024 • edited

ruevs commented Apr 17, 2024

ruevs commented Apr 17, 2024 • edited

jwesthues commented Apr 18, 2024

dgramop commented Apr 18, 2024

dgramop commented Apr 20, 2024

ruevs commented Apr 20, 2024

ruevs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgramop commented Apr 30, 2024

dgramop commented Jun 3, 2024

dgramop commented Apr 6, 2024 •

edited

dgramop commented Apr 7, 2024 •

edited

dgramop commented Apr 8, 2024 •

edited

dgramop commented Apr 16, 2024 •

edited

dgramop commented Apr 17, 2024 •

edited

ruevs commented Apr 17, 2024 •

edited

ruevs commented Apr 17, 2024 •

edited