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

Generalize constraints to observables #48

Merged
glen merged 25 commits from Vectornaut/dyna3:observables_on_main into main 2025-03-10 23:43:25 +00:00
Conversation 69 Commits 25 Files changed 7 +87 -87
4 changed files with 87 additions and 87 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 24139ad5e9 - Show all commits

20
app-proto/main.css
View file

@ -78,12 +78,12 @@ summary.selected {
background-color: var(--selection-highlight);
}
summary > div, .observable {
summary > div, .regulator {
padding-top: 4px;
padding-bottom: 4px;
}
.element, .observable {
.element, .regulator {
display: flex;
flex-grow: 1;
padding-left: 8px;
@ -108,7 +108,7 @@ details[open]:has(li) .element-switch::after {
flex-grow: 1;
}
.observable-label {
.regulator-label {
flex-grow: 1;
}
@ -124,37 +124,37 @@ details[open]:has(li) .element-switch::after {
width: 56px;
}
.observable {
.regulator {
font-style: italic;
}
.observable-type {
.regulator-type {
padding: 2px 8px 0px 8px;
font-size: 10pt;
}
.observable.invalid-constraint {
.regulator.invalid-constraint {
color: var(--text-invalid);
}
.observable > input {
.regulator > input {
color: inherit;
background-color: inherit;
border: 1px solid var(--border);
border-radius: 2px;
}
.observable > input::placeholder {
.regulator > input::placeholder {
color: inherit;
opacity: 54%;
font-style: italic;
}
.observable.valid-constraint > input {
.regulator.valid-constraint > input {
background-color: var(--display-background);
}
.observable.invalid-constraint > input {
.regulator.invalid-constraint > input {
border-color: var(--border-invalid);
}

26
app-proto/src/add_remove.rs
View file

@ -6,8 +6,8 @@ use crate::{
AppState,
assembly::{
Assembly,
glen marked this conversation as resolved
glen commented 2025-03-08 00:23:06 +00:00
Review
Copy link

In assembly.rs, you put all of the sub-namespaces under a top-level item on a single line, like

    assembly::{Assembly, Element}

whereas here this has been spread across three lines. They should be consistent in format. If it's the same to you, I prefer a formats that balances the desire for fewer linebreaks (to keep a good amount of information visible on screen at one time) with the need for clear organization and readability (which too few linebreaks can engender). That is, I prefer the assembly.rs format.

In assembly.rs, you put all of the sub-namespaces under a top-level item on a single line, like ``` assembly::{Assembly, Element} ``` whereas here this has been spread across three lines. They should be consistent in format. If it's the same to you, I prefer a formats that balances the desire for fewer linebreaks (to keep a good amount of information visible on screen at one time) with the need for clear organization and readability (which too few linebreaks can engender). That is, I prefer the assembly.rs format.
Vectornaut commented 2025-03-08 23:30:50 +00:00
Review
Copy link

In assembly.rs, you put all of the sub-namespaces under a top-level item on a single line […], whereas here this has been spread across three lines. They should be consistent in format.

Good catch. My convention has been to start with each use declaration on one line. If that line gets too long, I switch the outermost braced list from space-separated to newline-separated. Then I do the same thing recursively at lower list levels. The use declarations you noticed break this convention; I've corrected them In commit b9db7a5. I think the convention is followed everywhere else, but I'll keep an eye on it whenever I revise use declarations in future commits.

> In assembly.rs, you put all of the sub-namespaces under a top-level item on a single line […], whereas here this has been spread across three lines. They should be consistent in format. Good catch. My convention has been to start with each `use` declaration on one line. If that line gets too long, I switch the outermost braced list from space-separated to newline-separated. Then I do the same thing recursively at lower list levels. The `use` declarations you noticed break this convention; I've corrected them In commit b9db7a5. I think the convention is followed everywhere else, but I'll keep an eye on it whenever I revise `use` declarations in future commits.
Observable,
ObservableRole,
Regulator,
RegulatorRole,
Element
},
engine::Q
@ -210,8 +210,8 @@ pub fn AddRemove() -> View {
}
);
let desired = create_signal(0.0);
let role = create_signal(ObservableRole::Measurement);
state.assembly.insert_observable(Observable {
let role = create_signal(RegulatorRole::Measurement);
state.assembly.insert_regulator(Regulator {
subjects: subjects,
measured: measured,
desired: desired,
@ -221,22 +221,22 @@ pub fn AddRemove() -> View {
state.selection.update(|sel| sel.clear());
/* DEBUG */
// print updated observable list
console::log_1(&JsValue::from("Observables:"));
state.assembly.observables.with(|obsls| {
for (_, obs) in obsls.into_iter() {
// print updated regulator list
console::log_1(&JsValue::from("Regulators:"));
state.assembly.regulators.with(|regs| {
for (_, reg) in regs.into_iter() {
console::log_5(
&JsValue::from(" "),
&JsValue::from(obs.subjects.0),
&JsValue::from(obs.subjects.1),
&JsValue::from(reg.subjects.0),
&JsValue::from(reg.subjects.1),
&JsValue::from(":"),
&JsValue::from(obs.desired.get_untracked())
&JsValue::from(reg.desired.get_untracked())
);
}
});
// update the realization when the observable becomes
// constrained, or is edited while constrained
// update the realization when the regulator becomes
// a constraint, or is edited while acting as a constraint
create_effect(move || {
console::log_1(&JsValue::from(
format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)

54
app-proto/src/assembly.rs
View file

@ -7,9 +7,9 @@ use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */
use crate::engine::{realize_gram, local_unif_to_std, ConfigSubspace, PartialMatrix};
// the types of the keys we use to access an assembly's elements and observables
// the types of the keys we use to access an assembly's elements and regulators
pub type ElementKey = usize;
pub type ObservableKey = usize;
pub type RegulatorKey = usize;
pub type ElementColor = [f32; 3];
@ -26,7 +26,7 @@ pub struct Element {
pub label: String,
pub color: ElementColor,
pub representation: Signal<DVector<f64>>,
pub observables: Signal<BTreeSet<ObservableKey>>,
pub regulators: Signal<BTreeSet<RegulatorKey>>,
// a serial number, assigned by `Element::new`, that uniquely identifies
// each element
@ -61,7 +61,7 @@ impl Element {
label: label,
color: color,
representation: create_signal(representation),
observables: create_signal(BTreeSet::default()),
regulators: create_signal(BTreeSet::default()),
serial: serial,
column_index: None
}
@ -111,30 +111,30 @@ impl Element {
}
}
pub enum ObservableRole {
pub enum RegulatorRole {
Measurement,
Constraint(bool)
}
impl ObservableRole {
impl RegulatorRole {
pub fn is_valid_constraint(&self) -> bool {
glen marked this conversation as resolved Outdated
glen commented 2025-02-25 23:49:20 +00:00
Outdated
Review
Copy link

Tiny writing nit: this is a "garden path" phrasing -- I read "format discussed above" and started looking for the format specification earlier in the file, since I was pretty sure I hadn't seen one. Maybe reword just a bit to avoid that trap I fell into?

Tiny writing nit: this is a "garden path" phrasing -- I read "format discussed above" and started looking for the format specification earlier in the file, since I was pretty sure I hadn't seen one. Maybe reword just a bit to avoid that trap I fell into?
Vectornaut commented 2025-02-27 08:34:08 +00:00
Outdated
Review
Copy link

Nice catch! I've changed "above" to "at" to resolve the ambiguity (7cbd926). Does that read all right to you?

Nice catch! I've changed "above" to "at" to resolve the ambiguity (7cbd926). Does that read all right to you?
glen commented 2025-03-01 01:32:09 +00:00
Outdated
Review
Copy link

I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.

I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.
match self {
ObservableRole::Measurement => false,
ObservableRole::Constraint(valid) => *valid
RegulatorRole::Measurement => false,
RegulatorRole::Constraint(valid) => *valid
}
}
}
#[derive(Clone)]
pub struct Observable {
pub struct Regulator {
pub subjects: (ElementKey, ElementKey),
pub measured: ReadSignal<f64>,
pub desired: Signal<f64>,
pub desired_text: Signal<String>,
pub role: Signal<ObservableRole>
pub role: Signal<RegulatorRole>
}
impl Observable {
impl Regulator {
fn role_is_valid_constraint_untracked(&self) -> bool {
self.role.with_untracked(|role| role.is_valid_constraint())
}
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:01:30 +00:00
Outdated
Review
Copy link

Note here we are baking into this design the notion that the only specification of an Absent SpecifiedValue that we ever care to remember is ''. Is that OK? I worry in particular that when we want to allow disabling while remembering the un-disabled spec, this could bite us. Or maybe in that situation, the bit as to whether a regulator is active goes into the Regulator, and an inactive Regulator with a Present SpecifiedValue represents that state. So this is probably OK, but thought I'd just raise the point before we merge this.

Note here we are baking into this design the notion that the only specification of an Absent SpecifiedValue that we ever care to remember is `''`. Is that OK? I worry in particular that when we want to allow disabling while remembering the un-disabled spec, this could bite us. Or maybe in that situation, the bit as to whether a regulator is active goes into the Regulator, and an inactive Regulator with a Present SpecifiedValue represents that state. So this is probably OK, but thought I'd just raise the point before we merge this.
Vectornaut commented 2025-02-27 08:34:49 +00:00
Outdated
Review
Copy link

I definitely think we should give the Absent variant a specification field if we end up with more than one way to specify absence. However, I'm not sure it makes sense to add that field now. It feels weird to have a field whose value should always be an empty string.

I definitely think we should give the `Absent` variant a specification field if we end up with more than one way to specify absence. However, I'm not sure it makes sense to add that field now. It feels weird to have a field whose value should always be an empty string.
glen commented 2025-03-01 01:36:18 +00:00
Outdated
Review
Copy link

I think it would be weirder to have

pub enum SpecifiedValue {
    Absent(absentSpec: String),
    Present {
        spec: String,
        value: f64
    }
}

-- I mean, the spec should be the spec and stored in the same place whether it corresponds to an absent or a present SpecifiedValue. So if you think that's likely where we're headed, we should redesign this now, shouldn't we? Thoughts?

I think it would be weirder to have ``` pub enum SpecifiedValue { Absent(absentSpec: String), Present { spec: String, value: f64 } } ``` -- I mean, the spec should be the spec and stored in the same place whether it corresponds to an absent or a present SpecifiedValue. So if you think that's likely where we're headed, we should redesign this now, shouldn't we? Thoughts?
Vectornaut commented 2025-03-01 02:26:45 +00:00
Outdated
Review
Copy link

To me, it seems perfectly natural to have this:

pub enum SpecifiedValue {
    Absent(String),
    Present {
        spec: String,
        value: f64
    }
}

An absent SpecifiedValue has only a specification. A present SpecifiedValue has both a specification and a value. The specification is always stored in the same place. When I write something like

if let Ok(spec_val) = SpecifiedValue::try_from(my_spec) {
    print!("{}", spec_val.spec());
}

the string my_spec is stored in spec_val and then returned by spec_val.spec(), regardless of whether spec_val is Absent or Present.

I thought the main point of the SpecifiedValue type was to store a specification together with all the value data derived from it, making it easy to ensure that the specification and the data always correspond. It's harder to do that if a SpecifiedValue doesn't carry its specification along with it.

To me, it seems perfectly natural to have this: ```rust pub enum SpecifiedValue { Absent(String), Present { spec: String, value: f64 } } ``` An absent `SpecifiedValue` has only a specification. A present `SpecifiedValue` has both a specification and a value. The specification is always stored in the same place. When I write something like ``` if let Ok(spec_val) = SpecifiedValue::try_from(my_spec) { print!("{}", spec_val.spec()); } ``` the string `my_spec` is stored in `spec_val` and then returned by `spec_val.spec()`, regardless of whether `spec_val` is `Absent` or `Present`. I thought the main point of the `SpecifiedValue` type was to store a specification together with all the value data derived from it, making it easy to ensure that the specification and the data always correspond. It's harder to do that if a `SpecifiedValue` doesn't carry its specification along with it.
glen commented 2025-03-01 02:46:26 +00:00
Outdated
Review
Copy link

Wow, I really don't understand why this detail of our design/implementation continues to be such a sticking point for us. It seems like it really should be simple:

I thought the main point of the SpecifiedValue type was to store a specification together with all the value data derived from it, making it easy to ensure that the specification and the data always correspond.

We totally agree on this. So what is the best, clearest, simplest, easiest-to-use design of the SpecifiedValue type that embodies this principle? Feel free to start again from scratch if need be, I want to get this right.

It's harder to do that if a SpecifiedValue doesn't carry its specification along with it.

We agree on this, too. Of course a SpecifiedValue should carry its specification with it -- it should be its primary data.

The specification is always stored in the same place.

But to me this is patently not true in your corrected version of this potential way of implementing a SpecifiedValue with multiple ways of being Absent (sorry, I forgot tuple fields can't be named). In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the spec field of the payload. Two different places for the "same" string -- to my eyes, very disorienting.

Since SpecifiedValue is moving to a separate file anyway, should it be entirely opaque data-wise and have a method-only interface (and not be matchable upon, for example, in case at some point the best implementation isn't an enum)? The methods would I guess be something like is_present() and spec() and value(), with the latter I suppose returning NaN in case is_present() is false? Or perhaps value() panics if you call it when is_present() is false? That design change would immediately make the following conversation, about whether to have an is_present() method or use matching, moot and resolvable, because there would only be the methods.

As always, looking forward to your thoughts. And I do truly apologize (and am puzzled) that something seemingly so simple is being so recalcitrant of a mutually satisfying design/implementation.

Wow, I really don't understand why this detail of our design/implementation continues to be such a sticking point for us. It seems like it really should be simple: > I thought the main point of the SpecifiedValue type was to store a specification together with all the value data derived from it, making it easy to ensure that the specification and the data always correspond. We totally agree on this. So what is the best, clearest, simplest, easiest-to-use design of the SpecifiedValue type that embodies this principle? Feel free to start again from scratch if need be, I want to get this right. > It's harder to do that if a SpecifiedValue doesn't carry its specification along with it. We agree on this, too. Of course a SpecifiedValue should carry its specification with it -- it should be its _primary_ data. > The specification is always stored in the same place. But to me this is patently not true in your corrected version of this potential way of implementing a SpecifiedValue with multiple ways of being Absent (sorry, I forgot tuple fields can't be named). In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the `spec` field of the payload. Two different places for the "same" string -- to my eyes, very disorienting. Since SpecifiedValue is moving to a separate file anyway, should it be entirely opaque data-wise and have a method-only interface (and _not_ be matchable upon, for example, in case at some point the best implementation isn't an enum)? The methods would I guess be something like is_present() and spec() and value(), with the latter I suppose returning `NaN` in case is_present() is false? Or perhaps value() panics if you call it when is_present() is false? That design change would immediately make the following conversation, about whether to have an is_present() method or use matching, moot and resolvable, because there would only be the methods. As always, looking forward to your thoughts. And I do truly apologize (and am puzzled) that something seemingly so simple is being so recalcitrant of a mutually satisfying design/implementation.
Vectornaut commented 2025-03-01 03:09:57 +00:00
Outdated
Review
Copy link

In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the spec field of the payload.

If you find it less disorienting, we could do this:

pub enum SpecifiedValue {
    Absent {
        spec: String
    },
    Present {
        spec: String,
        value: f64
    }
}

Now both variants have a field called spec, and the Present variant also has a field called value. This would make match arms that handle Absent and Present values look more similar.

To access the specification of a general SpecifiedValue, we'd still need to call the spec() method, which does the matching internally. As far as I know, different variants of an enum are always handled separately in the end. We can hide the separate handling within methods of the enum, but we can't get rid of it.

The answers to this StackExchange question discuss various approaches to creating data types that have multiple variants with common fields.

  • The current Proposal 1b approach, which uses the spec() method to access the specification field across all variants, is similar in spirit to this answer.
  • When I was implementing Proposal 1a, I tried implementing the Deref trait for OptionalSpecifiedValue, kind of like in this answer. I switched to Proposal 1b before getting the trait working, though.
> In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the spec field of the payload. If you find it less disorienting, we could do this: ```rust pub enum SpecifiedValue { Absent { spec: String }, Present { spec: String, value: f64 } } ``` Now both variants have a field called `spec`, and the `Present` variant also has a field called `value`. This would make `match` arms that handle `Absent` and `Present` values look more similar. To access the specification of a general `SpecifiedValue`, we'd still need to call the `spec()` method, which does the matching internally. As far as I know, different variants of an enum are always handled separately in the end. We can hide the separate handling within methods of the enum, but we can't get rid of it. The answers to [this StackExchange question](https://stackoverflow.com/questions/49186751/sharing-a-common-value-in-all-enum-values) discuss various approaches to creating data types that have multiple variants with common fields. - The current Proposal 1b approach, which uses the `spec()` method to access the specification field across all variants, is similar in spirit to [this answer](https://stackoverflow.com/a/77559109). - When I was implementing Proposal 1a, I tried implementing the `Deref` trait for `OptionalSpecifiedValue`, kind of like in [this answer](https://stackoverflow.com/a/67467313). I switched to Proposal 1b before getting the trait working, though.
glen commented 2025-03-01 03:34:51 +00:00
Outdated
Review
Copy link
  • Your thoughts on just a method-only interface with an opaque underlying datatype that we can iterate on if we see fit without changing any client code? Maybe our real sticking point is that the proposals so far are exposing too much of the implementation? Given this enduring debate, I am warming to such a way of ending it: I think we really ought to be able to agree on what such an interface would be. Then the data layout of the underlying implementation is much less critical -- basically anything reasonable that supports the interface.

The current Proposal 1b approach, which uses the spec() method to access the specification field across all variants, is similar in spirit to this answer.

Hmm; it would seem to me that something like

struct SpecifiedValue {
   spec: String,
   value: Option<f64>
}

would be much closer to the spirit of that answer. There is always a spec, that's the primary data, and there may be a value, if the spec is a "Present" spec. Then we just need to make it clear what is the primary location/means of testing whether a SpecifiedValue is "present". I realize this may be full-circle...

* Your thoughts on just a method-only interface with an opaque underlying datatype that we can iterate on if we see fit without changing any client code? Maybe our real sticking point is that the proposals so far are exposing too much of the implementation? Given this enduring debate, I am warming to such a way of ending it: I think we really _ought_ to be able to agree on what such an interface would be. Then the data layout of the underlying implementation is _much_ less critical -- basically anything reasonable that supports the interface. > The current Proposal 1b approach, which uses the spec() method to access the specification field across all variants, is similar in spirit to this answer. Hmm; it would seem to me that something like ``` struct SpecifiedValue { spec: String, value: Option<f64> } ``` would be much closer to the spirit of that answer. There is always a spec, that's the primary data, and there may be a value, if the spec is a "Present" spec. Then we just need to make it clear what is **the** primary location/means of testing whether a SpecifiedValue is "present". I realize this may be full-circle...
Vectornaut commented 2025-03-03 23:41:23 +00:00
Outdated
Review
Copy link

Your thoughts on just a method-only interface with an opaque underlying datatype that we can iterate on if we see fit without changing any client code?

At this point, I can't predict whether that would make things cleaner or messier. I'd want value, and other methods that fetch derived data, to fail gracefully when the set point is absent—for example, by having an Option return type and returning None when the set point is absent. This might make it annoying to write client code that uses several pieces of derived data at once, but we don't have that problem yet, since there's only one piece of derived data.

Hmm; it would seem to me that something like

struct SpecifiedValue {
    spec: String,
    value: Option<f64>
}

would be much closer to the spirit of that answer. […] I realize this may be full-circle...

I agree that this—let's call it Proposal 1c—would be close to the original Proposal 1. The big differences I see would be:

  • All the specified value data would be encapsulated in the SpecifiedValue type, instead of being loose in the regulator.
  • When we only want to know whether the value is present, we'd call a method, which might internally check whether an arbitrary piece of derived data is None or Some(_).

By the way, I think it's likely that we'll revisit the specified value data type when we generalize from decimal numbers to more generalized expressions. At that point, we'll probably be using specified values more, so we'll have a better idea of what we want from them.

> Your thoughts on just a method-only interface with an opaque underlying datatype that we can iterate on if we see fit without changing any client code? At this point, I can't predict whether that would make things cleaner or messier. I'd want `value`, and other methods that fetch derived data, to fail gracefully when the set point is absent—for example, by having an `Option` return type and returning `None` when the set point is absent. This might make it annoying to write client code that uses several pieces of derived data at once, but we don't have that problem yet, since there's only one piece of derived data. > Hmm; it would seem to me that something like > > ``` > struct SpecifiedValue { > spec: String, > value: Option<f64> > } > ``` > > would be much closer to the spirit of that answer. […] I realize this may be full-circle... I agree that this—let's call it Proposal 1c—would be close to the original Proposal 1. The big differences I see would be: - All the specified value data would be encapsulated in the `SpecifiedValue` type, instead of being loose in the regulator. - When we only want to know whether the value is present, we'd call a method, which might internally check whether an arbitrary piece of derived data is `None` or `Some(_)`. By the way, I think it's likely that we'll revisit the specified value data type when we generalize from decimal numbers to more generalized expressions. At that point, we'll probably be using specified values more, so we'll have a better idea of what we want from them.
Vectornaut commented 2025-03-04 07:36:34 +00:00
Outdated
Review
Copy link

I've switched to Proposal 1c (84bfdef), which we adopted during today's meeting. The SpecifiedValue structure is read-only, courtesy of the readonly crate, so nothing you do with it outside the specified module should be able to break the consistency between its fields. I've confirmed, for example, that outside the specified module:

  • You can't construct a SpecifiedValue by manually initializing its fields.
  • The fields of a SpecifiedValue can't be assigned to or borrowed as mutable.
I've switched to Proposal 1c (84bfdef), which we adopted during today's meeting. The `SpecifiedValue` structure is read-only, courtesy of the [readonly](https://docs.rs/readonly/latest/readonly/) crate, so nothing you do with it outside the `specified` module should be able to break the consistency between its fields. I've confirmed, for example, that outside the `specified` module: - You can't construct a `SpecifiedValue` by manually initializing its fields. - The fields of a `SpecifiedValue` can't be assigned to or borrowed as mutable.
glen commented 2025-03-04 15:42:32 +00:00
Outdated
Review
Copy link

Looks good. Let me just see if I now understand: currently in the code the only way to produce a SpecifiedValue is via the try_from operation on a string; and if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place). Please let me know either way if I've got that all straight. Thanks.

Looks good. Let me just see if I now understand: currently in the code the _only_ way to produce a SpecifiedValue is via the try_from operation on a string; and if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place). Please let me know either way if I've got that all straight. Thanks.
Vectornaut commented 2025-03-10 19:50:35 +00:00
Outdated
Review
Copy link

currently in the code the only way to produce a SpecifiedValue is via the try_from operation on a string;

There's one more way: the from_empty_spec method is public too.

if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place).

Yup!

> currently in the code the only way to produce a SpecifiedValue is via the try_from operation on a string; There's one more way: the `from_empty_spec` method is public too. > if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place). Yup!
@ -151,9 +151,9 @@ type AssemblyMotion<'a> = Vec<ElementMotion<'a>>;
// a complete, view-independent description of an assembly
#[derive(Clone)]
pub struct Assembly {
// elements and observables
// elements and regulators
pub elements: Signal<Slab<Element>>,
pub observables: Signal<Slab<Observable>>,
pub regulators: Signal<Slab<Regulator>>,
glen marked this conversation as resolved Outdated
glen commented 2025-02-25 23:50:55 +00:00
Outdated
Review
Copy link

Maybe add "currently" -- "These are currently the only valid..." -- since we are definitely expecting to add other specifications (if we weren't, we would likely not go to this trouble).

Maybe add "currently" -- "These are currently the only valid..." -- since we are definitely expecting to add other specifications (if we weren't, we would likely not go to this trouble).
Vectornaut commented 2025-02-27 08:35:27 +00:00
Outdated
Review
Copy link

Done (7cbd926).

Done (7cbd926).
// solution variety tangent space. the basis vectors are stored in
// configuration matrix format, ordered according to the elements' column
@ -175,13 +175,13 @@ impl Assembly {
pub fn new() -> Assembly {
Assembly {
elements: create_signal(Slab::new()),
observables: create_signal(Slab::new()),
regulators: create_signal(Slab::new()),
tangent: create_signal(ConfigSubspace::zero(0)),
elements_by_id: create_signal(FxHashMap::default())
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:13:27 +00:00
Outdated
Review
Copy link

Why the difference in the interface between SpecifiedValue::try_from and Regulator::try_set? I.e., it seems to me that they could both return a Result, or could both return a boolean success flag...

Why the difference in the interface between SpecifiedValue::try_from and Regulator::try_set? I.e., it seems to me that they could both return a Result, or could both return a boolean success flag...
Vectornaut commented 2025-02-26 20:28:57 +00:00
Outdated
Review
Copy link

We could indeed modify try_set so that it returns a clone of the Result<SpecifiedValue> computed from the given specification. This might be useful if the caller wants a detailed error report when the try_set fails. Right now, though, the code that calls try_set only wants to know whether the attempt succeeded.

The associated function try_from is a constructor, so it can't just return a success flag. Would revising the comment on the TryFrom implementation help communicate this? Readers already familiar with the TryFrom trait would know this already, but making documentation broadly accessible is often worthwhile.

We could indeed modify `try_set` so that it returns a clone of the `Result<SpecifiedValue>` computed from the given specification. This might be useful if the caller wants a detailed error report when the `try_set` fails. Right now, though, the code that calls `try_set` only wants to know whether the attempt succeeded. The associated function `try_from` is a constructor, so it can't just return a success flag. Would revising the comment on the `TryFrom` implementation help communicate this? Readers already familiar with the [`TryFrom`](https://doc.rust-lang.org/std/convert/trait.TryFrom.html) trait would know this already, but making documentation broadly accessible is often worthwhile.
glen commented 2025-02-27 03:58:03 +00:00
Outdated
Review
Copy link

OK, you're saying try_from generates a SpecifiedValue, so if it succeeds, we definitely need to provide that entity. That makes sense.

What seems a bit confusing is a different interface on what's almost the identical operation. Why would try_set have to return a clone? Doesn't Rust allow a read-only alias to a struct? Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue? Couldn't a Result of that ReadSignal be provided in case of success or something like that? Presumably it's not hard to just check if what comes back is a Result rather than an Error, if that's all you care about?

OK, you're saying try_from generates a SpecifiedValue, so if it succeeds, we definitely need to provide that entity. That makes sense. What seems a bit confusing is a different interface on what's almost the identical operation. Why would try_set have to return a clone? Doesn't Rust allow a read-only alias to a struct? Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue? Couldn't a Result of that ReadSignal be provided in case of success or something like that? Presumably it's not hard to just check if what comes back is a Result rather than an Error, if that's all you care about?
Vectornaut commented 2025-02-27 07:31:29 +00:00
Outdated
Review
Copy link

Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue?

Yes: the set_point field is public. Thus, there's no need for try_set to return something that gives you access to the set point. If you have access to a regulator's try_set method, you also have access to its set_point field.

Why would try_set have to return a clone?

Here's the story as I vaguely understand it, based on my shaky knowledge of ownership in Rust. When the specification is valid, try_from returns a result of the form Ok(set_pt). We then call self.set_point.set(set_pt), which moves set_pt into the signal self.set_point. That means we can't return set_pt. I don't see a straightforward way to extract a reference to the value stored in a signal. I can use Signal::with to apply a closure to a reference to the stored value, but if I try to return that reference, the compiler can't infer an appropriate lifetime for it. Someone who understands lifetimes better might be able to make this work, but nothing I've tried has worked.

If your main goal is to return a detailed error report when try_set fails, we could have try_set return a Result<(), ParseFloatError>.

> Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue? Yes: the `set_point` field is public. Thus, there's no need for `try_set` to return something that gives you access to the set point. If you have access to a regulator's `try_set` method, you also have access to its `set_point` field. > Why would try_set have to return a clone? Here's the story as I vaguely understand it, based on my shaky knowledge of ownership in Rust. When the specification is valid, `try_from` returns a result of the form `Ok(set_pt)`. We then call `self.set_point.set(set_pt)`, which *moves* `set_pt` into the signal `self.set_point`. That means we can't return `set_pt`. I don't see a straightforward way to extract a reference to the value stored in a signal. I can use `Signal::with` to apply a closure to a reference to the stored value, but if I try to return that reference, the compiler can't infer an appropriate lifetime for it. Someone who understands lifetimes better might be able to make this work, but nothing I've tried has worked. If your main goal is to return a detailed error report when `try_set` fails, we could have `try_set` return a `Result<(), ParseFloatError>`.
glen commented 2025-03-01 02:02:45 +00:00
Outdated
Review
Copy link

No, my goal is not to have two non-parallel interfaces for essentially the same operation. But you are right, currently the error info is simply lost when you use try_set rather than try_from. Which begs the question: if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be; it seems as though it can't ever be wrong to make the set_point be any actual SpecifiedValue. And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface. To go from a possible string that might be a spec to changing the set_point of a Regulator, you first try_from it into a SpecifiedValue and then upon success set the set_point to that. Then you will at least receive the error info, which you can ignore if you like. Would such a refactoring actually be the cleanest interface? Looking forward to your thoughts.

No, my goal is not to have two non-parallel interfaces for essentially the same operation. But you are right, currently the error info is simply lost when you use try_set rather than try_from. Which begs the question: if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be; it seems as though it can't ever be wrong to make the set_point be any actual SpecifiedValue. And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface. To go from a possible string that might be a spec to changing the set_point of a Regulator, you first try_from it into a SpecifiedValue and then upon success set the set_point to that. Then you will at least receive the error info, which you can ignore if you like. Would such a refactoring actually be the cleanest interface? Looking forward to your thoughts.
Vectornaut commented 2025-03-01 02:42:36 +00:00
Outdated
Review
Copy link

[…] if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be […]

Yes, I think we should add a set method for this as soon as we have a reason to use it.

And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface.

Even if there were a set method, though, I would still want to keep try_set as a wrapper until we get to a point where we never have a reason to use it. I think that try_set is a concise way to express an operation that feels natural and self-contained.

> […] if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be […] Yes, I think we should add a `set` method for this as soon as we have a reason to use it. > And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface. Even if there were a `set` method, though, I would still want to keep `try_set` as a wrapper until we get to a point where we never have a reason to use it. I think that `try_set` is a concise way to express an operation that feels natural and self-contained.
glen commented 2025-03-01 02:59:59 +00:00
Outdated
Review
Copy link

I am not yet fluent in Rust syntax, so I am sure the below are not correct, but why does something like

myReg.set(try_from('foobar'))

need an abbreviation

myReg.try_set('foobar')

Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a Result<SpecifiedValue, SomeKindaError>. But then wouldn't it be most natural just to overload set to also take such an entity, rather than create a new method? And then if you do want the error payload, you could first capture the result of try_from and match on it, and do whatever with the error if that's what matches? This factoring seems simpler and less redundant to me. I guess my point is, we are already setting Regulators, so we already have a reason for such a method; and we can already try to convert a string to a SpecifiedValue; and so we don't need a redundant combo of the two with a different interface, it seems to me...

I am not yet fluent in Rust syntax, so I am sure the below are not correct, but why does something like ``` myReg.set(try_from('foobar')) ``` need an abbreviation ``` myReg.try_set('foobar') ``` Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a `Result<SpecifiedValue, SomeKindaError>`. But then wouldn't it be most natural just to overload `set` to also take such an entity, rather than create a new method? And then if you do want the error payload, you could first capture the result of try_from and match on it, and do whatever with the error if that's what matches? This factoring seems simpler and less redundant to me. I guess my point is, we are _already_ `set`ting Regulators, so we already have a reason for such a method; and we can already try to convert a string to a SpecifiedValue; and so we don't need a redundant combo of the two with a different interface, it seems to me...
Vectornaut commented 2025-03-01 03:26:39 +00:00
Outdated
Review
Copy link

Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a Result<SpecifiedValue, SomeKindaError>.

Exactly.

But then wouldn't it be most natural just to overload set to also take such an entity, rather than create a new method?

As far as I know, Rust doesn't have function overloading, so the versions of set that take a SpecifiedValue and a Result<SpecifiedValue, ParseFloatError> would have to have different names. We could choose set and set_if_ok, for example. I could look through the standard library to find out what kind of naming might be idiomatic.

> Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a Result<SpecifiedValue, SomeKindaError>. Exactly. > But then wouldn't it be most natural just to overload set to also take such an entity, rather than create a new method? As far as I know, Rust doesn't have function overloading, so the versions of `set` that take a `SpecifiedValue` and a `Result<SpecifiedValue, ParseFloatError>` would have to have different names. We could choose `set` and `set_if_ok`, for example. I could look through the standard library to find out what kind of naming might be idiomatic.
glen commented 2025-03-01 06:18:28 +00:00
Outdated
Review
Copy link

Rust doesn't have function overloading

Wow that's kind of wild and seems to invite non-DRYness (repeating the type of the operand in the function name, in effect).

If this basic interface idea seems OK, then the names of the two flavors of set are up to you, and you can only implement the one you actually end up wanting to use if it's only one, but please do comment what you would plan to name the other one.

> Rust doesn't have function overloading Wow that's kind of wild and seems to invite non-DRYness (repeating the type of the operand in the function name, in effect). If this basic interface idea seems OK, then the names of the two flavors of set are up to you, and you can only implement the one you actually end up wanting to use if it's only one, but please do comment what you would plan to name the other one.
glen commented 2025-03-01 06:28:23 +00:00
Outdated
Review
Copy link

Rust doesn't have function overloading

Oh, this SO answer says the way to support myReg.set(aPlainSpecifiedValue) and myReg.set(aResultThatCouldBe) with the choice made at compile time based on the type of the argument is with a parametrized Trait, which is pretty much the same thing as supporting function overloading. So that would seem preferable to me to two different names; but if you strongly prefer to not use a trait and have two names in this case, and plan them out in this PR, I can live with that, too.

> Rust doesn't have function overloading Oh, [this SO answer](https://stackoverflow.com/a/56509699) says the way to support `myReg.set(aPlainSpecifiedValue)` and `myReg.set(aResultThatCouldBe)` with the choice made at compile time based on the type of the argument is with a parametrized Trait, which is pretty much the same thing as supporting function overloading. So that would seem preferable to me to two different names; but if you strongly prefer to not use a trait and have two names in this case, and plan them out in this PR, I can live with that, too.
Vectornaut commented 2025-03-01 09:26:33 +00:00
Outdated
Review
Copy link

The idea of using a parameterized trait is nice, but in this case I'd prefer to use methods named set and set_if_ok. The former always updates the set point, while the latter only updates the set point under a certain condition, and I like how the different names communicate that.

please do comment what you would plan to name the other one.

Just here, or in the code too?

The idea of using a parameterized trait is nice, but in this case I'd prefer to use methods named `set` and `set_if_ok`. The former always updates the set point, while the latter only updates the set point under a certain condition, and I like how the different names communicate that. > please do comment what you would plan to name the other one. Just here, or in the code too?
glen commented 2025-03-01 14:05:24 +00:00
Outdated
Review
Copy link

In the code, thanks.

In the code, thanks.
Vectornaut commented 2025-03-03 21:16:29 +00:00
Outdated
Review
Copy link

Done (894931a).

Done (894931a).
}
}
// --- inserting elements and observables ---
// --- inserting elements and regulators ---
// insert an element into the assembly without checking whether we already
// have an element with the same identifier. any element that does have the
@ -224,14 +224,14 @@ impl Assembly {
);
}
pub fn insert_observable(&self, observable: Observable) {
let subjects = observable.subjects;
let key = self.observables.update(|obsls| obsls.insert(observable));
let subject_observables = self.elements.with(
|elts| (elts[subjects.0].observables, elts[subjects.1].observables)
pub fn insert_regulator(&self, regulator: Regulator) {
let subjects = regulator.subjects;
let key = self.regulators.update(|regs| regs.insert(regulator));
let subject_regulators = self.elements.with(
|elts| (elts[subjects.0].regulators, elts[subjects.1].regulators)
);
subject_observables.0.update(|obsls| obsls.insert(key));
subject_observables.1.update(|obsls| obsls.insert(key));
subject_regulators.0.update(|regs| regs.insert(key));
subject_regulators.1.update(|regs| regs.insert(key));
}
// --- realization ---
@ -248,13 +248,13 @@ impl Assembly {
let (gram, guess) = self.elements.with_untracked(|elts| {
// set up the off-diagonal part of the Gram matrix
let mut gram_to_be = PartialMatrix::new();
self.observables.with_untracked(|obsls| {
for (_, obs) in obsls {
if obs.role_is_valid_constraint_untracked() {
let subjects = obs.subjects;
self.regulators.with_untracked(|regs| {
for (_, reg) in regs {
if reg.role_is_valid_constraint_untracked() {
let subjects = reg.subjects;
let row = elts[subjects.0].column_index.unwrap();
let col = elts[subjects.1].column_index.unwrap();
gram_to_be.push_sym(row, col, obs.desired.get_untracked());
gram_to_be.push_sym(row, col, reg.desired.get_untracked());
}
}
});

74
app-proto/src/outline.rs
View file

@ -12,33 +12,33 @@ use crate::{
AppState,
assembly,
glen marked this conversation as resolved Outdated
glen commented 2025-03-08 00:26:29 +00:00
Outdated
Review
Copy link

Another use that needs to be harmonized with all our uses

Another `use` that needs to be harmonized with all our `uses`
Vectornaut commented 2025-03-08 23:31:30 +00:00
Outdated
Review
Copy link

Corrected, as discussed above.

Corrected, as discussed [above](#issuecomment-2317).
assembly::{
Observable,
ObservableKey,
ObservableRole::*,
Regulator,
RegulatorKey,
RegulatorRole::*,
ElementKey
}
};
// an editable view of the Lorentz product representing an observable
// an editable view of a regulator
#[component(inline_props)]
fn ObservableInput(observable: Observable) -> View {
fn RegulatorInput(regulator: Regulator) -> View {
view! {
input(
r#type="text",
placeholder=observable.measured.with(|result| result.to_string()),
bind:value=observable.desired_text,
placeholder=regulator.measured.with(|result| result.to_string()),
bind:value=regulator.desired_text,
on:change=move |event: Event| {
let target: HtmlInputElement = event.target().unwrap().unchecked_into();
let value = target.value();
if value.is_empty() {
observable.role.set(Measurement);
regulator.role.set(Measurement);
} else {
match target.value().parse::<f64>() {
Ok(desired) => batch(|| {
observable.desired.set(desired);
observable.role.set(Constraint(true));
regulator.desired.set(desired);
regulator.role.set(Constraint(true));
}),
Err(_) => observable.role.set(Constraint(false))
Err(_) => regulator.role.set(Constraint(false))
glen marked this conversation as resolved
glen commented 2025-03-10 19:36:54 +00:00
Review
Copy link

Please could you explain to me the three moves in the three closures in this view! macro, the one that specifies the class of the input element, the one that specifies its change handler, and the one that specifies its keydown handler?

If I am understanding Rust correctly, the move annotation specifies that the closure should take ownership of any free variables that appear therein. So that would mean that the function that computes the class of the input takes ownership of the regulator. Why is that desired? Is it because the input element may not actually be realized until after the RegulatorInput function returns, and the ownership transfer extends the lifetime of the regulator until the closure has a chance to run and grab the info it needs to set the class (or perhaps actually, it has to persist that regulator indefinitely and then it can react to all future changes to the regulator)? The class-calculating closure of the input element seems like a slightly odd place for the long-term ownership of the regulator to reside, but maybe it doesn't really matter what owns it, as long as it persists indefinitely?

But it seems my suggestions in the previous paragraph can't possibly be correct, because regulator also occurs free in the closure passed as on:change, but the ownership of one entity can't exist in two places, if I am understanding correctly. So I guess please just explain to me what's going on here.

And finally, the third move seems the most confusing. For the life of me, I can't see any free variables in the closure passed as on:keydown, so there doesn't seem to be anything to move, and so the move shouldn't be there.

Thanks in advance for illuminating these things for me.

Please could you explain to me the three `move`s in the three closures in this `view!` macro, the one that specifies the class of the input element, the one that specifies its change handler, and the one that specifies its keydown handler? If I am understanding Rust correctly, the `move` annotation specifies that the closure should take ownership of any free variables that appear therein. So that would mean that the function that computes the class of the input takes ownership of the regulator. Why is that desired? Is it because the input element may not actually be realized until after the RegulatorInput function returns, and the ownership transfer extends the lifetime of the regulator until the closure has a chance to run and grab the info it needs to set the class (or perhaps actually, it has to persist that regulator indefinitely and then it can react to all future changes to the regulator)? The class-calculating closure of the input element seems like a slightly odd place for the long-term ownership of the regulator to reside, but maybe it doesn't really matter what owns it, as long as it persists indefinitely? But it seems my suggestions in the previous paragraph can't possibly be correct, because `regulator` also occurs free in the closure passed as `on:change`, but the ownership of one entity can't exist in two places, if I am understanding correctly. So I guess please just explain to me what's going on here. And finally, the third `move` seems the most confusing. For the life of me, I can't see _any_ free variables in the closure passed as `on:keydown`, so there doesn't seem to be anything _to_ move, and so the `move` shouldn't be there. Thanks in advance for illuminating these things for me.
glen commented 2025-03-10 23:26:48 +00:00
Review
Copy link

Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal.

Anyhow, I now get the first two moves. And now I see that reset_value is just a local closure, and that's what's being moved by the third move (for the keydown event). So that's good. Resolving.

Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal. Anyhow, I now get the first two `move`s. And now I see that `reset_value` is just a local closure, and that's what's being `move`d by the third `move` (for the keydown event). So that's good. Resolving.
};
}
}
@ -46,30 +46,30 @@ fn ObservableInput(observable: Observable) -> View {
}
}
// a list item that shows an observable in an outline view of an element
// a list item that shows a regulator in an outline view of an element
#[component(inline_props)]
fn ObservableOutlineItem(observable_key: ObservableKey, element_key: ElementKey) -> View {
fn RegulatorOutlineItem(regulator_key: RegulatorKey, element_key: ElementKey) -> View {
let state = use_context::<AppState>();
let assembly = &state.assembly;
let observable = assembly.observables.with(|obsls| obsls[observable_key].clone());
let other_subject = if observable.subjects.0 == element_key {
observable.subjects.1
let regulator = assembly.regulators.with(|regs| regs[regulator_key].clone());
let other_subject = if regulator.subjects.0 == element_key {
regulator.subjects.1
} else {
observable.subjects.0
regulator.subjects.0
};
let other_subject_label = assembly.elements.with(|elts| elts[other_subject].label.clone());
let class = observable.role.map(
let class = regulator.role.map(
glen marked this conversation as resolved Outdated
glen commented 2025-03-04 16:00:16 +00:00
Outdated
Review
Copy link

This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set valid to true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just set valid to false and do nothing to the regulator? That organization would seem to read more crisply, as long as it isn't too cumbersome to write... And in fact, if I recall, isn't that sort of thing what if let OK(blah) = try_from { ... } else {valid.set(false)} is for?

P.S. If the code here switched to something like this suggestion, then as this is the only instance of set_if_ok, you could remove that method in favor of a simpler set method, or in fact maybe just a direct call of regulator.set_point.set(...), eliminating the entire impl Regulator block at lines 127-135 of assembly.rs, which seems like a win.

So I guess I am specifically suggesting replacing this block with

if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
   valid.set(true)
   regulator.set_point.set(spec)
} else { valid.set(false) }

-- presuming the syntax is OK -- and just removing that impl Regulator block in assembly.rs altogether.

This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set `valid` to true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just set `valid` to false and do nothing to the regulator? That organization would seem to read more crisply, as long as it isn't too cumbersome to write... And in fact, if I recall, isn't that sort of thing what `if let OK(blah) = try_from { ... } else {valid.set(false)}` is for? P.S. If the code here switched to something like this suggestion, then as this is the only instance of `set_if_ok`, you could remove that method in favor of a simpler `set` method, or in fact maybe just a direct call of `regulator.set_point.set(...)`, eliminating the entire `impl Regulator` block at lines 127-135 of assembly.rs, which seems like a win. So I guess I am specifically suggesting replacing this block with ``` if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) { valid.set(true) regulator.set_point.set(spec) } else { valid.set(false) } ``` -- presuming the syntax is OK -- and just removing that `impl Regulator` block in assembly.rs altogether.
Vectornaut commented 2025-03-05 22:25:09 +00:00
Outdated
Review
Copy link

So I guess I am specifically suggesting replacing this block with

if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
    valid.set(true)
    regulator.set_point.set(spec)
} else { valid.set(false) }

That sounds great to me. I'd reorganize the block to avoid the repetition of valid.set. Depending on whether you prefer if let or match, we could do either of these:

valid.set(
    if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) {
        regulator.set_point.set(set_pt);
        true
    } else { false }
)

valid.set(
    match SpecifiedValue::try_from(value.get_clone_untracked()) {
        Ok(set_pt) => {
            regulator.set_point.set(set_pt);
            true
        }
        Err(_) => false
    }
)

In the match version, the argument of valid.set is just an inlined version of the old Regulator::try_set method. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit 894931a, which replaced Regulator::try_set with Regulator::set_if_ok. We can also rewrite try_set to use if let instead of match, of course.

> So I guess I am specifically suggesting replacing this block with > > ```rust > if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) { > valid.set(true) > regulator.set_point.set(spec) > } else { valid.set(false) } That sounds great to me. I'd reorganize the block to avoid the repetition of `valid.set`. Depending on whether you prefer `if let` or `match`, we could do either of these: ```rust valid.set( if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) { regulator.set_point.set(set_pt); true } else { false } ) ``` ```rust valid.set( match SpecifiedValue::try_from(value.get_clone_untracked()) { Ok(set_pt) => { regulator.set_point.set(set_pt); true } Err(_) => false } ) ``` In the `match` version, the argument of `valid.set` is just an inlined version of the old `Regulator::try_set` method. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit 894931a, which replaced `Regulator::try_set` with `Regulator::set_if_ok`. We can also rewrite `try_set` to use `if let` instead of `match`, of course.
|role| match role {
Measurement => "observable",
Constraint(true) => "observable valid-constraint",
Constraint(false) => "observable invalid-constraint"
Measurement => "regulator",
Constraint(true) => "regulator valid-constraint",
Constraint(false) => "regulator invalid-constraint"
}
);
view! {
li(class=class.get()) {
div(class="observable-label") { (other_subject_label) }
div(class="observable-type") { "Inversive distance" }
ObservableInput(observable=observable)
div(class="regulator-label") { (other_subject_label) }
div(class="regulator-type") { "Inversive distance" }
RegulatorInput(regulator=regulator)
div(class="status")
}
}
@ -93,9 +93,9 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
).collect::<Vec<_>>()
)
};
let observed = element.observables.map(|obsls| obsls.len() > 0);
let observable_list = element.observables.map(
|obsls| obsls.clone().into_iter().collect()
let regulated = element.regulators.map(|regs| regs.len() > 0);
let regulator_list = element.regulators.map(
|regs| regs.clone().into_iter().collect()
);
let details_node = create_node_ref();
view! {
@ -110,7 +110,7 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
state.select(key, event.shift_key());
event.prevent_default();
},
"ArrowRight" if observed.get() => {
"ArrowRight" if regulated.get() => {
let _ = details_node
.get()
.unchecked_into::<web_sys::Element>()
@ -157,16 +157,16 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
div(class="status")
}
}
ul(class="observables") {
ul(class="regulators") {
Keyed(
list=observable_list,
view=move |obs_key| view! {
ObservableOutlineItem(
observable_key=obs_key,
list=regulator_list,
view=move |reg_key| view! {
RegulatorOutlineItem(
regulator_key=reg_key,
element_key=key
)
},
key=|obs_key| obs_key.clone()
key=|reg_key| reg_key.clone()
)
}
}
@ -174,9 +174,9 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
}
}
// a component that lists the elements of the current assembly, showing the
// observables associated with each element as a collapsible sub-list. its
// implementation is based on Kate Morley's HTML + CSS tree views:
// a component that lists the elements of the current assembly, showing each
// element's regulators in a collapsible sub-list. its implementation is based
// on Kate Morley's HTML + CSS tree views:
//
// https://iamkate.com/code/tree-views/
//