feat: Point coordinate regulators #118

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

View file

@ -31,9 +31,14 @@ The latest prototype is in the folder `app-proto`. It includes both a user inter
3. Call `rustup target add wasm32-unknown-unknown` to add the [most generic 32-bit WebAssembly target](https://doc.rust-lang.org/nightly/rustc/platform-support/wasm32-unknown-unknown.html)
4. Call `cargo install wasm-pack` to install the [WebAssembly toolchain](https://rustwasm.github.io/docs/wasm-pack/)
5. Call `cargo install trunk` to install the [Trunk](https://trunkrs.dev/) web-build tool
- In the future, `trunk` can be updated with the same command. You may

Very minor: the rest of the list is formatted as one sentence per list item, with no final period. It would be nice to keep that consistent.

Very minor: the rest of the list is formatted as one sentence per list item, with no final period. It would be nice to keep that consistent.

OK, I have properly punctuated the whole file, so that complete sentences have periods, and fragments do not.

OK, I have properly punctuated the whole file, so that complete sentences have periods, and fragments do not.
need the `--locked` flag if your ambient version of `rustc` does not
match that required by `trunk`.
Vectornaut marked this conversation as resolved Outdated

The information here looks accurate to me, although I've never installed a global tool with the --locked flag, and I don't know whether it's recommended.

The existing style for this file does not use manual line-wrapping. I'd prefer to keep the new text consistent with that style by putting it all on one line.

The information here looks accurate to me, although I've never installed a global tool with the `--locked` flag, and I don't know whether it's recommended. The existing style for this file does not use manual line-wrapping. I'd prefer to keep the new text consistent with that style by putting it all on one line.

Ok, I will go ahead and remove the newlines in the additions to this file, but i will say that trying to impose formatting rules like this on markdown files feels rather contradictory to the spirit/genesis of markdown: it's an explicit reaction to various markup languages to be more free-form and lightweight.

Ok, I will go ahead and remove the newlines in the additions to this file, but i will say that trying to impose formatting rules like this on markdown files feels rather contradictory to the spirit/genesis of markdown: it's an explicit reaction to various markup languages to be more free-form and lightweight.

--locked is in fact now part of the installation command that trunk itself recommends, so i feel pretty safe recommending it here.

`--locked` is in fact now part of the installation command that trunk itself recommends, so i feel pretty safe recommending it here.
6. Add the `.cargo/bin` folder in your home directory to your executable search path
- This lets you call Trunk, and other tools installed by Cargo, without specifying their paths
- On POSIX systems, the search path is stored in the `PATH` environment variable
- Alternatively, if you don't want to adjust your `PATH`, you can install
`trunk` in another directory `DIR` via `cargo install --root DIR trunk`

I'll trust your experience with installing Cargo tools at custom locations, since I've never tried it!

The request above about manual line-wrapping also applies here.

I'll trust your experience with installing Cargo tools at custom locations, since I've never tried it! The [request](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3346) above about manual line-wrapping also applies here.
### Play with the prototype

View file

@ -5,7 +5,7 @@ use std::{
cmp::Ordering,
collections::{BTreeMap, BTreeSet},
fmt,
fmt::{Debug, Formatter},
fmt::{Debug, Display, Formatter},
hash::{Hash, Hasher},
rc::Rc,
sync::{atomic, atomic::AtomicU64},
@ -128,7 +128,7 @@ pub trait Element: Serial + ProblemPoser + DisplayItem {
impl Debug for dyn Element {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
glen marked this conversation as resolved Outdated

This change is irrelevant to this PR so we really oughtnt do it but no big deal I guess.

This change is irrelevant to this PR so we really oughtnt do it but no big deal I guess.

I'd be fine with leaving it out of this pull request. I made it because this pull request implements Display for Axis using the terser fmt::Result convention, and I wanted the previous implementation of Debug for dyn Element to look consistent.

I'd be fine with leaving it out of this pull request. I made it because this pull request implements `Display` for `Axis` using the terser `fmt::Result` convention, and I wanted the previous implementation of `Debug` for `dyn Element` to look consistent.

like I said, no big deal either way, resolving.

like I said, no big deal either way, resolving.
self.id().fmt(f)
Debug::fmt(&self.id(), f)
}
}
@ -248,13 +248,19 @@ impl Serial for Sphere {
}
}
fn index_message(thing: &str, name: &str, actor: &str) -> String {
format!(
"{thing} \"{name}\" must be indexed before {actor} writes problem data"
)
}
impl ProblemPoser for Sphere {
fn pose(&self, problem: &mut ConstraintProblem) {
let index = self.column_index().expect(
format!("Sphere \"{}\" should be indexed before writing problem data", self.id).as_str()
);
index_message("Sphere", &self.id, "it").as_str());
problem.gram.push_sym(index, index, 1.0);
problem.guess.set_column(index, &self.representation.get_clone_untracked());
problem.guess.set_column(
index, &self.representation.get_clone_untracked());
}
}
@ -357,11 +363,11 @@ impl Serial for Point {
impl ProblemPoser for Point {
fn pose(&self, problem: &mut ConstraintProblem) {
let index = self.column_index().expect(
format!("Point \"{}\" should be indexed before writing problem data", self.id).as_str()
);
index_message("Point", &self.id, "it").as_str());
problem.gram.push_sym(index, index, 0.0);
problem.frozen.push(Self::WEIGHT_COMPONENT, index, 0.5);
problem.guess.set_column(index, &self.representation.get_clone_untracked());
problem.guess.set_column(
index, &self.representation.get_clone_untracked());
}
}
@ -406,7 +412,8 @@ pub struct InversiveDistanceRegulator {
impl InversiveDistanceRegulator {
pub fn new(subjects: [Rc<dyn Element>; 2]) -> Self {
let representations = subjects.each_ref().map(|subj| subj.representation());
let representations = subjects.each_ref().map(
|subj| subj.representation());
let measurement = create_memo(move || {
representations[0].with(|rep_0|
representations[1].with(|rep_1|
@ -448,8 +455,8 @@ impl ProblemPoser for InversiveDistanceRegulator {
if let Some(val) = set_pt.value {
let [row, col] = self.subjects.each_ref().map(
|subj| subj.column_index().expect(
"Subjects should be indexed before inversive distance regulator writes problem data"
)
index_message("Subject", subj.id(),
"inversive distance regulator").as_str())
);
problem.gram.push_sym(row, col, val);
}
@ -502,8 +509,8 @@ impl ProblemPoser for HalfCurvatureRegulator {
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 half-curvature regulator writes problem data"
);
index_message("Subject", &self.subject.id,
"half-curvature regulator").as_str());
problem.frozen.push(Sphere::CURVATURE_COMPONENT, col, val);
}
});
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.
@ -519,7 +526,7 @@ impl Axis {
}
}
impl fmt::Display for Axis {
impl Display for Axis {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(self.name())
}
@ -539,7 +546,9 @@ impl PointCoordinateRegulator {
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() }
Self {
subject, axis, measurement, set_point, serial: Self::next_serial()
}
}
}
@ -558,10 +567,11 @@ 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 must be indexed before point-coordinate regulator poses.");
index_message("Subject", &self.subject.id,
"point-coordinate regulator").as_str());
problem.frozen.push(self.axis as usize, col, val);
// Check if all three spatial coordinates have been frozen, and if so,
// freeze the norm component as well
// If all three of the subject's spatial coordinates have been
// frozen, then freeze its 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.
let mut coords = [0.0; Axis::CARDINALITY];
let mut nset: usize = 0;
for &MatrixEntry {index, value} in &(problem.frozen) {
@ -573,7 +583,9 @@ impl ProblemPoser for PointCoordinateRegulator {
if nset == Axis::CARDINALITY {
let [x, y, z] = coords;
problem.frozen.push(
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
Point::NORM_COMPONENT,
col,
point(x,y,z)[Point::NORM_COMPONENT]);
}
}
});
@ -672,7 +684,8 @@ impl Assembly {
let id = elt.id().clone();
let elt_rc = Rc::new(elt);
self.elements.update(|elts| elts.insert(elt_rc.clone()));
self.elements_by_id.update(|elts_by_id| elts_by_id.insert(id, elt_rc.clone()));
self.elements_by_id.update(
|elts_by_id| elts_by_id.insert(id, elt_rc.clone()));
// create and insert the element's default regulators
for reg in elt_rc.default_regulators() {

Flagging this as a potential mistake because it adds a new change to the pull request, rather than reverting one of the changes in commit c0e6ebf, and I don't see an obvious reason for this pull request to touch it. Could you confirm that this change was deliberate if it was, and revert it if it wasn't?

Flagging this as a potential mistake because it adds a new change to the pull request, rather than reverting one of the changes in commit c0e6ebf, and I don't see an obvious reason for this pull request to touch it. Could you confirm that this change was deliberate if it was, and revert it if it wasn't?

Oh, when I was undoing, there was some exactly analogous line that was the other way, so it was easy just to falsely revert this. I will fix the mistaken non-reversion.

Oh, when I was undoing, there was some exactly analogous line that was the other way, so it was easy just to falsely revert this. I will fix the mistaken non-reversion.
@ -748,7 +761,8 @@ impl Assembly {
pub fn load_config(&self, config: &DMatrix<f64>) {
for elt in self.elements.get_clone_untracked() {
elt.representation().update(
|rep| rep.set_column(0, &config.column(elt.column_index().unwrap()))
|rep| rep.set_column(
0, &config.column(elt.column_index().unwrap()))
);
}
}
@ -893,7 +907,8 @@ impl Assembly {
if column_index < realized_dim {
// this element had a column index when we started, so by
// invariant (1), it's reflected in the tangent space
let mut target_columns = motion_proj.columns_mut(0, realized_dim);
let mut target_columns =
motion_proj.columns_mut(0, realized_dim);
target_columns += self.tangent.with(
|tan| tan.proj(&elt_motion.velocity, column_index)
);
@ -901,9 +916,10 @@ impl Assembly {
// this element didn't have a column index when we started, so
// by invariant (2), it's unconstrained
let mut target_column = motion_proj.column_mut(column_index);
let unif_to_std = elt_motion.element.representation().with_untracked(
|rep| local_unif_to_std(rep.as_view())
);
let unif_to_std = elt_motion
.element.representation()
.with_untracked(
|rep| local_unif_to_std(rep.as_view()));
target_column += unif_to_std * elt_motion.velocity;
}
}
@ -920,7 +936,9 @@ impl Assembly {
elt.project_to_normalized(rep);
},
None => {
console_log!("No velocity to unpack for fresh element \"{}\"", elt.id())
console_log!(
"No velocity to unpack for fresh element \"{}\"",
elt.id())
},
};
});
@ -940,7 +958,8 @@ mod tests {
use crate::engine;

I'd indent this line by two tabs to match the convention you've used for other multi-line expressions. This won't affect the string, because the string continuation escape absorbs all subsequent whitespace, resuming the string at the next non-whitespace character.

I'd indent this line by two tabs to match the convention you've used for other multi-line expressions. This won't affect the string, because the [string continuation escape](https://doc.rust-lang.org/reference/expressions/literal-expr.html#string-continuation-escapes) absorbs all subsequent whitespace, resuming the string at the next non-whitespace character.

Ah, didn't know that \ at end of line (a convention I hate, but apparently the only one available in this context) sucked up leading whitespace on the next line as well. Will do.

Ah, didn't know that \ at end of line (a convention I hate, but apparently the only one available in this context) sucked up leading whitespace on the next line as well. Will do.
#[test]
#[should_panic(expected = "Sphere \"sphere\" should be indexed before writing problem data")]
#[should_panic(expected =
Vectornaut marked this conversation as resolved

When I originally wrote this test, it bothered me that the identifiers sphere0 and sphere1 were assigned but never tested. The new, more informative indexing error message makes them part of the test, which is very satisfying.

When I originally wrote this test, it bothered me that the identifiers `sphere0` and `sphere1` were assigned but never tested. The new, more informative indexing error message makes them part of the test, which is very satisfying.
"Sphere \"sphere\" must be indexed before it writes problem data")]
fn unindexed_element_test() {
let _ = create_root(|| {
let elt = Sphere::default("sphere".to_string(), 0);
@ -949,17 +968,20 @@ mod tests {
}
#[test]
#[should_panic(expected = "Subjects should be indexed before inversive distance regulator writes problem data")]
#[should_panic(expected = "Subject \"sphere1\" must be indexed before \
inversive distance regulator writes problem data")]
fn unindexed_subject_test_inversive_distance() {
let _ = create_root(|| {
let subjects = [0, 1].map(
|k| Rc::new(Sphere::default(format!("sphere{k}"), k)) as Rc<dyn Element>
|k| Rc::new(Sphere::default(
format!("sphere{k}"), k)) as Rc<dyn Element>
);
subjects[0].set_column_index(0);
InversiveDistanceRegulator {
subjects: subjects,
measurement: create_memo(|| 0.0),
set_point: create_signal(SpecifiedValue::try_from("0.0".to_string()).unwrap()),
set_point: create_signal(
SpecifiedValue::try_from("0.0".to_string()).unwrap()),
serial: InversiveDistanceRegulator::next_serial()
}.pose(&mut ConstraintProblem::new(2));
});
@ -988,8 +1010,10 @@ mod tests {
// nudge the sphere repeatedly along the `z` axis
const STEP_SIZE: f64 = 0.0025;
const STEP_CNT: usize = 400;
let sphere = assembly.elements_by_id.with(|elts_by_id| elts_by_id[sphere_id].clone());
let velocity = DVector::from_column_slice(&[0.0, 0.0, STEP_SIZE, 0.0]);
let sphere = assembly.elements_by_id.with(
|elts_by_id| elts_by_id[sphere_id].clone());
let velocity =
DVector::from_column_slice(&[0.0, 0.0, STEP_SIZE, 0.0]);
for _ in 0..STEP_CNT {
assembly.deform(
vec![
@ -1007,7 +1031,8 @@ mod tests {
let final_half_curv = sphere.representation().with_untracked(
|rep| rep[Sphere::CURVATURE_COMPONENT]
);
assert!((final_half_curv / INITIAL_HALF_CURV - 1.0).abs() < DRIFT_TOL);
assert!((final_half_curv / INITIAL_HALF_CURV - 1.0).abs()
< DRIFT_TOL);
});
}
}
}