feat: Point coordinate regulators #118

Merged
Vectornaut merged 9 commits from glen/dyna3:pointCoordRegulators into main 2025-10-13 22:52:03 +00:00
5 changed files with 117 additions and 5 deletions
Showing only changes of commit ecbbe2068c - Show all commits

21
app-proto/Cargo.lock generated
View file

@ -255,6 +255,7 @@ dependencies = [
"charming",
"console_error_panic_hook",
"dyna3",
"enum-iterator",
"itertools",
"js-sys",
"lazy_static",
@ -271,6 +272,26 @@ version = "1.13.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "60b1af1c220855b6ceac025d3f6ecdd2b7c4894bfe9cd9bda4fbb4bc7c0d4cf0"
[[package]]
name = "enum-iterator"
version = "2.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a4549325971814bda7a44061bf3fe7e487d447cba01e4220a4b454d630d7a016"
dependencies = [
"enum-iterator-derive",
]
[[package]]
name = "enum-iterator-derive"
version = "1.5.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "685adfa4d6f3d765a26bc5dbc936577de9abf756c1feeb3089b01dd395034842"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "equivalent"
version = "1.0.1"

View file

@ -10,6 +10,7 @@ default = ["console_error_panic_hook"]
dev = []
[dependencies]
enum-iterator = "2.3.0"
itertools = "0.13.0"
js-sys = "0.3.70"
lazy_static = "1.5.0"

View file

@ -1,3 +1,4 @@
use enum_iterator::{all, Sequence};
use nalgebra::{DMatrix, DVector, DVectorView};
use std::{
cell::Cell,
@ -26,6 +27,7 @@ use crate::{
ConfigSubspace,
ConstraintProblem,
DescentHistory,
MatrixEntry,
Realization,
},
specified::SpecifiedValue,
@ -269,6 +271,7 @@ pub struct Point {
impl Point {
const WEIGHT_COMPONENT: usize = 3;
const NORM_COMPONENT: usize = 4;
pub fn new(
id: String,
@ -303,6 +306,15 @@ impl Element for Point {
)
}

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.

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.

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>
})
.collect()
}
fn id(&self) -> &String {
&self.id
}
@ -446,14 +458,14 @@ impl ProblemPoser for InversiveDistanceRegulator {
}
pub struct HalfCurvatureRegulator {
pub subject: Rc<dyn Element>,
pub subject: Rc<Sphere>,
pub measurement: ReadSignal<f64>,
pub set_point: Signal<SpecifiedValue>,
serial: u64,
}
impl HalfCurvatureRegulator {
pub fn new(subject: Rc<dyn Element>) -> Self {
pub fn new(subject: Rc<Sphere>) -> Self {
let measurement = subject.representation().map(
|rep| rep[Sphere::CURVATURE_COMPONENT]
);
@ -498,6 +510,69 @@ impl ProblemPoser for HalfCurvatureRegulator {
}
}
#[derive(Clone, Copy, Sequence)]
pub enum Axis {X = 0, Y = 1, Z = 2}
impl Axis {
glen marked this conversation as resolved Outdated

I generally prefer to punctuate code as one would text, when possible. Can we remove the two extraneous spaces you just added here? They don't seem to improve readability (as we are all used to no space between brackets and content).

I generally prefer to punctuate code as one would text, when possible. Can we remove the two extraneous spaces you just added here? They don't seem to improve readability (as we are all used to no space between brackets and content).

This spacing convention is widely used in Rust code, and it's recommended in the Rust style guide:

For a single-line block, put spaces after the opening brace and before the closing brace.

I like it a lot, and I've used it in hundreds of places in the dyna3 source code. If you're adamant about changing it, I'd prefer to do it throughout the source code in a formatting-only pull request.

This spacing convention is widely used in Rust code, and it's [recommended](https://doc.rust-lang.org/beta/style-guide/expressions.html#blocks) in the Rust style guide: > For a single-line block, put spaces after the opening brace and before the closing brace. I like it a lot, and I've used it in hundreds of places in the dyna3 source code. If you're adamant about changing it, I'd prefer to do it throughout the source code in a formatting-only pull request.

We don't have to fix it now. I don't care at all about the Rust style guide, it's an assembly of weird rigidities from my point of view (what business does a language have semi-enforcing snake_case identifiers? I thought typography reiterating semantic category went into the trashbin with Perl a decade or more ago...). I very much like the convention of punctuating code as close as possible to regular text. I get it that you are used to these spaces that look very extra to me. We'll figure out how to resolve this at some point. No need to spin our wheels on it at the moment.

We don't have to fix it now. I don't care at all about the Rust style guide, it's an assembly of weird rigidities from my point of view (what business does a _language_ have semi-enforcing snake_case identifiers? I thought typography reiterating semantic category went into the trashbin with Perl a decade or more ago...). I very much like the convention of punctuating code as close as possible to regular text. I get it that you are used to these spaces that look very extra to me. We'll figure out how to resolve this at some point. No need to spin our wheels on it at the moment.
pub const N_AXIS: usize = (Axis::Z as usize) + 1;
Vectornaut marked this conversation as resolved Outdated

The associated constant Axis::N_AXIS, which is defined by hand here, has the same value as the associated constant Axis::CARDINALITY, which is defined automatically when we derive the Sequence trait. I'd recommend removing N_AXIS and using CARDINALITY in its place. I'm confident that CARDINALITY is currently 3, because I get a compilation error if I replace it with a value different from 3 in the definition of Axis::NAME.

The associated constant `Axis::N_AXIS`, which is defined by hand here, has the same value as the associated constant [`Axis::CARDINALITY`](https://docs.rs/enum-iterator/latest/enum_iterator/trait.Sequence.html#associatedconstant.CARDINALITY), which is defined automatically when we derive the `Sequence` trait. I'd recommend removing `N_AXIS` and using `CARDINALITY` in its place. I'm confident that `CARDINALITY` is currently 3, because I get a compilation error if I replace it with a value different from 3 in the definition of `Axis::NAME`.

Done in latest commit

Done in latest commit
pub const NAME: [&str; Axis::N_AXIS] = ["X", "Y", "Z"];
}
pub struct PointCoordinateRegulator {
pub subject: Rc<Point>,
Vectornaut marked this conversation as resolved Outdated

Whoops, here's a formatting mistake from the revision I made. I should've just written

impl Display for Axis

and changed the relevant use expression as follows:

- fmt::{Debug, Formatter}
+ fmt::{Debug, Display, Formatter}

Would you mind including that change in your next round of revisions?

Whoops, here's a formatting mistake from the revision I made. I should've just written ```rust impl Display for Axis ``` and changed the relevant `use` expression as follows: ```diff - fmt::{Debug, Formatter} + fmt::{Debug, Display, Formatter} ``` Would you mind including that change in your next round of revisions?

Yes of course, and this makes sense.

Yes of course, and this makes sense.
pub axis: Axis,
pub measurement: ReadSignal<f64>,
pub set_point: Signal<SpecifiedValue>,
serial: u64
}
impl PointCoordinateRegulator {
pub fn new(subject: Rc<Point>, axis: Axis) -> Self {
let measurement = subject.representation().map(
move |rep| rep[axis as usize]
);
let set_point = create_signal(SpecifiedValue::from_empty_spec());
Self { subject, axis, measurement, set_point, serial: Self::next_serial() }
}
}
impl Serial for PointCoordinateRegulator {
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 }
}
impl ProblemPoser for PointCoordinateRegulator {
fn pose(&self, problem: &mut ConstraintProblem) {
self.set_point.with_untracked(|set_pt| {
if let Some(val) = set_pt.value {
let col = self.subject.column_index().expect(
"Subject must be indexed before point-coordinate regulator poses.");
problem.frozen.push(self.axis as usize, col, val);
// Check if all three coordinates have been frozen, and if so,
// freeze the coradius as well
Vectornaut marked this conversation as resolved Outdated

Design

This kind of interaction between problem posers is something we didn't anticipate when designing the ProblemPoser trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a /* KLUDGE */ flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the ProblemPoser trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.

Testing

To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call

// log the Gram matrix
console_log!("Gram matrix:\n{}", problem.gram);

with

// log the Gram matrix and frozen entries
console_log!("Gram matrix:\n{}", problem.gram);
console_log!("Frozen entries:\n{}", problem.frozen);

I tried this out locally, and it confirms that the interaction system is working in at least some cases.

Commenting

I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the /* KLUDGE */ flag suggested above and uses the capitalization convention that most other comments follow.

// if the three representation components that specify the
// point's Euclidean coordinates have been frozen, then freeze
// the norm component too
/* KLUDGE */
// we didn't anticipate this kind of interaction when we
// designed the problem poser system, so the implementation
// feels awkward
### Design This kind of interaction between problem posers is something we didn't anticipate when designing the `ProblemPoser` trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a `/* KLUDGE */` flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request. ### Testing To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call ```rust // log the Gram matrix console_log!("Gram matrix:\n{}", problem.gram); ``` with ```rust // log the Gram matrix and frozen entries console_log!("Gram matrix:\n{}", problem.gram); console_log!("Frozen entries:\n{}", problem.frozen); ``` I tried this out locally, and it confirms that the interaction system is working in at least some cases. ### Commenting I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the `/* KLUDGE */` flag suggested above and uses the capitalization convention that most other comments follow. ```rust // if the three representation components that specify the // point's Euclidean coordinates have been frozen, then freeze // the norm component too /* KLUDGE */ // we didn't anticipate this kind of interaction when we // designed the problem poser system, so the implementation // feels awkward ```

@Vectornaut wrote in #118 (comment):

the best way to resolve the tension would be to redesign the ProblemPoser trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.

What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3259: > the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request. What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.

OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.

I agree that the comment in issue #90 is enough to remind us about this.

What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

We discussed this a little during our check-in meeting, and I jotted down a comment about our conclusions on issue #90.

I agree that the comment in issue #90 is enough to remind us about this. > What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks. We discussed this a little during our check-in meeting, and I jotted down a [comment](https://code.studioinfinity.org/StudioInfinity/dyna3/issues/90#issuecomment-3288) about our conclusions on issue #90.
let mut coords = [0.0; Axis::N_AXIS];
let mut nset: usize = 0;
for &MatrixEntry {index, value} in &(problem.frozen) {
if index.1 == col && index.0 < Axis::N_AXIS {

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

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".

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).

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.

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.

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.

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.
nset += 1;
coords[index.0] = value
}
Vectornaut marked this conversation as resolved Outdated

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.)

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.

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.

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.

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?

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.

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 { ... ```

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.
}
if nset == Axis::N_AXIS {
let [x, y, z] = coords;
problem.frozen.push(
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
}
}
});
}
}
Vectornaut marked this conversation as resolved

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.

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.
// the velocity is expressed in uniform coordinates
pub struct ElementMotion<'a> {
pub element: Rc<dyn Element>,

View file

@ -6,9 +6,11 @@ use web_sys::{KeyboardEvent, MouseEvent, wasm_bindgen::JsCast};
use crate::{
AppState,
assembly::{
Axis,
Element,
HalfCurvatureRegulator,
InversiveDistanceRegulator,
PointCoordinateRegulator,
Regulator,
},
specified::SpecifiedValue
@ -119,6 +121,19 @@ impl OutlineItem for HalfCurvatureRegulator {
}
}
impl OutlineItem for PointCoordinateRegulator {
fn outline_item(self: Rc<Self>, _element: &Rc<dyn Element>) -> View {
view! {
li(class = "regulator") {
div(class = "regulator-label") { (Axis::NAME[self.axis as usize]) }
div(class = "regulator-type") { "Coordinate" }
Vectornaut marked this conversation as resolved Outdated

In the outline items for previously implemented regulators, we use the regulator-label div to say which other element, if any, is subject to the regulator, and we use the regulator-type label to say what's being regulated. To bring the outline item for the point coordinate regulator in line with this convention, I'd rewrite it like this.

let reg_type = format!("{} coordinate", Axis::NAME[self.axis as usize]);
view! {
    li(class = "regulator") {
        div(class = "regulator-label") // for spacing
        div(class = "regulator-type") { (reg_type) }
        RegulatorInput(regulator = self)
        div(class = "status")
    }
}

I'm following the code organization we used for the half-curvature regulator, which is the only previously implemented single-subject regulator.

In the outline items for previously implemented regulators, we use the `regulator-label` div to say which other element, if any, is subject to the regulator, and we use the `regulator-type` label to say what's being regulated. To bring the outline item for the point coordinate regulator in line with this convention, I'd rewrite it like this. ```rust let reg_type = format!("{} coordinate", Axis::NAME[self.axis as usize]); view! { li(class = "regulator") { div(class = "regulator-label") // for spacing div(class = "regulator-type") { (reg_type) } RegulatorInput(regulator = self) div(class = "status") } } ``` I'm following the code organization we used for the half-curvature regulator, which is the only previously implemented single-subject regulator.

done in latest commit

done in latest commit

The code and the resulting interface both look like what I expect, so I'd say this is resolved.

The code and the resulting interface both look like what I expect, so I'd say this is resolved.
RegulatorInput(regulator = self)
div(class = "status")
}
}
}
}
// a list item that shows an element in an outline view of an assembly
#[component(inline_props)]
fn ElementOutlineItem(element: Rc<dyn Element>) -> View {

View file

@ -46,14 +46,14 @@ pub fn project_sphere_to_normalized(rep: &mut DVector<f64>) {
// normalize a point's representation vector by scaling
pub fn project_point_to_normalized(rep: &mut DVector<f64>) {
rep.scale_mut(0.5 / rep[3]);
rep.scale_mut(0.5 / rep[3]); //FIXME: This 3 should be Point::WEIGHT_COMPONENT
Vectornaut marked this conversation as resolved Outdated

I've been flagging desired edits like this with /* TO DO */ // This 3... rather than //FIXME: This 3.... I'd like to stick to that format, because I think keeping flags consistent makes it easier to search for flagged code. I just started a wiki page to document the code flags I've been using.

I've been flagging desired edits like this with `/* TO DO */ // This 3...` rather than `//FIXME: This 3...`. I'd like to stick to that format, because I think keeping flags consistent makes it easier to search for flagged code. I just started a wiki page to document the [code flags](wiki/Code-flags) I've been using.

I figured rather than any flag I would just go ahead and do it. But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState. How do we get rid of these? If there is an easy direct way please add a commit doing so. Or do we need to move Point into a different module commonly used by main and lib, and take AppState out of lib again? If so, let me know and I can go ahead and do that. And can we turn off the warning that hates on a module named appState.rs? To my mind, the clear place to define the AppState struct is in an appState module... Thanks.

I figured rather than any flag I would just go ahead and do it. But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState. How do we get rid of these? If there is an easy direct way please add a commit doing so. Or do we need to move Point into a different module commonly used by main and lib, and take AppState out of lib again? If so, let me know and I can go ahead and do that. And can we turn off the warning that hates on a module named appState.rs? To my mind, the clear place to define the AppState struct is in an appState module... Thanks.

I'd prefer to just add a /* TO DO */ flag, because I think the best way to fix the issue would involve a broader discussion about the organization of the engine and assembly submodules, which is outside the scope of this pull request. I've included a little intro to that discussion below.

But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState.

These warnings in the library build are in line with the way I've been thinking of the engine submodule: as something that lives at a lower level of abstraction than the assembly submodule, and could eventually be spun off into a stand-alone library that the assembly submodule interacts with through an API.

I see this design as a path toward making the engine modular, like we ultimately want it to be. The modularity is very imperfect right now, but the imperfections are all in the assembly submodule, which refers to implementation details of the engine that it shouldn't really need or have access to. The WEIGHT_COMPONENT and NORM_COMPONENTconstants associated with Point are examples of those imperfections.

One approach to fixing them might be to add a new submodule that acts as a translation layer between the assembly and engine submodules by implementing ProblemPoser for all the elements and regulators. This translation layer might also be a natural home for the project_point_to_normalized method, which wants to refer to WEIGHT_COMPONENT.

I'd prefer to just add a `/* TO DO */` flag, because I think the best way to fix the issue would involve a broader discussion about the organization of the engine and assembly submodules, which is outside the scope of this pull request. I've included a little intro to that discussion below. > But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState. These warnings in the library build are in line with the way I've been thinking of the engine submodule: as something that lives at a lower level of abstraction than the assembly submodule, and could eventually be spun off into a stand-alone library that the assembly submodule interacts with through an API. I see this design as a path toward making the engine modular, like we ultimately want it to be. The modularity is very imperfect right now, but the imperfections are all in the assembly submodule, which refers to implementation details of the engine that it shouldn't really need or have access to. The `WEIGHT_COMPONENT` and `NORM_COMPONENT`constants associated with `Point` are examples of those imperfections. One approach to fixing them might be to add a new submodule that acts as a translation layer between the assembly and engine submodules by implementing `ProblemPoser` for all the elements and regulators. This translation layer might also be a natural home for the `project_point_to_normalized` method, which wants to refer to `WEIGHT_COMPONENT`.

This is now covered by issue #120. Thanks for posting that!

This is now covered by issue #120. Thanks for posting that!
}
// --- partial matrices ---
pub struct MatrixEntry {
index: (usize, usize),
value: f64,
pub index: (usize, usize),
pub value: f64,
}
pub struct PartialMatrix(Vec<MatrixEntry>);