StudioInfinity/dyna3
2
0
Fork 2
Code Issues 79 Pull requests 1 Projects 3 Releases 4 Wiki Activity Actions

feat: Introduce an action system and command language #138

Merged
glen merged 26 commits from Vectornaut/dyna3:actions into main 2026-05-30 16:29:44 +00:00
Conversation 24 Commits 26 Files changed 18 +1730 -139
Vectornaut commented 2026-03-19 07:49:20 +00:00
Member
Copy link

The branch to be merged introduces actions: trait objects representing things a user can do. It also introduces a command language for describing actions.

One purpose of the action system is to ensure consistency between different user affordances for doing the same thing. On the branch to be merged, as a proof of concept, the element and regulator creation buttons work by creating and executing actions, rather than by acting directly on the application state. A newly introduced keyboard shortcut system provides a different way to trigger each of these actions (addressing issue #132). You can also carry out actions by describing them in the command language and entering those descriptions into the newly introduced command prompt component (addressing issue #133).

The branch to be merged introduces [_actions_](wiki/Action-system): trait objects representing things a user can do. It also introduces a [_command language_](wiki/Command-language) for describing actions. One purpose of the action system is to ensure consistency between different user affordances for doing the same thing. On the branch to be merged, as a proof of concept, the element and regulator creation buttons work by creating and executing actions, rather than by acting directly on the application state. A newly introduced keyboard shortcut system provides a different way to trigger each of these actions (addressing issue #132). You can also carry out actions by describing them in the command language and entering those descriptions into the newly introduced _command prompt_ component (addressing issue #133).
In the process, revamp the element insertion system and turn elements'
identifiers and labels into signals.

This commit moves the responsibility for adding default regulators from
the assembly to the action system, but it the element creation buttons
still work directly on the assembly. This leads to a regression: the
element creation buttons leave out the default regulators.
Enable interface buttons to trigger actions rather than directly
manipulating the application state.

Reimplement the element and regulator creation buttons as action
buttons. This addresses the regression in the previous commit: the
element creation buttons now produce default regulators again.

This commit introduces a new regression: the button that creates an
inversive distance regulator no longer clears the selection. A reminder
to fix this is added as comment on the `ParametricAction` structure.
Unlike boxed actions, reference-counted actions can be cloned, so we can
use them as parameters for parametric actions.
This reverts `subject_array_from_vec` to its original form. We made it
more generic to work around difficulties with cloning boxed actions,
but we're not using boxed actions anymore.
Before this commit, the buttons and shortcuts for element creation used
parametric actions where the parameter was always `Ok(())`. With
constant actions, we can make those buttons and shortcuts simpler.
In the process, introduce a macro to streamline entity naming.
Since conditional actions are meant for repeated use, we rebrand them
as "actors".
We've gotten away with making `peek` return an owned value so far
because actions' values tend to be lightweight and clonable: often
reference-counted pointers. Conceptually, though, peeking should only
provide a reference, because we expect an action to hold onto its
precomputed value until it's executed.
Use this to implement the creation of half-curvature regulators.
In the process, unify regulator creation actions.
In particular, state the programmer-enforced assumptions that the action
system relies on.
Explain the proper use and spirit of actors
All checks were successful
/ test (pull_request) Successful in 3m49s
Details
80d7cf733b
Move actors to their own module
Some checks failed
/ test (pull_request) Failing after 1m41s
Details
dd05799e20 
In the process, rename the module providing the `ActorButton` component
to match the component's current name.
Vectornaut commented 2026-03-19 21:50:28 +00:00
Author
Member
Copy link

@glen I've moved the Actor trait into its own module, as we discussed during the meeting, so the pull request is ready for review! I think this move is an all-around improvement: it reduces clutter in the action module, and it improves the organization of action and actor imports.

@glen I've moved the `Actor` trait into its own module, as we discussed during the meeting, so the pull request is ready for review! I think this move is an all-around improvement: it reduces clutter in the `action` module, and it improves the organization of action and actor imports.
Actually check in the new actor module
All checks were successful
/ test (pull_request) Successful in 3m42s
Details
8c28db05d2
Clarify naming of descriptor declaration action
All checks were successful
/ test (pull_request) Successful in 3m55s
Details
8e1d866d29
Vectornaut changed title from feat: Introduce an action system and command language to WIP: feat: Introduce an action system and command language 2026-03-26 04:58:03 +00:00
Vectornaut commented 2026-03-26 05:07:04 +00:00
Author
Member
Copy link

I just noticed that regulators created by descendant actions aren't actually being resolved correctly. The command

HalfCurvature Sphere @ 3

doesn't set the regulator of the newly created sphere, because we end up looking for the half-curvature regulator in a clone of the sphere creation action rather than in the original sphere creation action. The cloned sphere creation action creates a different sphere, with a different default half-curvature regulator.

I didn't notice this before because the sphere created by the cloned action has the same curvature has the sphere created by the original action. The command HalfCurvature Sphere therefore prints the expected measurement, even though it's not reading from the expected regulator.

I've put the pull request in draft mode while I work on a fix.

I just noticed that regulators created by descendant actions aren't actually being resolved correctly. The command ``` HalfCurvature Sphere @ 3 ``` doesn't set the regulator of the newly created sphere, because we end up looking for the half-curvature regulator in a clone of the sphere creation action rather than in the original sphere creation action. The cloned sphere creation action creates a different sphere, with a different default half-curvature regulator. I didn't notice this before because the sphere created by the cloned action has the same curvature has the sphere created by the original action. The command `HalfCurvature Sphere` therefore prints the expected measurement, even though it's not reading from the expected regulator. I've put the pull request in draft mode while I work on a fix.
Use the UnitAction type alias more
All checks were successful
/ test (pull_request) Successful in 3m45s
Details
d316a8f2ed
Use the Self type more
All checks were successful
/ test (pull_request) Successful in 3m42s
Details
50b96be285
Vectornaut commented 2026-03-26 09:21:37 +00:00
Author
Member
Copy link

I've drafted a fix for the bug discussed in this comment. The draft includes a strengthened descendant_regulator_resolution_test that's able to catch the bug, using the technique described in the comment.

The fix works by introducing an environment that keeps track of all the elements and regulators that can be referenced within a command. We carry the environment through the whole interpretation process, updating it whenever the thing we're interpreting creates a regulator.

The current implementation of the environment is a little rough around the edges. For example:

  • It provides parallel "recorded" versions of all the creation command constructors, which take a mutable reference to an environment and record all created regulators in it. This code is pretty repetitive.
  • It clones the elements_by_id index from the state into the environment for convenience, even though we could've saved a clone by passing the application state and using the elements_by_id index from there.

The implementation is decent enough, though, that I'd feel okay including it in this first pass. Shall I go ahead and push it to the branch to be merged, or would you like to discuss it at a check-in meeting first?

I've drafted a fix for the bug discussed in [this comment](pulls/138#issuecomment-3793). The draft includes a strengthened `descendant_regulator_resolution_test` that's able to catch the bug, using the technique described in the comment. The fix works by introducing an environment that keeps track of all the elements and regulators that can be referenced within a command. We carry the environment through the whole interpretation process, updating it whenever the thing we're interpreting creates a regulator. The current implementation of the environment is a little rough around the edges. For example: - It provides parallel "recorded" versions of all the creation command constructors, which take a mutable reference to an environment and record all created regulators in it. This code is pretty repetitive. - It clones the `elements_by_id` index from the state into the environment for convenience, even though we could've saved a clone by passing the application state and using the `elements_by_id` index from there. The implementation is decent enough, though, that I'd feel okay including it in this first pass. Shall I go ahead and push it to the branch to be merged, or would you like to discuss it at a check-in meeting first?
glen commented 2026-03-28 10:02:27 +00:00
Owner
Copy link

@Vectornaut wrote in #138 (comment):

Shall I go ahead and push it to the branch to be merged,

Yes, I am keen to move this along and in to the main branch. Thanks!

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/138#issuecomment-3796: > Shall I go ahead and push it to the branch to be merged, Yes, I am keen to move this along and in to the main branch. Thanks!
This replaces the old system where regulator information was stored
throughout the action tree, and fixes a bug that the old system made
difficult to address. We modify `descendant_regulator_resolution_test`
to catch the bug.
Rename descriptor statements as declarations
All checks were successful
/ test (pull_request) Successful in 3m43s
Details
4c9ddff53a
Vectornaut changed title from WIP: feat: Introduce an action system and command language to feat: Introduce an action system and command language 2026-03-31 07:40:24 +00:00
Vectornaut commented 2026-03-31 07:46:27 +00:00
Author
Member
Copy link

Okay, I've pushed the fix and reactivated the pull request! Commit 1405329 does a reorganization that I'm finding helpful for implementing the "details" or "attributes" syntax we'll use to avoid repetition in the engine block and elsewhere in our save files.

Okay, I've pushed the fix and reactivated the pull request! Commit 1405329 does a reorganization that I'm finding helpful for implementing the "details" or "attributes" syntax we'll use to avoid repetition in the engine block and elsewhere in our save files.
glen commented 2026-05-28 17:22:55 +00:00
Owner
Copy link

In one of the comments you say

    // when the parameter of a parametric actor is read from the selection, we
	// typically want to clear the selection when the action runs

I just want to say that aspect of UI design is not at all clear. There are absolutely cases where one wants to run multiple actions on the same selection, and there can be cases where it's a bit of a process to get just the selection one wants -- say all of the elements of some conceptually coherent "subassembly," especially if we don't have a formalized concept of that within dyna3 -- and it could be frustrating for that selection to disappear and have to be selected again after, say, a color change but before wanting to do a move or a copy or something. So my intuition is that actually the default for actions is that they leave the selection alone. Actions that delete some or all of the selection elements should likely reset the selection. Possibly actions that create new element(s) should switch the selection to those new element(s). Not at all sure on some of these points, we should just implement the path of least resistance (which I gather is to leave the selection be) and then later see if we find ourselves wishing something different had happened.

You don't need to change the comment in this PR, to avoid rebasing later work, but as convenient please remove the comment and add something in the issues/wiki about designing selection behavior resulting from actions. Thanks!

In one of the comments you say ``` // when the parameter of a parametric actor is read from the selection, we // typically want to clear the selection when the action runs ``` I just want to say that aspect of UI design is not at all clear. There are absolutely cases where one wants to run multiple actions on the same selection, and there can be cases where it's a bit of a process to get just the selection one wants -- say all of the elements of some conceptually coherent "subassembly," especially if we don't have a formalized concept of that within dyna3 -- and it could be frustrating for that selection to disappear and have to be selected again after, say, a color change but before wanting to do a move or a copy or something. So my intuition is that actually the default for actions is that they leave the selection alone. Actions that delete some or all of the selection elements should likely reset the selection. Possibly actions that create new element(s) should switch the selection to those new element(s). Not at all sure on some of these points, we should just implement the path of least resistance (which I gather is to leave the selection be) and then later see if we find ourselves wishing something different had happened. You don't need to change the comment in this PR, to avoid rebasing later work, but as convenient please remove the comment and add something in the issues/wiki about designing selection behavior resulting from actions. Thanks!
👍 1
glen reviewed 2026-05-28 17:33:30 +00:00
app-proto/src/assembly.rs
@ -141,2 +158,4 @@
}
impl Display for dyn Element {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
glen commented 2026-05-28 17:33:30 +00:00
Owner
Copy link

Where is this used? Isn't it more important to show the id, which uniquely designates the element, than the label? I'm not necessarily saying don't show the label, but the id seems a higher priority piece of information, if anything.

Where is this used? Isn't it more important to show the id, which uniquely designates the element, than the label? I'm not necessarily saying don't show the label, but the id seems a higher priority piece of information, if anything.
Vectornaut commented 2026-05-28 19:38:42 +00:00
Author
Member
Copy link

When you enter an element descriptor at the command prompt, this Display implementation is used to print the element on the browser console. In general, the Display trait is used for "pretty printing" structures. Note that dyn Element also implements the Debug trait, which I think is more often used to format information about a structure's internals; the Debug formatting just gives the ID.

In the next pull request, we'll start showing each element's ID after its label in the outline view. For consistency, in that pull request, maybe we should also start showing an element's ID in parentheses after its label in the Display formatting.

When you enter an element descriptor at the command prompt, this `Display` implementation is used to [print the element](https://code.studioinfinity.org/Vectornaut/dyna3/src/commit/4c9ddff53a8b8a05d2d375edd8f9f652b7151af1/app-proto/src/action.rs#L181-L184) on the browser console. In general, the `Display` trait is used for "pretty printing" structures. Note that `dyn Element` also [implements](https://code.studioinfinity.org/Vectornaut/dyna3/src/commit/4c9ddff53a8b8a05d2d375edd8f9f652b7151af1/app-proto/src/assembly.rs#L154-L158) the `Debug` trait, which I think is more often used to format information about a structure's internals; the `Debug` formatting just gives the ID. In the next pull request, we'll start showing each element's ID after its label in the outline view. For consistency, in that pull request, maybe we should also start showing an element's ID in parentheses after its label in the `Display` formatting.
glen commented 2026-05-28 20:05:34 +00:00
Owner
Copy link

Sounds fine to leave this until the next PR, but please make the ID the primary identifier and if you want to show the label, make that appear "subordinate" in some way, like being in quotes after the ID for example. Resolving for now.

Sounds fine to leave this until the next PR, but please make the ID the primary identifier and if you want to show the label, make that appear "subordinate" in some way, like being in quotes after the ID for example. Resolving for now.
glen marked this conversation as resolved
glen reviewed 2026-05-28 17:35:49 +00:00
app-proto/src/assembly.rs
@ -208,3 +228,2 @@
Self::new(
id,
format!("Sphere {id_num}"),
"sphere".to_string(),
glen commented 2026-05-28 17:35:49 +00:00
Owner
Copy link

Is it a worry that multiple calls to the default() method for Sphere will produce entities with the same id()? Ultimately we never want there to be such an occurrence. Is there currently no provision to prevent such id collisions?

Is it a worry that multiple calls to the default() method for Sphere will produce entities with the same id()? Ultimately we never want there to be such an occurrence. Is there currently no provision to prevent such id collisions?
Vectornaut commented 2026-05-28 19:52:28 +00:00
Author
Member
Copy link

The default ID provided by Element::default is used as a base for the ID assigned by Assembly::insert_element_sequential, which appends a sequence number to prevent collisions.

In the current design of the command system, it's hard to check for ID collisions at the time when elements are created. This is because when we interpret a command, we create all the new elements it refers to before any of them are inserted into the assembly.

The next pull request will introduce an interpretation environment structure (Environment), which might make it easier to check for collisions at the time when elements are created. I'm not sure we'll want to do that, though. The current design, with insert_element_sequential, feels like a hack to me, but I also feel like it's too early to put a lot of effort into trying to come up with something better.

The default ID provided by `Element::default` is used as a base for the ID assigned by [`Assembly::insert_element_sequential`](https://code.studioinfinity.org/Vectornaut/dyna3/src/commit/4c9ddff53a8b8a05d2d375edd8f9f652b7151af1/app-proto/src/assembly.rs#L805-L828), which appends a sequence number to prevent collisions. In the current design of the command system, it's hard to check for ID collisions at the time when elements are created. This is because when we interpret a command, we create all the new elements it refers to before any of them are inserted into the assembly. The next pull request will introduce an interpretation environment structure (`Environment`), which might make it easier to check for collisions at the time when elements are created. I'm not sure we'll want to do that, though. The current design, with `insert_element_sequential`, feels like a hack to me, but I also feel like it's too early to put a lot of effort into trying to come up with something better.
glen marked this conversation as resolved
glen reviewed 2026-05-28 17:37:12 +00:00
app-proto/src/assembly.rs
@ -309,3 +331,2 @@
Self::new(
id,
format!("Point {id_num}"),
"point".to_string(),
glen commented 2026-05-28 17:37:12 +00:00
Owner
Copy link

Same question as for Spheres -- issue with always multiple ids "point"?

Same question as for Spheres -- issue with always multiple ids "point"?
Vectornaut commented 2026-05-28 19:53:06 +00:00
Author
Member
Copy link

Same answer as for Sphere.

[Same answer](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/138#issuecomment-3822) as for `Sphere`.
glen marked this conversation as resolved
glen reviewed 2026-05-28 17:42:37 +00:00
app-proto/src/assembly.rs
@ -458,3 +489,3 @@
let [row, col] = self.subjects.each_ref().map(
|subj| subj.column_index().expect(
indexing_error("Subject", subj.id(),
indexing_error("Subject", &subj.id().get_clone(),
glen commented 2026-05-28 17:42:37 +00:00
Owner
Copy link

I probably skipped over this same point a couple places above, but does get_clone produce a new signal? If so, it seems odd to me that an error message should need a signal rather than just the current value of the signal... Or am I misunderstanding?

I probably skipped over this same point a couple places above, but does get_clone produce a new _signal_? If so, it seems odd to me that an error message should need a signal rather than just the current value of the signal... Or am I misunderstanding?
Vectornaut commented 2026-05-28 20:04:38 +00:00
Author
Member
Copy link

When a signal's value type implements Clone, you can use get_clone or get_clone_untracked to get a clone of its current value, with or without reactive tracking. When a signal's value type also implements Copy, you can use get or get_untracked to get a copy of its current value. When a signal's value type doesn't even implement Clone, you can work with its current value using with and with_untracked.

When a signal's value type implements `Clone`, you can use [`get_clone`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.get_clone) or [`get_clone_untracked`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.get_clone_untracked) to get a clone of its current value, with or without reactive tracking. When a signal's value type also implements `Copy`, you can use [`get`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.get) or [`get_untracked`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.get_untracked) to get a copy of its current value. When a signal's value type doesn't even implement `Clone`, you can work with its current value using [`with`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.with) and [`with_untracked`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.with_untracked).
Vectornaut commented 2026-05-30 06:54:25 +00:00
Author
Member
Copy link

Following up on our discussion during yesterday's check-in: I've confirmed that this code should call get_clone_untracked rather than get_clone to avoid unwanted reactivity. It's corrected in the next pull request, where it would otherwise have caused a noticeable bug for the first time.

Following up on our discussion during yesterday's check-in: I've confirmed that this code should call `get_clone_untracked` rather than `get_clone` to avoid unwanted reactivity. It's corrected in the next pull request, where it would otherwise have caused a noticeable bug for the first time.
glen commented 2026-05-30 16:20:22 +00:00
Owner
Copy link

OK, I will merge despite this bug since you have a correction on its heels; let's get the next PR filed as soon as feasible.

OK, I will merge despite this bug since you have a correction on its heels; let's get the next PR filed as soon as feasible.
glen marked this conversation as resolved
glen reviewed 2026-05-28 17:44:29 +00:00
app-proto/src/assembly.rs
@ -571,3 +618,2 @@
let col = self.subject.column_index().expect(
indexing_error("Subject", &self.subject.id,
"point-coordinate regulator").as_str());
indexing_error(
glen commented 2026-05-28 17:44:29 +00:00
Owner
Copy link

Another comment about all of these error messages: it's a bit of a code smell when there are so many copies of the same code phrase. Is there any way to reduce the boilerplate here? Move this identical shared logic "up the tree", so to speak?

Another comment about all of these error messages: it's a bit of a code smell when there are so many copies of the same code phrase. Is there any way to reduce the boilerplate here? Move this identical shared logic "up the tree", so to speak?
Vectornaut commented 2026-05-30 07:05:32 +00:00
Author
Member
Copy link

Nice catch! I've filed issue #144 to propose a refactoring.

Nice catch! I've filed issue #144 to propose a refactoring.
glen marked this conversation as resolved
glen reviewed 2026-05-28 17:55:42 +00:00
app-proto/src/assembly.rs
@ -717,3 +813,3 @@
id_num += 1;
id = format!("{default_id}{id_num}");
id = format!("{id_base}{id_num}");
}
glen commented 2026-05-28 17:55:42 +00:00
Owner
Copy link

Just for the record, I'd suggest a sort of loop that sets the id and tries it inside the loop, so that the acceptable id is properly set once the loop is exited, so that the logic of converting the id_num to an id does not have to be repeated (creating the possibility that the two expressions would get out of sync by accident). No change necessary given the stack of changes hereafter, but please do watch out for this sort of non-DRY code pattern.

Just for the record, I'd suggest a sort of loop that sets the id and tries it inside the loop, so that the acceptable id is properly set once the loop is exited, so that the logic of converting the id_num to an id does not have to be repeated (creating the possibility that the two expressions would get out of sync by accident). No change necessary given the stack of changes hereafter, but please do watch out for this sort of non-DRY code pattern.
Vectornaut commented 2026-05-28 21:40:00 +00:00
Author
Member
Copy link

I'm reading this as a suggestion to make one or both of the following changes (in future code). Could you clarify whether you meant one, the other, or both?

  • Move the initialization

    let mut id = format!("{id_base}{id_num}");

    inside the trial loop, so that "the logic of converting the id_num to an id does not have to be repeated."

  • Move the assignment

    elt_rc.id().set_silent(id.clone());

    inside the trial loop, and do it on every loop step, so we can't accidentally remove it later. Note that this assignment does not repeat the logic of converting the id_num to an ID. The ID we assign is exactly the one for which elts_by_id.contains_key(&id) returns false, breaking us out of the loop.

I'm reading this as a suggestion to make one or both of the following changes (in future code). Could you clarify whether you meant one, the other, or both? - Move the initialization ```rust let mut id = format!("{id_base}{id_num}"); ``` inside the trial loop, so that "the logic of converting the id_num to an id does not have to be repeated." - Move the assignment ```rust elt_rc.id().set_silent(id.clone()); ``` inside the trial loop, and do it on every loop step, so we can't accidentally remove it later. Note that this assignment does _not_ repeat the logic of converting the `id_num` to an ID. The ID we assign is exactly the one for which `elts_by_id.contains_key(&id)` returns `false`, breaking us out of the loop.
glen commented 2026-05-28 21:50:19 +00:00
Owner
Copy link

I meant only the first of these. I think you'll find it best in the long run not to have two lines of code that need to match for the code to really make sense, even when the lines are close to each other.

I don't see any reason to reassign the id of the element multiple times as you try to find a fresh id; setting it once after the loop indeed seems better. Resolving.

I meant only the first of these. I think you'll find it best in the long run not to have two lines of code that need to match for the code to really make sense, even when the lines are close to each other. I don't see any reason to reassign the id of the element multiple times as you try to find a fresh id; setting it once after the loop indeed seems better. Resolving.
glen marked this conversation as resolved
glen reviewed 2026-05-28 18:00:12 +00:00
app-proto/src/components/add_remove.rs
@ -8,4 +14,4 @@
};
#[component]
pub fn AddRemove() -> View {
glen commented 2026-05-28 18:00:12 +00:00
Owner
Copy link

I realize that this is not a change in this PR, but I am confused about the name of the AddRemove component -- there is not actually any provision for removing anything from an assembly at the moment? If not, then the next touch on this source file should probably either add a removal ability or rename it to just be about adding. Again, no change needed now, and I understand that maybe the plan is to put removal here, but plans have a way of changing and it seems better to have things consistent in the meantime. Or maybe I am mistaken and there is some facility for removal?

I realize that this is not a change in this PR, but I am confused about the name of the AddRemove component -- there is not actually any provision for removing anything from an assembly at the moment? If not, then the next touch on this source file should probably either add a removal ability or rename it to just be about adding. Again, no change needed now, and I understand that maybe the plan is to put removal here, but plans have a way of changing and it seems better to have things consistent in the meantime. Or maybe I am mistaken and there is some facility for removal?
Vectornaut commented 2026-05-28 21:22:07 +00:00
Author
Member
Copy link

Yes, I named the component AddRemove because I intend for it to eventually include a removal button. You're correct that we don't yet have any way to remove elements or regulators.

If not, then the next touch on this source file should probably either add a removal ability or rename it to just be about adding.

Fine with me. Keep in mind, though, that if we rename AddRemove, we'll also have to edit main.rs. We'll have to edit main.css too if we want the corresponding DOM element ID to be consistently named.

If not, then the next touch on this source file should probably either add a removal ability […]

One heads-up: since the command language doesn't have any "verbs" yet, adding a removal command would involve adding new syntax to the command language. Of course, we can always add removal actions without corresponding commands, use those actions in removal buttons and keyboard shortcuts, and file an issue to make those actions accessible through the command prompt later.

Yes, I named the component `AddRemove` because I intend for it to eventually include a removal button. You're correct that we don't yet have any way to remove elements or regulators. > If not, then the next touch on this source file should probably either add a removal ability or rename it to just be about adding. Fine with me. Keep in mind, though, that if we rename `AddRemove`, we'll also have to edit `main.rs`. We'll have to edit `main.css` too if we want the corresponding DOM element ID to be consistently named. > If not, then the next touch on this source file should probably either add a removal ability […] One heads-up: since the command language doesn't have any "verbs" yet, adding a removal command would involve adding new syntax to the command language. Of course, we can always add removal actions without corresponding commands, use those actions in removal buttons and keyboard shortcuts, and file an issue to make those actions accessible through the command prompt later.
glen commented 2026-05-28 21:42:00 +00:00
Owner
Copy link

Fine with me. Keep in mind, though, that if we rename AddRemove, we'll also have to edit main.rs. We'll have to edit main.css too if we want the corresponding DOM element ID to be consistently named.

All fine.

Of course, we can always add removal actions without corresponding commands, use those actions in removal buttons and keyboard shortcuts, and file an issue to make those actions accessible through the command prompt later.

Please let's not -- I very much want to maintain one-one parity between commands and actions, so that everything that occurs can show up in the command listener. So every action should be the possible result of a command, and of course vice-versa. (Not every action neccessarily has to be available via some shortcut or via some menu item, though.)

I would mildly disagree that the command language has no verbs; regulator @ 0.5 is implicitly a verb -- it could have been written set regulator to 0.5.

I doubt that a syntax like this one for @ is appropriate for most verbs, though. So I agree, the most straghtforward sort of a command for deleting an entity would be just an imperative delete regulator. And yes, the language will have to be able to parse such things and we will have to worry about whether there could be an entity with id delete and so on -- but that's life.

Closing this conversation for the sake of this PR, as it was just prospective.

> Fine with me. Keep in mind, though, that if we rename AddRemove, we'll also have to edit main.rs. We'll have to edit main.css too if we want the corresponding DOM element ID to be consistently named. All fine. > Of course, we can always add removal actions without corresponding commands, use those actions in removal buttons and keyboard shortcuts, and file an issue to make those actions accessible through the command prompt later. Please let's not -- I very much want to maintain one-one parity between commands and actions, so that everything that occurs can show up in the command listener. So every action should be the possible result of a command, and of course vice-versa. (Not every action neccessarily has to be available via some shortcut or via some menu item, though.) I would mildly disagree that the command language has no verbs; `regulator @ 0.5` is implicitly a verb -- it could have been written `set regulator to 0.5`. I doubt that a syntax like this one for `@` is appropriate for most verbs, though. So I agree, the most straghtforward sort of a command for deleting an entity would be just an imperative `delete regulator`. And yes, the language will have to be able to parse such things and we will have to worry about whether there could be an entity with id `delete` and so on -- but that's life. Closing this conversation for the sake of this PR, as it was just prospective.
glen commented 2026-05-28 21:44:51 +00:00
Owner
Copy link

P.S. if there was a strong reason for preferring a syntax like regulator delete that had the operand(s) first followed by the verb, that could be OK, but absent any such reason, the pseudo-English imperative word order delete regulator is better

P.S. if there was a strong reason for preferring a syntax like `regulator delete` that had the operand(s) first followed by the verb, that could be OK, but absent any such reason, the pseudo-English imperative word order `delete regulator` is better
glen marked this conversation as resolved
glen reviewed 2026-05-28 18:07:20 +00:00
app-proto/src/components/add_remove.rs
@ -66,1 +33,3 @@
}
ParametricActorButton(
parameter = state.selection_receivers.map(
/* TO DO */
glen commented 2026-05-28 18:07:20 +00:00
Owner
Copy link

On our call today, let's quickly go over the selection_receivers mechanism -- I just don't have enough of a grasp of Rust yet to decipher the fairly intricate typing/closures/whatever is going on here... Thanks!

On our call today, let's quickly go over the selection_receivers mechanism -- I just don't have enough of a grasp of Rust yet to decipher the fairly intricate typing/closures/whatever is going on here... Thanks!
glen marked this conversation as resolved
glen commented 2026-05-28 18:10:03 +00:00
Owner
Copy link

OK, I have finished going through the code. I don't think any of my comments require a code change at this point, unless you are motivated by any of them to do so. I think some of them may involve scheduling future code tweaks, but I have tried to be cognizant of the fact that you have code stacked on top of this. So I think you can generally just respond to/acknowledge the comments and then we can merge. (Well, I will also give it a spin and run the tests, but I presume as you have been using this for some time that all that will go fine.) Thanks!

OK, I have finished going through the code. I don't think any of my comments require a code change at this point, unless you are motivated by any of them to do so. I think some of them may involve scheduling future code tweaks, but I have tried to be cognizant of the fact that you have code stacked on top of this. So I think you can generally just respond to/acknowledge the comments and then we can merge. (Well, I will also give it a spin and run the tests, but I presume as you have been using this for some time that all that will go fine.) Thanks!
glen commented 2026-05-28 19:18:41 +00:00
Owner
Copy link

OK, it seems to run fine. Once this PR is merged, please file issue(s) for the following lack-of-feedback situations:

  1. If I type "sphere" in the command listener and hit enter, there is no appearance of anything happening. There should be at least some acknowledgment that I tried to enter a command and it was not valid.
  2. If I select two spheres (say) and their little arrows happen to be rotated to point right in the outline (i.e., constraints hidden), then when I hit Shift-R there is no indication that anything has happened, even though in fact the inversive distance regulator between them was created. There needs to be some apparent action.

The second item raises the question on whether there should be any feedback on unbound keys. I think as long as bound keys reliably produce feedback that something occurred, there need not be any feedback for unbound keys, so no action item on that unless you have a different view on this point.

And then there is the question of what sort of keys we should use for shortcuts. I believe that generally speaking the most commonly used default shortcuts should not need any modifiers. So if "create inversive distance regulator" is going to be a basic standard shortcut and it is going to be on the letter r, I would recommend that the shortcut be just plain (i.e. lowercase) "r" rather than "R" requiring Shift-R to invoke. Of course, that's really a future issue (I sort of doubt this will end up on the letter "r" in the end anyway), so please just create a wiki item or issue on shortcut design. Thanks!

OK, it seems to run fine. Once this PR is merged, please file issue(s) for the following lack-of-feedback situations: 1) If I type "sphere" in the command listener and hit enter, there is no appearance of anything happening. There should be at least some acknowledgment that I tried to enter a command and it was not valid. 2) If I select two spheres (say) and their little arrows happen to be rotated to point right in the outline (i.e., constraints hidden), then when I hit Shift-R there is no indication that anything has happened, even though in fact the inversive distance regulator between them was created. There needs to be some apparent action. The second item raises the question on whether there should be any feedback on unbound keys. I think as long as bound keys reliably produce feedback that something occurred, there need not be any feedback for unbound keys, so no action item on that unless you have a different view on this point. And then there is the question of what sort of keys we should use for shortcuts. I believe that generally speaking the most commonly used default shortcuts should not need any modifiers. So if "create inversive distance regulator" is going to be a basic standard shortcut and it is going to be on the letter r, I would recommend that the shortcut be just plain (i.e. lowercase) "r" rather than "R" requiring Shift-R to invoke. Of course, that's really a future issue (I sort of doubt this will end up on the letter "r" in the end anyway), so please just create a wiki item or issue on shortcut design. Thanks!
glen commented 2026-05-28 19:21:47 +00:00
Owner
Copy link

OK, the examples and tests compile and run without error, so once we've resolved all the above conversation points this will be ready to merge. As I said, I suspect that can happen without any code changes except any you might be motivated to do by the comments. Thanks so much!

OK, the examples and tests compile and run without error, so once we've resolved all the above conversation points this will be ready to merge. As I said, I suspect that can happen without any code changes except any you might be motivated to do by the comments. Thanks so much!
Vectornaut commented 2026-05-28 21:49:22 +00:00
Author
Member
Copy link

Once this PR is merged, please file issue(s) for the following lack-of-feedback situations

Both of these situations would be resolved if we addressed the interface enhancement issue #139. Should I give them their own issues anyway, since we could also resolve them without addressing #139?

> Once this PR is merged, please file issue(s) for the following lack-of-feedback situations Both of these situations would be resolved if we addressed the interface enhancement issue #139. Should I give them their own issues anyway, since we could also resolve them without addressing #139?
glen commented 2026-05-28 21:56:05 +00:00
Owner
Copy link

@Vectornaut wrote in #138 (comment):

Once this PR is merged, please file issue(s) for the following lack-of-feedback situations

Both of these situations would be resolved if we addressed the interface enhancement issue #139. Should I give them their own issues anyway, since we could also resolve them without addressing #139?

Excellent question. I think it's best for them to have their own issue, as it isn't necessarily clear if updating the command listener entirely suffices for feedback: we definitely want in the future the ability to hide/show/possibly rearrange the various views (e.g. have two distinct graphical views open, maybe not have the outline view, etc.). Such configurability might encompass not having the command listener visible. So if that's the only source of feedback, just updating the command history might not be enough feedback.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/138#issuecomment-3830: > > Once this PR is merged, please file issue(s) for the following lack-of-feedback situations > > Both of these situations would be resolved if we addressed the interface enhancement issue #139. Should I give them their own issues anyway, since we could also resolve them without addressing #139? Excellent question. I think it's best for them to have their own issue, as it isn't necessarily clear if updating the command listener entirely suffices for feedback: we definitely want in the future the ability to hide/show/possibly rearrange the various views (e.g. have two distinct graphical views open, maybe not have the outline view, etc.). Such configurability might encompass not having the command listener visible. So if that's the only source of feedback, just updating the command history might not be enough feedback.
Vectornaut commented 2026-05-28 23:02:42 +00:00
Author
Member
Copy link

And then there is the question of what sort of keys we should use for shortcuts. […] Of course, that's really a future issue (I sort of doubt this will end up on the letter "r" in the end anyway), so please just create a wiki item or issue on shortcut design.

I've added a section on keyboard controls to the "Interface" wiki page! (I also tidied the page up a bit.)

> And then there is the question of what sort of keys we should use for shortcuts. […] Of course, that's really a future issue (I sort of doubt this will end up on the letter "r" in the end anyway), so please just create a wiki item or issue on shortcut design. I've added a section on [keyboard controls](wiki/Interface#keyboard-controls) to the "Interface" wiki page! (I also tidied the page up a bit.)
glen commented 2026-05-28 23:39:28 +00:00
Owner
Copy link

@Vectornaut wrote in #138 (comment):

I've added a section on keyboard controls to the "Interface" wiki page! (I also tidied the page up a bit.)

Looks great.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/138#issuecomment-3835: > I've added a section on [keyboard controls](/StudioInfinity/dyna3/wiki/Interface#keyboard-controls) to the "Interface" wiki page! (I also tidied the page up a bit.) Looks great.
👍 1
Vectornaut commented 2026-05-30 02:35:21 +00:00
Author
Member
Copy link

I think it's best for [these lack-of-feedback situations] to have their own issue, as it isn't necessarily clear if updating the command listener entirely suffices for feedback: we definitely want in the future the ability to hide/show/possibly rearrange the various views (e.g. have two distinct graphical views open, maybe not have the outline view, etc.).

Done, through issues #142 and #143! I've written that #142 could be resolved by addressing #139, because I think the command listener will necessarily be visible and salient when someone is entering a command. In contrast, I've emphasized that efforts to address #143 shouldn't depend on a particular view being visible, as you've pointed out here.

> I think it's best for [[these](#issuecomment-3819) lack-of-feedback situations] to have their own issue, as it isn't necessarily clear if updating the command listener entirely suffices for feedback: we definitely want in the future the ability to hide/show/possibly rearrange the various views (e.g. have two distinct graphical views open, maybe not have the outline view, etc.). Done, through issues #142 and #143! I've written that #142 could be resolved by addressing #139, because I think the command listener will necessarily be visible and salient when someone is entering a command. In contrast, I've emphasized that efforts to address #143 shouldn't depend on a particular view being visible, as you've pointed out here.
Vectornaut commented 2026-05-30 07:09:52 +00:00
Author
Member
Copy link

I think I've now addressed all the review questions and filed all of the issues on our to-do list. If you don't see anything else to do, the branch should be ready to merge.

I think I've now addressed all the review questions and filed all of the issues on our to-do list. If you don't see anything else to do, the branch should be ready to merge.
glen commented 2026-05-30 16:28:37 +00:00
Owner
Copy link

OK, merging in the interest of moving on to the next, more polished PR.

OK, merging in the interest of moving on to the next, more polished PR.
glen merged commit bbc76c2797 into main 2026-05-30 16:29:44 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something is not working

  
design
Issue that entails architecture/design decisions

  
duplicate
This issue or pull request already exists

  
engine
Issue concerning the engine

  
enhancement
New feature

  
maintenance
Refactors andother behavior-orthogonal tasks

  
prospective
This will become an issue once a particular feature is implemented or pull request is merged

  
question
More information is needed

  
regression
A bug that's resurfaced or a feature that's accidentally been removed

  
stub
We need to fill in the details of this issue

  
todo
Planned step in implementation

  
ui
Issue concerning the user interface

  
wontfix
This won't be fixed

No labels
bug
design
duplicate
engine
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
StudioInfinity/dyna3!138
No description provided.