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 +8 -4
1 changed files with 8 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 7cbd92618b - Show all commits

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

@ -30,8 +30,11 @@ pub struct Element {
pub label: String,
pub color: ElementColor,
pub representation: Signal<DVector<f64>>,
glen marked this conversation as resolved Outdated
glen commented 2025-02-25 23:45:02 +00:00
Outdated
Review
Copy link

Is it worth commenting that this is intended to be the set of all regulators that have this Element as a subject? Or is that entirely clear? Similarly, I think it should be noted somewhere, perhaps as a comment here, the mechanism/responsibility for maintaining that information. When a regulator is inserted into an Assembly, is it the assembly's responsibility to tell each of the subjects? I imagine that's the design, but it would be good to have it written down somewhere.

Is it worth commenting that this is intended to be the set of all regulators that have this Element as a subject? Or is that entirely clear? Similarly, I think it should be noted somewhere, perhaps as a comment here, the mechanism/responsibility for maintaining that information. When a regulator is inserted into an Assembly, is it the assembly's responsibility to tell each of the subjects? I imagine that's the design, but it would be good to have it written down somewhere.
glen commented 2025-02-26 01:31:52 +00:00
Outdated
Review
Copy link

I mean, of course by now I have gotten to that section of the code and seen that what I surmised is exactly what's happening, but I think the obligations of the various structures should be made clear not just in the implementing code in case some later coder comes along and doesn't realize that if the Assembly modifies regulators in some way, it's the Assembly's responsibility to update this information on the elements...

I mean, of course by now I have gotten to that section of the code and seen that what I surmised is exactly what's happening, but I think the obligations of the various structures should be made clear not just in the implementing code in case some later coder comes along and doesn't realize that if the Assembly modifies regulators in some way, it's the Assembly's responsibility to update this information on the elements...
Vectornaut commented 2025-02-27 08:32:14 +00:00
Outdated
Review
Copy link

I've added a comment to this effect (7cbd926). Let me know if I should make it more detailed.

I've added a comment to this effect (7cbd926). Let me know if I should make it more detailed.
glen commented 2025-03-01 01:28:49 +00:00
Outdated
Review
Copy link

Detail level was fine. I just tweaked grammar/explicitness level. Resolving.

Detail level was fine. I just tweaked grammar/explicitness level. Resolving.
// the regulators that affect this element. the ambient assembly is
// responsible for keeping this set up to date
pub regulators: Signal<BTreeSet<RegulatorKey>>,
// a serial number, assigned by `Element::new`, that uniquely identifies
// each element
pub serial: u64,
@ -117,7 +120,7 @@ impl Element {
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.
// to construct a `SpecifiedValue` that might be `Present`, use the associated
// function `try_from`. this ensures that `spec` is always a valid specification
// of `value` according to the format discussed above the implementation of
// of `value` according to the format discussed at the implementation of
// `TryFrom<String>`
pub enum SpecifiedValue {
Absent,
@ -154,7 +157,8 @@ impl SpecifiedValue {
// specification is empty, the `SpecifiedValue` is `Absent`. if the
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).
// specification parses to a floating-point value `x`, the `SpecifiedValue` is
// `Present`, with a `value` of `x`, and the specification is stored in `spec`.
// these are the only valid specifications; any other produces an error
// these are currently the only valid specifications; any other produces an
// error
impl TryFrom<String> for SpecifiedValue {
type Error = ParseFloatError;
@ -183,7 +187,7 @@ impl Regulator {
self.set_point.set(set_pt);
true
}
Err(_) => false,
Err(_) => false
}
}
}