If you click the new requests it crashes on the describe screen. This can be "fixed" by adding one point in the EntityRequest mapping table, but we have no use for a point with a request for a parameter.

To use this, we will need to change the Evil Spirit expression parser to accept a list of name/handle pairs and NOT create new parameters on the fly if they are not in the list. Another function will need to be added to create that list from the group/request structure, but that can also handle scope/precedence rules to create group-specific list.

This should work without any new file format changes - parameter names are just in the requests and should save automatically.

and NOT create new parameters on the fly if they are not in the list

This seems like a "obviously" good idea. this will make parameters easier to conceptualize and groups easier to abstract for users

@phkahler @dgramop it seems we are on the way to having four different ways of "doing" parameters - both from the user's point of view and internally.

1. Op::VARIABLE

• Text starting with a capital letter in a constraint. Still not implemented, crashes intentionally.
• https://github.com/phkahler/solvespace/blob/a53e4a0e3902a5d9cc71423479635eb9230d4d6c/src/expr.h#L22
• https://github.com/phkahler/solvespace/blob/a53e4a0e3902a5d9cc71423479635eb9230d4d6c/src/expr.cpp#L342

2. "Silent" parameters - just type an expression containing parameters (starting with a small letter) in a constraint.

• Works
  

3. Relation constraints.

• Like comments but contain an expression with parameters. Mostly works
  
  Both of the above stored here: https://github.com/phkahler/solvespace/blob/a53e4a0e3902a5d9cc71423479635eb9230d4d6c/src/sketch.h#L717

4. Named parameter entities - this pull request

• Stored here https://github.com/phkahler/solvespace/blob/a53e4a0e3902a5d9cc71423479635eb9230d4d6c/src/sketch.h#L362
• Not working at this point

I still have not dug into the details of the named parameters code so I am not going to give you my opinion how things should work internally as not to bias you.

As for the user visible behaviour - perhaps they all can co-exist even though 1. in my opinion should be swallowed by 2. and 3. For me 2. and 3. are great and should definitely stay, I'm sure 4. is very desirable for some styles/habits of modelling.

We should think about how all of the above interact, how they are stored (in the save files and in RAM) and how the UI for finding/resolving unsolvable constraints should work - since 4. departs from everything that exists so far (including 2. and 3.)

If you prefer I can move this message to #77 as to not scatter the discussion.

@ruevs I think option 1 can go away. We aren't sure how that was supposed to work.

2 and 3 suffer the same problems as the demo. Scope / group rules aren't followed properly because parameters are created on the fly by the expression parser with no regard for group order.

4 provides a way to create named parameters. Actually a request which has the name, and a regular parameter with no name. 2 and 3 should be modified, or re-implemented over this structure.

I need to squash the commits here and then change the parser to use them. Then we can see if it should be the path forward. This approach should fix the scope problems.

Agree on axing 1. here if you need me to re-impl 2 and 3 over that structure, unless you think it's in-scope this PR

I've made a new personal branch off master that implements this text window for adding named parameters. I'm about to re-implement the old demo. It's not clear to me how the demo handles constants vs expressions in dimensions, so this comment is my notes on the process:

The regular code stores a number in valA of a constraint. The demo code in the function ConstraintBase::Generate() make a call to Expr::From(expression.c_str(), false, l); presumably to make sure any named parameters are added to the global list. This might create a parameter belonging to the constraint, or might create it without any "parent" at all. Either way, we will not need to do this since we will not be creating parameters as a side effect of expression evaluation - that is the root of the scope problems in the demo and could also lead to many more issues.

The demo adds a member string "expression" to constraints. If this string is not empty it is used to create an expression (object), otherwise it creates a simple expression from valA. It is not clear to me what determines which path is taken. This makes sense for compatibility, but does it sometimes rely on valA for new inputs?

In drawconstaint.cpp function Constraint::Label() also checks if the expression string and will display that instead of valA if it's not empty.

The rest of the demo code is modifying the expression parser to look up parameter handles by name, create them if the name doesn't exist, and count the number of named parameters.

The UI code in GraphicsWindow::EditControlDone() of all places decides to store the input string in the "expression" member or use valA depending if there are any named parameters in the expression string.

I think it makes sense to use keep using valA for numeric inputs so the label can be displayed as a string with units like feet and inches, or even just "mm". For this reason I think counting named parameters in the parser is a good thing to keep. BTW once an expression is used I don't see anywhere that would switch back to using valA in the demo, which seems like another bug.

The new version will not add a name string to parameters (it's in the request instead), and I'm also thinking about using the "comment" member in constraints for the user input rather than creating a new one called "expression" like the demo. This means zero changes to the file format, but it's also unfortunate that "comment" is so specific and not "string" because "comment" appears in .slvs files. "string" would have been a generic member like valA but instead we have "comment".

2 and 3 suffer the same problems as the demo. Scope / group rules aren't followed properly because parameters are created on the fly by the expression parser with no regard for group order.

2. and 3. are a "beautiful" user experience (ignoring the scope problems in the current implementation) - they should stay - I mean the user interface - not the implementation.

2 and 3 suffer the same problems as the demo. Scope / group rules aren't followed properly because parameters are created on the fly by the expression parser with no regard for group order.

2. and 3. are a "beautiful" user experience (ignoring the scope problems in the current implementation) - they should stay - I mean the user interface - not the implementation.

They will stay. My only requirement will be that you define variables in the text window before using them on the sketch.

Internally parameters can be created 3 ways: by a request, a group, or a constraint. In any of those cases the number of parameters is well defined and doesn't change. It is not IMHO reasonable to create them in an expression, even if that's part of a constraint. Use them there sure. Have "relation" constraints sure. Just don't be creating named parameters in there ;-)

@ruevs Is it really so bad to have to define them before use?

BTW I'm not certain associating them with requests is the best. I might have preferred an entity, but they don't need to have a visible sketch thingy. I'm still trying to understand how circles have a "dimension" and how the diameter label is created. I've stuck the parameter value in the same slot (64) as a radius in case maybe that help to add an editable sketch thing later.

@ruevs Is it really so bad to have to define them before use?

I'm not sure. Depends on how hard it would be to make the "scope rules" work if defining "on the fly" :-)

The problem with "on the fly" isn't just scope rules. Suppose:

1. Make a dimension with 50+x in group 2
2. Make another with 25+x+y in group 3

scope rules should make the x in the group 3 expression a constant since it was "solved" in group 2.

3. Change the dimension in group 2 to just plain 50.

At this point the "ownership" of x needs to actually change groups from 2 to 3. We could still figure out the first usage, but there is still the problem of "what" owns the parameters. I'm not sure a constraint can contain a variable number of parameters - that changes when the expression changes. So where to keep them? And BTW none of this logic belongs in the middle of the expression parser! ;-)

So I'd like to have a single function scan the requests (in this PR) and create a name/handle list for the current group. This can be passed to the expression parser which will need about 10 lines added to the version in master to handle those. The new function can apply scope rules to handle duplicate names or whatever. We could also use such a function with "on the fly" names to resolve scope, but the ownership issue might still be a problem. Internally, who would own the parameters?

I have a new branch on my fork called named_parameters. It includes the text window stuff and also using names for constraints. It obeys group ordering WRT parameters defined in previous groups, but the scope rules are still not there. General expressions are going to be a problem due to Unit issues, so I will probably limit dimensions to either a constant or a named parameter. You can experiment and see what issues are going to happen with units, particularly if you change the setting from mm to inches for example (the geometry should not change but it probably will). Playing around with it for feedback is encouraged.