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

feat: Point coordinate regulators #118

Merged
Vectornaut merged 9 commits from glen/dyna3:pointCoordRegulators into main 2025-10-13 22:52:03 +00:00
Conversation 27 Commits 9 Files changed 6 +23 -40
1 changed files with 23 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit c081f1a809 - Show all commits

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

@ -308,11 +308,10 @@ impl Element for Point {
Vectornaut commented 2025-10-10 05:53:03 +00:00
Outdated
Copy link

Our current convention is that all the lines in a block, including blank lines, should have the block indentation. Under that convention, this blank line would need to be indented by four spaces, just like the lines above it, below it, and elsewhere in the impl block.

Our current convention is that all the lines in a block, including blank lines, should have the block indentation. Under that convention, this blank line would need to be indented by four spaces, just like the lines above it, below it, and elsewhere in the `impl` block.
glen commented 2025-10-10 15:29:34 +00:00
Outdated
Copy link

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). I will go ahead and remove all trailing whitespace for consistency in an immediately following PR.

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). I will go ahead and remove all trailing whitespace for consistency in an immediately following PR.
Vectornaut commented 2025-10-10 22:49:25 +00:00
Outdated
Copy link

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line).

This is the convention the Rust style guide recommends, so I'm fine with it, even though it's not my favorite.

I have a slight preference for keeping this pull request consistent with the existing style, and removing all the trailing whitespace at once in the upcoming pull request.

> A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). This is the convention the [Rust style guide](https://doc.rust-lang.org/beta/style-guide/index.html#trailing-whitespace) recommends, so I'm fine with it, even though it's not my favorite. I have a slight preference for keeping this pull request consistent with the existing style, and removing all the trailing whitespace at once in the upcoming pull request.
fn default_regulators(self: Rc<Self>) -> Vec<Rc<dyn Regulator>> {
all::<Axis>()
.map(
|axis| Rc::new(
PointCoordinateRegulator::new(self.clone(), axis)
) as Rc::<dyn Regulator>
)
.map(|axis| {
Rc::new(PointCoordinateRegulator::new(self.clone(), axis))
as Rc::<dyn Regulator>
})
.collect()
}
@ -539,32 +538,19 @@ impl PointCoordinateRegulator {
let measurement = subject.representation().map(
move |rep| rep[axis as usize]
);
let set_point = create_signal(SpecifiedValue::from_empty_spec());
let serial = Self::next_serial();
Self { subject, axis, measurement, set_point, serial }
Self { subject, axis, measurement, set_point, serial: Self::next_serial() }
}
}
impl Serial for PointCoordinateRegulator {
fn serial(&self) -> u64 {
self.serial
}
fn serial(&self) -> u64 { self.serial }
}
impl Regulator for PointCoordinateRegulator {
fn subjects(&self) -> Vec<Rc<dyn Element>> {
vec![self.subject.clone()]
}
fn measurement(&self) -> ReadSignal<f64> {
self.measurement
}
fn set_point(&self) -> Signal<SpecifiedValue> {
self.set_point
}
fn subjects(&self) -> Vec<Rc<dyn Element>> { vec![self.subject.clone()] }
fn measurement(&self) -> ReadSignal<f64> { self.measurement }
fn set_point(&self) -> Signal<SpecifiedValue> { self.set_point }
}
impl ProblemPoser for PointCoordinateRegulator {
@ -572,25 +558,22 @@ impl ProblemPoser for PointCoordinateRegulator {
self.set_point.with_untracked(|set_pt| {
if let Some(val) = set_pt.value {
let col = self.subject.column_index().expect(
"Subject should be indexed before point coordinate regulator writes problem data"
);
"Subject must be indexed before point-coordinate regulator poses.");
Vectornaut commented 2025-10-10 06:20:18 +00:00
Outdated
Copy link

For consistency with InversiveDistanceRegulator, I'd change the panic message to:

Subject should be indexed before point coordinate regulator writes problem data

For consistency with `InversiveDistanceRegulator`, I'd change the panic message to: > Subject should be indexed before point coordinate regulator writes problem data
glen commented 2025-10-10 15:30:51 +00:00
Outdated
Copy link

As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".

As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".
👍 1
glen commented 2025-10-10 17:35:03 +00:00
Outdated
Copy link

Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).

Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).
Vectornaut commented 2025-10-10 22:40:11 +00:00
Outdated
Copy link

The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes.

  • Add more detail to the helper name: maybe something like index_required_message, missing_index_panic_message, or index_before_posing_message?
  • Rename the thing argument to something like role or kind_of_thing. Or, more broadly, name the arguments something like object_kind, object_name, actor?

As far as I can see from the code, "must" is correct.

Agreed.

The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes. - Add more detail to the helper name: maybe something like `index_required_message`, `missing_index_panic_message`, or `index_before_posing_message`? - Rename the `thing` argument to something like `role` or `kind_of_thing`. Or, more broadly, name the arguments something like `object_kind`, `object_name`, `actor`? > As far as I can see from the code, "must" is correct. Agreed.
Vectornaut commented 2025-10-10 22:43:51 +00:00
Outdated
Copy link

To me, the placement of the new helper is kind of weird to me. It's right in the middle of the Sphere definition and implementation section, which makes it seem associated with Sphere, when in fact it's relevant to many problem-posers. It might fit better above the definition of the ProblemPoser trait, although I'd be open to other ideas about where to put it.

To me, the placement of the new helper is kind of weird to me. It's right in the middle of the `Sphere` definition and implementation section, which makes it seem associated with `Sphere`, when in fact it's relevant to many problem-posers. It might fit better above the definition of the `ProblemPoser` trait, although I'd be open to other ideas about where to put it.
glen commented 2025-10-10 23:15:46 +00:00
Outdated
Copy link

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.
Vectornaut commented 2025-10-10 23:35:30 +00:00
Outdated
Copy link

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it.

Great!

Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject.

The main thing I'd like to clarify, if possible, is that thing and name go together, and that thing is a type or role, while name is an identifier.

> Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Great! > Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. The main thing I'd like to clarify, if possible, is that `thing` and `name` go together, and that `thing` is a type or role, while `name` is an identifier.
problem.frozen.push(self.axis as usize, col, val);
// if all three of the subject's spatial coordinates have been
// frozen, then freeze its norm component too
let mut coords_frozen = [0.0; Axis::CARDINALITY];
let mut n_set: usize = 0;
for &MatrixEntry { index, value } in &(problem.frozen) {
let (row_frozen, col_frozen) = index;
if col_frozen == col && row_frozen < Axis::CARDINALITY {
n_set += 1;
coords_frozen[row_frozen] = value
// Check if all three spatial coordinates have been frozen, and if so,
// freeze the norm component as well
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-10-10 06:16:29 +00:00
Outdated
Copy link

I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed.

I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment.

I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like.

// if all three of the subject's spatial coordinates have been
// frozen, then freeze its norm component too

(I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)

I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed. I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment. I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like. ``` // if all three of the subject's spatial coordinates have been // frozen, then freeze its norm component too ``` (I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)
glen commented 2025-10-10 15:32:30 +00:00
Outdated
Copy link

I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.

I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.
glen commented 2025-10-10 16:02:49 +00:00
Outdated
Copy link

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.
Vectornaut commented 2025-10-10 22:07:16 +00:00
Outdated
Copy link

Thanks for reflowing and rewording the comment!

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style.

The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the line width and comment width rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.

Thanks for reflowing and rewording the comment! > Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them. I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style. The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the [line width](https://doc.rust-lang.org/beta/style-guide/index.html#indentation-and-line-width) and [comment width](https://doc.rust-lang.org/beta/style-guide/index.html#comments) rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.
glen commented 2025-10-10 22:57:11 +00:00
Outdated
Copy link

I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?
Vectornaut commented 2025-10-10 23:05:52 +00:00
Outdated
Copy link

I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.

> I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok? Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.
glen commented 2025-10-11 01:22:49 +00:00
Outdated
Copy link

The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks.

As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment:

  • switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot.
  • switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g.
fn myfn(arg: Type) {
   if condition {
      take_action(arg, other, long,
         arguments, in, list)}}

fn my otherfn() -> ReturnType {
   ...
The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks. As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment: * switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot. * switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g. ``` fn myfn(arg: Type) { if condition { take_action(arg, other, long, arguments, in, list)}} fn my otherfn() -> ReturnType { ... ```
Vectornaut commented 2025-10-11 02:12:05 +00:00
Outdated
Copy link

So I'd like to just stick with 80, thanks.

Sounds good!

In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment

Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.

> So I'd like to just stick with 80, thanks. Sounds good! > In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.
let mut coords = [0.0; Axis::CARDINALITY];
let mut nset: usize = 0;
for &MatrixEntry {index, value} in &(problem.frozen) {
if index.1 == col && index.0 < Axis::CARDINALITY {
nset += 1;
coords[index.0] = value
}
}
if n_set == Axis::CARDINALITY {
let [x, y, z] = coords_frozen;
let norm = point(x, y, z)[Point::NORM_COMPONENT];
problem.frozen.push(Point::NORM_COMPONENT, col, norm);
if nset == Axis::CARDINALITY {
let [x, y, z] = coords;
Vectornaut marked this conversation as resolved
Vectornaut commented 2025-10-13 22:35:51 +00:00
Copy link

Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as { index, value } elsewhere, but during review we discussed potentially changing this convention in the future.

Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as `{ index, value }` elsewhere, but during review we discussed potentially changing this convention in the future.
glen commented 2025-10-13 23:08:12 +00:00
Copy link

It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.

It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.
👍 1
problem.frozen.push(
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
}
}
});