Add descriptor details to the command language #148
No reviewers
Labels
No labels
bug
design
duplicate
engine
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
StudioInfinity/dyna3!148
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Vectornaut/dyna3:details"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I'll add more details to this description later. I'm posting it as-is because I don't want writing the description to delay the opening of the pull request.
Summary
The branch to be merged improves the command language introduced in pull request #138, adding all the features we need to write parsimonious loading scripts for our current test assemblies. The central new feature is the descriptor details syntax, which is based on the idea of "creation attributes" from our design discussions.
Features
In addition to the descriptor details syntax, the branch to be merged introduces the following language features:
Loading scripts
The branch to be merged includes a loading script for each test assembly, proving that it includes enough language features to do that. The test assembly menu now has a second entry for each test assembly, which loads the assembly using a script. I've done a few visual and numerical checks to verify that the loading scripts produce the same assemblies as the original loading functions, although these checks aren't particularly thorough or reproducible.
SetLabelcode 951f836211@ -163,0 +164,4 @@f,"{}{}",self.label().get_clone_untracked(),self.representation().get_clone_untracked(),Please now can you put the id in here as the primary identifier? That's how we want to use it -- id should be the basic/standard/common way to refer to an entity; the label is just for showing the entity, and it will be quite common for assemblies to have multiple entities with the same label. Thanks.
Done in commit
23a7382. I don't like how the label and identifier have the opposite ordering as in the outline view, but ultimately it's up to you.Hmm, I am definitely not trying for there to be any difference in ordering. What I am trying for is the id to be the primary identifier of an entity throughout dyna3, and for the label to (a) be a secondary characteristic specifying one aspect of the decoration of an entity, much akin to "color," and (b) ultimately default to the id if no label other than that has ever been specified. So in the end if our canonical name-list for points ends up being A,B,C,... then I would expect the first point created with no id specified have id "A" and thereby an implicit label "A" and so on. I would expect to see in the graphic display, assuming that label display is on for this point, the text "A" plotted next to this point, and in the outline view, that this point is simply designated as
A; there is no need to indicate an id and label separately, because the id is always shown and no label has ever been set.Then maybe I create a bunch more points, but then decide I want the point with id "I" to have label "centroid" so I change it, however that's currently done. I would then expect that (a) assuming label display is on for this point in the graphic display, the text plotted next to the point changes from "I" to "centroid", and (b) the primary designation of this point in an outline view remains "I", but that there might well be some indication that the label has been set, so the outline might change from showing just
Ito showing (say)I "centroid"or something like that.I guess we've never really had a conversation about the use/semantics of labels and ids along these lines before, for which I apologize. So I realize switching to their roles as described in this comment might be out of scope for this PR, and if so, should be captured in a separate issue. If so, just let me know and I will file an issue laying out the desired roles of id and label, and in the meantime, you should do only as much in this PR insofar as moving toward the goals of that issue as is consistent with the scope of this PR. Thanks!
@ -197,2 +202,3 @@pub representation: Signal<DVector<f64>>,pub color: Signal<ElementColor>,pub ghost: Signal<bool>,pub representation: Signal<DVector<f64>>,Is the new order of fields in this struct important? Would generally avoid just permuting without a specific motive..
When I was organizing
action.rs, putting the label, color, and ghost descriptors together led to the most readable code, because these are all element attributes, interpreted in similar ways. Since the representation descriptor is an engine data attribute, the code for interpreting it looks pretty different.I didn't want to break with my convention of putting interpreter code sections related to
Elementfields in the same order as those fields, so I decided to re-order the fields. My original motivation for putting the representation field between the label and color fields feels pretty weak (and I barely remember it), so I think there's little cost to changing it.@ -299,2 +308,3 @@pub representation: Signal<DVector<f64>>,pub color: Signal<ElementColor>,pub ghost: Signal<bool>,pub representation: Signal<DVector<f64>>,Same comment on permuting fields.
Same response as above.
OK, resolving both of these comments per your explanation. However: do we want to have some section of these structures commented as the "attributes" so that it will be clear where future attributes should go? There will certainly be numerous others in the long run: size and or shape for points, maybe ultimately texture for spheres, maybe "hidden" state for both, etc.
@ -883,0 +908,4 @@|elts_by_id| !elts_by_id.contains_key(&id_new));if can_rename {console_log!("Can rename"); /* DEBUG */Should such debugging console logs be removed in a main-line commit? I generally think so...
Yes, I think so too. Done in commit
43eb341!@ -880,6 +899,42 @@ impl Assembly {}}pub fn try_rename_element(Name of this function? We are using "name" in a different way in dyna3, if I understand correctly, so that the name of a point element is "Point", and that name cannot be changed. Shouldn't this be something like
try_change_id?I see what you mean. I don't envision making "name" part of our user-facing terminology, because to me it feels like it could equally well refer to an identifier or a label, but we do use it internally in the
Namedtrait, which we apply to any structure we want to give an associatedNAMEconstant to. I agree that a method name like one of these would clarify the connection to theidmethod ofElement:try_change_idtry_change_element_idtry_set_idtry_set_element_idAny preferences?
I had suggested "(try_)change_id" as being closest to "rename". I don't know that we need to mention it operates on elements (unless we need a similar method for regulators?) since in a call it will have the element as an argument, so it will be clear it is operating on an element. So I guess my preference is "try_change_id" but I am ok with "try_set_id" as well, and if there is ever a need to have a similar operation for something other than elements, we could put the word "element" back in the name at that time.
Oh, and if we end up with a design by which renaming always succeeds by some mechanism, then I suppose we can remove the "try_" part from the name...
Done, in commit
9f64bd8.@ -883,0 +906,4 @@) -> bool {let can_rename = self.elements_by_id.with_untracked(|elts_by_id| !elts_by_id.contains_key(&id_new));Are the naming requests coming from "user interaction" or are they somehow internally generated? If the former, the most common behavior I have seen in other dyngeo systems is to change the id of the other element with the colliding id to an algorithmically generated variant, and accept the id change on the current element. Perhaps this is a point that warrants further discussion?
Right now, the only way to change an element's identifier is through the command prompt, although I imagine we'll want to provide ways to change it through the outline view and other views too.
This behavior sounds very confusing to me, so I'd like to play with some examples if possible. Neither of the systems I tried behaved this way.
A = (1, 0)andB = (0, 1)and then try to rename the second point by changing its input toA = (0, 1), I get an error in the second input cell: "This label is already in use A."Yes, that is what happens if you try to have the "algebra input" to GeoGebra have two lines with identical identifiers. Our command language is a bit different, so we will have to think about what should happen if we have two different "A is..." commands in a row.
However, in the web version of geogebra, if you create points with inputs A = (1, 0) and B = (0, 1) and then right click on point B in the graphic display and select settings, and then in the "Name" field delete "B" and enter "A" and hit return, then "B" is renamed to "A" and "A" is renamed to "A₁".
I should take back the "most common behavior" comment. I just tried three projects with recent code activity: Dr.Geo, Kig, and FluxGeo. They all simply allow one to give two points the same "name." However, the first two have no command language, so the concept of "name" is essentially what we're calling "label" -- there's no need/opportunity to refer to an object by name. In FluxGeo, though, there is a command interpreter, and renaming a second point to A produces the frustrating situation that there is no way to refer to that second point -- using A in the command interpreter continues to refer only to A. (The interpreter does report a unique interal object id, akin to our serial, on each object creation, but I don't see how to refer to an object by this id...) I would categorize this situation with respect to naming in FluxGeo as either "poorly designed" or "buggy".
The difficulty with just having a command from the person making the construction to rename a point to A fail with "'A' is already in use" is that often one is making a construction with specific names in mind for the points. But maybe it's not convenient to create them in the order A, B, C,... So the auto-assigned names get off and you have to rename some of them. So you focus on one you know the name of, and issue a rename command. There may be a lot of points already, and you didn't realize the name was in use, but you know you want this point to be named A. If it fails, you have to go and find the point that currently happens to be named A, and change its name to something. Well, the natural thing to change it to is the name that point is supposed to have. But maybe that name is in use, too! Etc, etc. And if the whole thing happens to be in a cycle, it leaves the human with the burden of breaking the cycle somewhere with a temporary name, and ending up doing n+1 renamings when there were only n things in the cycle. It's a recipe for frustration.
So I would say when the person wants to rename (i.e., change the id) of a point, and specifically instructs dyna3 to do so, then that id change has to succeed. But we also have to preserve the invariant that ids are distinct. So I don't see any way to avoid geogebra's behavior of altering the id of the entity that had the id that the person now wants to assign elsewhere. I am certainly open to other suggestions, though, so we should discuss.
Aha! Thanks for explaining.
In our setup, the thing that makes this behavior most confusing to me is to imagine reading a command history like this.
Given this history, what state could we be in?
Current behavior
If identifier assignment fails when you try to assign an identifier that's already in use, it's easy to understand what state we've ended up in. The first command has succeeded, as long as the identifier
awasn't in use already, and the subsequent commands have all failed. If the first command succeeded, we've created one point:This result matches what I'd naïvely expect the first command to do.
GeoGebra's behavior
With GeoGebra's behavior, we need to understand the renaming system and know the full state of the assembly to work out what state we've ended up in. If none of the
a-series identifiers were in use already, we end up with these points:This isn't a result I could've guessed before trying it. If any of the
a-series were in use, the result gets even more chaotic. For example, if we already had a trianglewe now have
If we already had a line segment
we now have
This may not be a big deal in GeoGebra, where there's no scripting system or command history, and thus no sense of tension between what the commands look like they do and what they actually do. In dyna3, though, it seems like we expect the command system to be pretty central, so I feel like it's more important for commands to have straightforward results.
@ -28,1 +21,3 @@},let script_result = batch(|| run_script(&state, &value.get_clone()));Is the idea that now what I type all at once in the command prompt might be multiple commands, and that's why this is now run_script instead of command.execute?
You can still only enter one statement at a time in the command prompt—not as an intentional decision, but just because statements are separated by newlines, which the HTML
<input type="text">element doesn't accept.I replaced
action::interpretwithaction::run_scriptbecause I only wanted to have one syntax rule that tries to match all the way to the end of the string it's given (thescriptrule), and I only wanted the action module to expose one function that interprets a code string.I don't see a need for a function that tries to parse exactly one statement, and returns an error if it finds multiple statements. It wouldn't be too hard to introduce one if we want it, though.
No, it wasn't my intention to have a different special function for single-command strings. I guess I was imagining that run_script was a loop over something like command_execute and so wondering why the change here in a context that can only have one command. But I infer from your comment that's not really how run_script is built and there really isn't any analogue of a single command_execute anymore, in which case this is fine as it is. Resolving.
OK, I've gone through the code. Nice work! It is nice that this PR is much smaller than the action system PR. I haven't actually tried to run the new code, but I think there are a few things to chew on in the comments I made in the meantime. I do see the increase in adding things untracked, so that's good.
Displayformatting1d30378fb69f64bd8a6fView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.