Generalize constraints to observables #48
|
|
@ -77,12 +77,12 @@ summary.selected {
|
|||
background-color: var(--selection-highlight);
|
||||
}
|
||||
|
||||
summary > div, .constraint {
|
||||
summary > div, .observable {
|
||||
padding-top: 4px;
|
||||
padding-bottom: 4px;
|
||||
}
|
||||
|
||||
.element, .constraint {
|
||||
.element, .observable {
|
||||
display: flex;
|
||||
flex-grow: 1;
|
||||
padding-left: 8px;
|
||||
|
|
@ -107,7 +107,7 @@ details[open]:has(li) .element-switch::after {
|
|||
flex-grow: 1;
|
||||
}
|
||||
|
||||
.constraint-label {
|
||||
.observable-label {
|
||||
flex-grow: 1;
|
||||
}
|
||||
|
||||
|
|
@ -123,22 +123,22 @@ details[open]:has(li) .element-switch::after {
|
|||
width: 56px;
|
||||
}
|
||||
|
||||
.constraint {
|
||||
.observable {
|
||||
font-style: italic;
|
||||
}
|
||||
|
||||
.constraint.invalid {
|
||||
.observable.invalid {
|
||||
color: var(--text-invalid);
|
||||
}
|
||||
|
||||
.constraint > input[type=text] {
|
||||
.observable > input[type=text] {
|
||||
color: inherit;
|
||||
background-color: inherit;
|
||||
border: 1px solid var(--border);
|
||||
border-radius: 2px;
|
||||
}
|
||||
|
||||
.constraint.invalid > input[type=text] {
|
||||
.observable.invalid > input[type=text] {
|
||||
border-color: var(--border-invalid);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -6,8 +6,8 @@ use crate::{
|
|||
AppState,
|
||||
assembly::{
|
||||
Assembly,
|
||||
|
glen marked this conversation as resolved
|
||||
Constraint,
|
||||
ConstraintRole,
|
||||
Observable,
|
||||
ObservableRole,
|
||||
Element
|
||||
},
|
||||
engine::Q
|
||||
|
|
@ -210,8 +210,8 @@ pub fn AddRemove() -> View {
|
|||
}
|
||||
);
|
||||
let desired = create_signal(0.0);
|
||||
let role = create_signal(ConstraintRole::Measure);
|
||||
state.assembly.insert_constraint(Constraint {
|
||||
let role = create_signal(ObservableRole::Measure);
|
||||
state.assembly.insert_observable(Observable {
|
||||
subjects: subjects,
|
||||
measured: measured,
|
||||
desired: desired,
|
||||
|
|
@ -221,16 +221,16 @@ pub fn AddRemove() -> View {
|
|||
state.selection.update(|sel| sel.clear());
|
||||
|
||||
/* DEBUG */
|
||||
// print updated constraint list
|
||||
console::log_1(&JsValue::from("Constraints:"));
|
||||
state.assembly.constraints.with(|csts| {
|
||||
for (_, cst) in csts.into_iter() {
|
||||
// print updated observable list
|
||||
console::log_1(&JsValue::from("Observables:"));
|
||||
state.assembly.observables.with(|obsls| {
|
||||
for (_, obs) in obsls.into_iter() {
|
||||
console::log_5(
|
||||
&JsValue::from(" "),
|
||||
&JsValue::from(cst.subjects.0),
|
||||
&JsValue::from(cst.subjects.1),
|
||||
&JsValue::from(obs.subjects.0),
|
||||
&JsValue::from(obs.subjects.1),
|
||||
&JsValue::from(":"),
|
||||
&JsValue::from(cst.desired.get_untracked())
|
||||
&JsValue::from(obs.desired.get_untracked())
|
||||
);
|
||||
}
|
||||
});
|
||||
|
|
@ -239,10 +239,10 @@ pub fn AddRemove() -> View {
|
|||
// constrained, or is edited while constrained
|
||||
create_effect(move || {
|
||||
console::log_1(&JsValue::from(
|
||||
format!("Constraint ({}, {}) updated", subjects.0, subjects.1)
|
||||
format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)
|
||||
));
|
||||
desired.track();
|
||||
if role.with(|r| matches!(r, ConstraintRole::Constrain)) {
|
||||
if role.with(|r| matches!(r, ObservableRole::Constrain)) {
|
||||
state.assembly.realize();
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 constraints
|
||||
// the types of the keys we use to access an assembly's elements and observables
|
||||
pub type ElementKey = usize;
|
||||
pub type ConstraintKey = usize;
|
||||
pub type ObservableKey = 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 constraints: Signal<BTreeSet<ConstraintKey>>,
|
||||
pub observables: Signal<BTreeSet<ObservableKey>>,
|
||||
|
||||
// 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),
|
||||
constraints: create_signal(BTreeSet::default()),
|
||||
observables: create_signal(BTreeSet::default()),
|
||||
serial: serial,
|
||||
column_index: None
|
||||
}
|
||||
|
|
@ -111,19 +111,19 @@ impl Element {
|
|||
}
|
||||
}
|
||||
|
||||
pub enum ConstraintRole {
|
||||
pub enum ObservableRole {
|
||||
Measure,
|
||||
Constrain,
|
||||
Invalid
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
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
Nice catch! I've changed "above" to "at" to resolve the ambiguity ( Nice catch! I've changed "above" to "at" to resolve the ambiguity (7cbd926). Does that read all right to you?
glen
commented
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.
|
||||
pub struct Constraint {
|
||||
pub struct Observable {
|
||||
pub subjects: (ElementKey, ElementKey),
|
||||
pub measured: ReadSignal<f64>,
|
||||
pub desired: Signal<f64>,
|
||||
pub desired_text: Signal<String>,
|
||||
pub role: Signal<ConstraintRole>
|
||||
pub role: Signal<ObservableRole>
|
||||
}
|
||||
|
||||
// the velocity is expressed in uniform coordinates
|
||||
|
|
@ -137,9 +137,9 @@ type AssemblyMotion<'a> = Vec<ElementMotion<'a>>;
|
|||
// a complete, view-independent description of an assembly
|
||||
#[derive(Clone)]
|
||||
pub struct Assembly {
|
||||
// elements and constraints
|
||||
// elements and observables
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
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 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
I definitely think we should give the 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
I think it would be weirder to have -- 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
To me, it seems perfectly natural to have this: An absent the string I thought the main point of the 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
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:
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.
We agree on this, too. Of course a SpecifiedValue should carry its specification with it -- it should be its primary data.
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 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 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
If you find it less disorienting, we could do this: Now both variants have a field called To access the specification of a general The answers to this StackExchange question discuss various approaches to creating data types that have multiple variants with common fields.
> 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
Hmm; it would seem to me that something like 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
At this point, I can't predict whether that would make things cleaner or messier. I'd want
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:
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
I've switched to Proposal 1c (
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
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
There's one more way: the
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!
|
||||
pub elements: Signal<Slab<Element>>,
|
||||
pub constraints: Signal<Slab<Constraint>>,
|
||||
pub observables: Signal<Slab<Observable>>,
|
||||
|
||||
// solution variety tangent space. the basis vectors are stored in
|
||||
// configuration matrix format, ordered according to the elements' column
|
||||
|
|
@ -161,13 +161,13 @@ impl Assembly {
|
|||
pub fn new() -> Assembly {
|
||||
Assembly {
|
||||
elements: create_signal(Slab::new()),
|
||||
constraints: create_signal(Slab::new()),
|
||||
observables: create_signal(Slab::new()),
|
||||
tangent: create_signal(ConfigSubspace::zero(0)),
|
||||
elements_by_id: create_signal(FxHashMap::default())
|
||||
}
|
||||
}
|
||||
|
||||
// --- inserting elements and constraints ---
|
||||
// --- inserting elements and observables ---
|
||||
|
||||
// insert an element into the assembly without checking whether we already
|
||||
// have an element with the same identifier. any element that does have the
|
||||
|
|
@ -210,14 +210,14 @@ impl Assembly {
|
|||
);
|
||||
}
|
||||
|
||||
pub fn insert_constraint(&self, constraint: Constraint) {
|
||||
let subjects = constraint.subjects;
|
||||
let key = self.constraints.update(|csts| csts.insert(constraint));
|
||||
let subject_constraints = self.elements.with(
|
||||
|elts| (elts[subjects.0].constraints, elts[subjects.1].constraints)
|
||||
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)
|
||||
);
|
||||
subject_constraints.0.update(|csts| csts.insert(key));
|
||||
subject_constraints.1.update(|csts| csts.insert(key));
|
||||
subject_observables.0.update(|obsls| obsls.insert(key));
|
||||
subject_observables.1.update(|obsls| obsls.insert(key));
|
||||
}
|
||||
|
||||
// --- realization ---
|
||||
|
|
@ -234,13 +234,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.constraints.with_untracked(|csts| {
|
||||
for (_, cst) in csts {
|
||||
if cst.role.with_untracked(|role| matches!(role, ConstraintRole::Constrain)) {
|
||||
let subjects = cst.subjects;
|
||||
self.observables.with_untracked(|obsls| {
|
||||
for (_, obs) in obsls {
|
||||
if obs.role.with_untracked(|role| matches!(role, ObservableRole::Constrain)) {
|
||||
let subjects = obs.subjects;
|
||||
let row = elts[subjects.0].column_index.unwrap();
|
||||
let col = elts[subjects.1].column_index.unwrap();
|
||||
gram_to_be.push_sym(row, col, cst.desired.get_untracked());
|
||||
gram_to_be.push_sym(row, col, obs.desired.get_untracked());
|
||||
}
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -12,33 +12,33 @@ use crate::{
|
|||
AppState,
|
||||
assembly,
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
Another Another `use` that needs to be harmonized with all our `uses`
Vectornaut
commented
Corrected, as discussed above. Corrected, as discussed [above](#issuecomment-2317).
|
||||
assembly::{
|
||||
Constraint,
|
||||
ConstraintKey,
|
||||
ConstraintRole::*,
|
||||
Observable,
|
||||
ObservableKey,
|
||||
ObservableRole::*,
|
||||
ElementKey
|
||||
}
|
||||
};
|
||||
|
||||
// an editable view of the Lorentz product representing a constraint
|
||||
// an editable view of the Lorentz product representing an observable
|
||||
#[component(inline_props)]
|
||||
fn LorentzProductInput(constraint: Constraint) -> View {
|
||||
fn ObservableInput(observable: Observable) -> View {
|
||||
view! {
|
||||
input(
|
||||
r#type="text",
|
||||
placeholder=constraint.measured.with(|result| result.to_string()),
|
||||
bind:value=constraint.desired_text,
|
||||
placeholder=observable.measured.with(|result| result.to_string()),
|
||||
bind:value=observable.desired_text,
|
||||
on:change=move |event: Event| {
|
||||
let target: HtmlInputElement = event.target().unwrap().unchecked_into();
|
||||
let value = target.value();
|
||||
if value.is_empty() {
|
||||
constraint.role.set(Measure);
|
||||
observable.role.set(Measure);
|
||||
} else {
|
||||
match target.value().parse::<f64>() {
|
||||
Ok(desired) => batch(|| {
|
||||
constraint.desired.set(desired);
|
||||
constraint.role.set(Constrain);
|
||||
observable.desired.set(desired);
|
||||
observable.role.set(Constrain);
|
||||
}),
|
||||
Err(_) => constraint.role.set(Invalid)
|
||||
Err(_) => observable.role.set(Invalid)
|
||||
|
glen marked this conversation as resolved
glen
commented
Please could you explain to me the three If I am understanding Rust correctly, the But it seems my suggestions in the previous paragraph can't possibly be correct, because And finally, the third 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
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 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,29 +46,29 @@ fn LorentzProductInput(constraint: Constraint) -> View {
|
|||
}
|
||||
}
|
||||
|
||||
// a list item that shows a constraint in an outline view of an element
|
||||
// a list item that shows an observable in an outline view of an element
|
||||
#[component(inline_props)]
|
||||
fn ConstraintOutlineItem(constraint_key: ConstraintKey, element_key: ElementKey) -> View {
|
||||
fn ObservableOutlineItem(observable_key: ObservableKey, element_key: ElementKey) -> View {
|
||||
let state = use_context::<AppState>();
|
||||
let assembly = &state.assembly;
|
||||
let constraint = assembly.constraints.with(|csts| csts[constraint_key].clone());
|
||||
let other_subject = if constraint.subjects.0 == element_key {
|
||||
constraint.subjects.1
|
||||
let observable = assembly.observables.with(|obsls| obsls[observable_key].clone());
|
||||
let other_subject = if observable.subjects.0 == element_key {
|
||||
observable.subjects.1
|
||||
} else {
|
||||
constraint.subjects.0
|
||||
observable.subjects.0
|
||||
};
|
||||
let other_subject_label = assembly.elements.with(|elts| elts[other_subject].label.clone());
|
||||
let class = constraint.role.map(
|
||||
let class = observable.role.map(
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
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 P.S. If the code here switched to something like this suggestion, then as this is the only instance of So I guess I am specifically suggesting replacing this block with -- presuming the syntax is OK -- and just removing that 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
That sounds great to me. I'd reorganize the block to avoid the repetition of In the > 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 {
|
||||
Measure => "constraint",
|
||||
Constrain => "constraint constrained",
|
||||
Invalid => "constraint invalid"
|
||||
Measure => "observable",
|
||||
Constrain => "observable constrained",
|
||||
Invalid => "observable invalid"
|
||||
}
|
||||
);
|
||||
view! {
|
||||
li(class=class.get()) {
|
||||
div(class="constraint-label") { (other_subject_label) }
|
||||
LorentzProductInput(constraint=constraint)
|
||||
div(class="observable-label") { (other_subject_label) }
|
||||
ObservableInput(observable=observable)
|
||||
div(class="status")
|
||||
}
|
||||
}
|
||||
|
|
@ -92,9 +92,9 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
|
|||
).collect::<Vec<_>>()
|
||||
)
|
||||
};
|
||||
let constrained = element.constraints.map(|csts| csts.len() > 0);
|
||||
let constraint_list = element.constraints.map(
|
||||
|csts| csts.clone().into_iter().collect()
|
||||
let observed = element.observables.map(|obsls| obsls.len() > 0);
|
||||
let observable_list = element.observables.map(
|
||||
|obsls| obsls.clone().into_iter().collect()
|
||||
);
|
||||
let details_node = create_node_ref();
|
||||
view! {
|
||||
|
|
@ -109,7 +109,7 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
|
|||
state.select(key, event.shift_key());
|
||||
event.prevent_default();
|
||||
},
|
||||
"ArrowRight" if constrained.get() => {
|
||||
"ArrowRight" if observed.get() => {
|
||||
let _ = details_node
|
||||
.get()
|
||||
.unchecked_into::<web_sys::Element>()
|
||||
|
|
@ -156,16 +156,16 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
|
|||
div(class="status")
|
||||
}
|
||||
}
|
||||
ul(class="constraints") {
|
||||
ul(class="observables") {
|
||||
Keyed(
|
||||
list=constraint_list,
|
||||
view=move |cst_key| view! {
|
||||
ConstraintOutlineItem(
|
||||
constraint_key=cst_key,
|
||||
list=observable_list,
|
||||
view=move |obs_key| view! {
|
||||
ObservableOutlineItem(
|
||||
observable_key=obs_key,
|
||||
element_key=key
|
||||
)
|
||||
},
|
||||
key=|cst_key| cst_key.clone()
|
||||
key=|obs_key| obs_key.clone()
|
||||
)
|
||||
}
|
||||
}
|
||||
|
|
@ -174,8 +174,8 @@ fn ElementOutlineItem(key: ElementKey, element: assembly::Element) -> View {
|
|||
}
|
||||
|
||||
// a component that lists the elements of the current assembly, showing the
|
||||
// constraints on each element as a collapsible sub-list. its implementation
|
||||
// is based on Kate Morley's HTML + CSS tree views:
|
||||
// observables associated with each element as a collapsible sub-list. its
|
||||
// implementation is based on Kate Morley's HTML + CSS tree views:
|
||||
//
|
||||
// https://iamkate.com/code/tree-views/
|
||||
//
|
||||
|
|
|
|||
In assembly.rs, you put all of the sub-namespaces under a top-level item on a single line, like
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.
Good catch. My convention has been to start with each
usedeclaration 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. Theusedeclarations you noticed break this convention; I've corrected them In commitb9db7a5. I think the convention is followed everywhere else, but I'll keep an eye on it whenever I reviseusedeclarations in future commits.