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

chore: wrap at 80 characters #128

Open
glen wants to merge 3 commits from glen/dyna3:aroundLineInEightyChars into main
pull from: glen/dyna3:aroundLineInEightyChars
merge into: StudioInfinity:main
Conversation 15 Commits 3 Files changed 7 +311 -150
7 changed files with 311 additions and 150 deletions
Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 4a3f47c8b5 - Show all commits

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

@ -261,7 +261,8 @@ impl ProblemPoser for Sphere {
let index = self.column_index().expect(
indexing_error("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());
}
}
@ -367,7 +368,8 @@ impl ProblemPoser for Point {
indexing_error("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());
}
}
@ -412,7 +414,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|
@ -518,6 +521,7 @@ impl ProblemPoser for HalfCurvatureRegulator {
#[derive(Clone, Copy, Sequence)]
pub enum Axis { X = 0, Y = 1, Z = 2 }
impl Axis {
fn name(&self) -> &'static str {
match self { Axis::X => "X", Axis::Y => "Y", Axis::Z => "Z" }
@ -544,7 +548,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()
}
}
}
@ -578,8 +584,8 @@ 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]);
problem.frozen.push(Point::NORM_COMPONENT,
col, point(x,y,z)[Point::NORM_COMPONENT]);
}
}
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 07:36:39 +00:00
Outdated
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut commented 2025-11-27 00:00:15 +00:00
Copy link

Our current convention is to put spaces after the commas between arguments.

Our current convention is to put spaces after the commas between arguments.
});
@ -678,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() {
@ -754,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()))
);
}
}
@ -899,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)
);
@ -907,7 +916,8 @@ 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(
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;
@ -926,7 +936,10 @@ 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()
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 07:39:44 +00:00
Outdated
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
)
},
};
});
@ -961,13 +974,15 @@ mod tests {
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>
Vectornaut commented 2025-11-22 07:45:23 +00:00
Outdated
Copy link

I find this quite hard to read. Would you be willing to consider something like this?

let subjects = [0, 1].map(
    |k| Rc::new(
        Sphere::default(format!("sphere{k}"), k)
    ) as Rc<dyn Element>);
I find this quite hard to read. Would you be willing to consider something like this? ```rust let subjects = [0, 1].map( |k| Rc::new( Sphere::default(format!("sphere{k}"), k) ) as Rc<dyn Element>); ```
glen commented 2025-11-22 15:39:07 +00:00
Outdated
Copy link

Yes this is a tricky one to split. My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case? Anyhow, I will give it another shot.

Yes this is a tricky one to split. My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case? Anyhow, I will give it another shot.
Vectornaut commented 2025-11-23 06:53:49 +00:00
Outdated
Copy link

My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case?

My preferences are:

  1. In languages with C-like syntax, show multi-line nesting using a combination of indentation and paired delimiters, like this: 
    foo(
    |u| {
        /* function body */
    },
    [
        /* array contents */
    ],
);
  2. In Rust, following convention, show multi-line chaining using hanging indent and leading ., like this: 
    long_variable_name
    .method_call()
    .field_access

Since (1) conflicts with your preferences, I asked for (2) in cases where that could prevent the violation of (1).

> My current attempt was motivated by your apparent preference to split before a `.`, rather than just after a `(`. Is that not the case? My preferences are: 1. In languages with C-like syntax, show multi-line nesting using a combination of indentation and paired delimiters, like this: ``` foo( |u| { /* function body */ }, [ /* array contents */ ], ); 2. In Rust, following convention, show multi-line chaining using hanging indent and leading `.`, like this: ``` long_variable_name .method_call() .field_access ``` Since (1) conflicts with your preferences, I asked for (2) in cases where that could prevent the violation of (1).
Vectornaut commented 2025-11-23 07:04:23 +00:00
Copy link

I find (2) above most readable when more than one dot item is chained, so I guess I'd be fine with switching back to Python-style hanging indent for single method calls. I realize that this is what you had in the previous commit, and I apologize for the flip-flop. I think I'm going to have to give up on finding a compromise position on (1) and just deal with formatting I find difficult to read until we use Husht to switch to a non-C-like syntax.

I find (2) above most readable when more than one dot item is chained, so I guess I'd be fine with switching back to Python-style hanging indent for single method calls. I realize that this is what you had in the previous commit, and I apologize for the flip-flop. I think I'm going to have to give up on finding a compromise position on (1) and just deal with formatting I find difficult to read until we use Husht to switch to a non-C-like syntax.
);
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));
});
@ -996,8 +1011,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![
@ -1015,7 +1032,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);
});
}
}

4
app-proto/src/components/add_remove.rs
View file

@ -39,7 +39,9 @@ pub fn AddRemove() -> View {
}
) { "Add point" }
button(
class = "emoji", /* KLUDGE */ // for convenience, we're using an emoji as a temporary icon for this button
/* KLUDGE */ // for convenience, we're using an emoji as an
// icon for this button
class = "emoji",
disabled = {
let state = use_context::<AppState>();
state.selection.with(|sel| sel.len() != 2)

116
app-proto/src/components/display.rs
View file

@ -50,7 +50,8 @@ impl SceneSpheres {
}
fn len_i32(&self) -> i32 {
self.representations.len().try_into().expect("Number of spheres must fit in a 32-bit integer")
self.representations.len().try_into().expect(
"Number of spheres must fit in a 32-bit integer")
}
fn push(
@ -127,8 +128,12 @@ impl DisplayItem for Sphere {
const HIGHLIGHT: f32 = 0.2;
let representation = self.representation.get_clone_untracked();
let color = if selected { self.color.map(|channel| 0.2 + 0.8*channel) } else { self.color };
let opacity = if self.ghost.get() { GHOST_OPACITY } else { DEFAULT_OPACITY };
let color =
if selected { self.color.map(|channel| 0.2 + 0.8*channel) }
else { self.color };
let opacity =
if self.ghost.get() { GHOST_OPACITY }
else { DEFAULT_OPACITY };
let highlight = if selected { 1.0 } else { HIGHLIGHT };
scene.spheres.push(representation, color, opacity, highlight);
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 07:50:57 +00:00
Outdated
Copy link

I think this is easier to read if either both branches are set off or both branches are inlined:

let opacity = if self.ghost.get() {
    GHOST_OPACITY
} else {
    DEFAULT_OPACITY
};

let opacity =
    if self.ghost.get() { GHOST_OPACITY }
    else { DEFAULT_OPACITY };

The latter option is already used on line 146 of outline.rs.

I think this is easier to read if either both branches are set off or both branches are inlined: ```rust let opacity = if self.ghost.get() { GHOST_OPACITY } else { DEFAULT_OPACITY }; ``` ```rust let opacity = if self.ghost.get() { GHOST_OPACITY } else { DEFAULT_OPACITY }; ``` The latter option is already used on line 146 of `outline.rs`.
glen commented 2025-11-22 15:42:12 +00:00
Outdated
Copy link

Won't surprise you that I don't see any importance for the two branches of a single if to match in this way, but modulo the location of the = the second is fine so once we figure the = out (always previous line, always next line, or prefer one way but mixed is ok when it helps with layout) i will change this.

Won't surprise you that I don't see any importance for the two branches of a single if to match in this way, but modulo the location of the = the second is fine so once we figure the = out (always previous line, always next line, or prefer one way but mixed is ok when it helps with layout) i will change this.
}
@ -145,7 +150,8 @@ impl DisplayItem for Sphere {
// `a*u^2 + b*u + c` by the linear function `b*u + c`
const DEG_THRESHOLD: f64 = 1e-9;
let rep = self.representation.with_untracked(|rep| assembly_to_world * rep);
let rep = self.representation.with_untracked(
|rep| assembly_to_world * rep);
let a = -rep[3] * dir.norm_squared();
let b = rep.rows_range(..3).dot(&dir);
let c = -rep[4];
@ -186,7 +192,9 @@ impl DisplayItem for Point {
const HIGHLIGHT: f32 = 0.5;
let representation = self.representation.get_clone_untracked();
let color = if selected { self.color.map(|channel| 0.2 + 0.8*channel) } else { self.color };
let color =
if selected { self.color.map(|channel| 0.2 + 0.8*channel) }
else { self.color };
let opacity = if self.ghost.get() { GHOST_OPACITY } else { 1.0 };
let highlight = if selected { 1.0 } else { HIGHLIGHT };
Vectornaut commented 2025-11-22 07:51:56 +00:00
Outdated
Copy link

I think this is easier to read if either both branches are set off or both branches are inlined, like above on lines 136–138.

I think this is easier to read if either both branches are set off or both branches are inlined, like above on lines 136–138.
scene.points.push(representation, color, opacity, highlight, selected);
@ -199,7 +207,8 @@ impl DisplayItem for Point {
assembly_to_world: &DMatrix<f64>,
pixel_size: f64,
) -> Option<f64> {
let rep = self.representation.with_untracked(|rep| assembly_to_world * rep);
let rep = self.representation.with_untracked(
|rep| assembly_to_world * rep);
if rep[2] < 0.0 {
// this constant should be kept synchronized with `point.frag`
const POINT_RADIUS_PX: f64 = 4.0;
@ -357,11 +366,12 @@ fn event_dir(event: &MouseEvent) -> (Vector3<f64>, f64) {
// this constant should be kept synchronized with `spheres.frag` and
// `point.vert`
const FOCAL_SLOPE: f64 = 0.3;
let horizontal = f64::from(event.client_x()) - rect.left();
let vertical = rect.bottom() - f64::from(event.client_y());
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-13 05:45:20 +00:00
Outdated
Copy link

I think names like x_relative, y_relative or x_rel, y_rel would be more descriptive, and more analogous with the original values event.client_x(), event.client_y(), than the current names horizontal, vertical.

I think names like `x_relative`, `y_relative` or `x_rel`, `y_rel` would be more descriptive, and more analogous with the original values `event.client_x()`, `event.client_y()`, than the current names `horizontal`, `vertical`.
glen commented 2025-11-21 21:13:01 +00:00
Outdated
Copy link

Renamed to x_relative, y_relative.

Renamed to x_relative, y_relative.
👍 1
(
Vector3::new(
FOCAL_SLOPE * (2.0*(f64::from(event.client_x()) - rect.left()) - width) / shortdim,
FOCAL_SLOPE * (2.0*(rect.bottom() - f64::from(event.client_y())) - height) / shortdim,
FOCAL_SLOPE * (2.0*horizontal - width) / shortdim,
FOCAL_SLOPE * (2.0*vertical - height) / shortdim,
-1.0,
),
FOCAL_SLOPE * 2.0 / shortdim,
@ -445,7 +455,8 @@ pub fn Display() -> View {
let performance = window().unwrap().performance().unwrap();
// get the display canvas
let canvas = display.get().unchecked_into::<web_sys::HtmlCanvasElement>();
let canvas =
display.get().unchecked_into::<web_sys::HtmlCanvasElement>();
let ctx = canvas
.get_context("webgl2")
.unwrap()
@ -458,7 +469,8 @@ pub fn Display() -> View {
// set blend mode
ctx.enable(WebGl2RenderingContext::BLEND);
ctx.blend_func(WebGl2RenderingContext::SRC_ALPHA, WebGl2RenderingContext::ONE_MINUS_SRC_ALPHA);
ctx.blend_func(WebGl2RenderingContext::SRC_ALPHA,
WebGl2RenderingContext::ONE_MINUS_SRC_ALPHA);
// set up the sphere rendering program
let sphere_program = set_up_program(
@ -487,16 +499,20 @@ pub fn Display() -> View {
// machine, the the length of a float or genType array seems to be
// capped at 1024 elements
console::log_2(
&ctx.get_parameter(WebGl2RenderingContext::MAX_FRAGMENT_UNIFORM_VECTORS).unwrap(),
&ctx.get_parameter(
WebGl2RenderingContext::MAX_FRAGMENT_UNIFORM_VECTORS).unwrap(),
&JsValue::from("uniform vectors available"),
);
// find the sphere program's vertex attribute
let viewport_position_attr = ctx.get_attrib_location(&sphere_program, "position") as u32;
let viewport_position_attr =
ctx.get_attrib_location(&sphere_program, "position") as u32;
// find the sphere program's uniforms
const SPHERE_MAX: usize = 200;
let sphere_cnt_loc = ctx.get_uniform_location(&sphere_program, "sphere_cnt");
let sphere_cnt_loc = ctx.get_uniform_location(
&sphere_program, "sphere_cnt"
);
let sphere_sp_locs = get_uniform_array_locations::<SPHERE_MAX>(
&ctx, &sphere_program, "sphere_list", Some("sp")
);
@ -509,10 +525,18 @@ pub fn Display() -> View {
let sphere_highlight_locs = get_uniform_array_locations::<SPHERE_MAX>(
&ctx, &sphere_program, "highlight_list", None
);
let resolution_loc = ctx.get_uniform_location(&sphere_program, "resolution");
let shortdim_loc = ctx.get_uniform_location(&sphere_program, "shortdim");
let layer_threshold_loc = ctx.get_uniform_location(&sphere_program, "layer_threshold");
let debug_mode_loc = ctx.get_uniform_location(&sphere_program, "debug_mode");
let resolution_loc = ctx.get_uniform_location(
&sphere_program, "resolution"
);
let shortdim_loc = ctx.get_uniform_location(
&sphere_program, "shortdim"
);
let layer_threshold_loc = ctx.get_uniform_location(
&sphere_program, "layer_threshold"
);
let debug_mode_loc = ctx.get_uniform_location(
&sphere_program, "debug_mode"
);
// load the viewport vertex positions into a new vertex buffer object
const VERTEX_CNT: usize = 6;
@ -526,13 +550,18 @@ pub fn Display() -> View {
1.0, 1.0, 0.0,
1.0, -1.0, 0.0,
];
let viewport_position_buffer = load_new_buffer(&ctx, &viewport_positions);
let viewport_position_buffer =
load_new_buffer(&ctx, &viewport_positions);
// find the point program's vertex attributes
let point_position_attr = ctx.get_attrib_location(&point_program, "position") as u32;
let point_color_attr = ctx.get_attrib_location(&point_program, "color") as u32;
let point_highlight_attr = ctx.get_attrib_location(&point_program, "highlight") as u32;
let point_selection_attr = ctx.get_attrib_location(&point_program, "selected") as u32;
let point_position_attr =
ctx.get_attrib_location(&point_program, "position") as u32;
let point_color_attr =
ctx.get_attrib_location(&point_program, "color") as u32;
let point_highlight_attr =
ctx.get_attrib_location(&point_program, "highlight") as u32;
let point_selection_attr =
ctx.get_attrib_location(&point_program, "selected") as u32;
// set up a repainting routine
let (_, start_animation_loop, _) = create_raf(move || {
@ -596,7 +625,8 @@ pub fn Display() -> View {
let realization_successful = state.assembly.realization_status.with(
|status| status.is_ok()
);
let step_val = state.assembly.step.with_untracked(|step| step.value);
let step_val =
state.assembly.step.with_untracked(|step| step.value);
let on_init_step = step_val.is_some_and(|n| n == 0.0);
let on_last_step = step_val.is_some_and(
|n| state.assembly.descent_history.with_untracked(
@ -606,7 +636,8 @@ pub fn Display() -> View {
let on_manipulable_step =
!realization_successful && on_init_step
|| realization_successful && on_last_step;
if on_manipulable_step && state.selection.with(|sel| sel.len() == 1) {
if on_manipulable_step
&& state.selection.with(|sel| sel.len() == 1) {
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 07:55:19 +00:00
Outdated
Copy link

I think the line beginning with && should be indented four spaces. (Currently, it's only indented three.)

I think the line beginning with `&&` should be indented four spaces. (Currently, it's only indented three.)
glen commented 2025-11-22 15:42:51 +00:00
Outdated
Copy link

Yes typo (I have not yet bothered to install a rust mode for emacs), will fix

Yes typo (I have not yet bothered to install a rust mode for emacs), will fix
👍 1
let sel = state.selection.with(
|sel| sel.into_iter().next().unwrap().clone()
);
@ -651,7 +682,8 @@ pub fn Display() -> View {
// measure mean frame interval
frames_since_last_sample += 1;
if frames_since_last_sample >= SAMPLE_PERIOD {
mean_frame_interval.set((time - last_sample_time) / (SAMPLE_PERIOD as f64));
mean_frame_interval.set(
(time - last_sample_time) / (SAMPLE_PERIOD as f64));
last_sample_time = time;
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 07:58:42 +00:00
Outdated
Copy link

The new variable SP64 is confusing to me for various reasons. Since we're taking an extra line anyway, could we do something like this instead?

mean_frame_interval.set(
    (time - last_sample_time) / SAMPLE_PERIOD as f64);
The new variable `SP64` is confusing to me for various reasons. Since we're taking an extra line anyway, could we do something like this instead? ```rust mean_frame_interval.set( (time - last_sample_time) / SAMPLE_PERIOD as f64); ```
glen commented 2025-11-22 15:47:17 +00:00
Outdated
Copy link

Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Abbreviation is one nice common way of dealing with line breaks. And Rust's conversion/coercion rules/syntax are just truly annoying, so i find the formula much easier to read without internal coercion. What else might we call the local abbreviation for the coercion of the global constant with the very long name?

Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Abbreviation is one nice common way of dealing with line breaks. And Rust's conversion/coercion rules/syntax are just truly annoying, so i find the formula much easier to read without internal coercion. What else might we call the local abbreviation for the coercion of the global constant with the very long name?
Vectornaut commented 2025-11-23 07:07:37 +00:00
Copy link

Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments.

Oh, I see. Like I said above, I may have to just accept going back to Python-style hanging indent for function calls and deal with the cognitive dissonance until we get to a non-C-like syntax with Husht.

What else might we call the local abbreviation for the coercion of the global constant with the very long name?

I'd use sample_period_f64.

> Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Oh, I see. Like I said [above](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3492), I may have to just accept going back to Python-style hanging indent for function calls and deal with the cognitive dissonance until we get to a non-C-like syntax with Husht. > What else might we call the local abbreviation for the coercion of the global constant with the very long name? I'd use `sample_period_f64`.
glen commented 2025-11-25 00:09:29 +00:00
Copy link

Well, that's too long to avoid the line break, so ok, I will revert to the function call being split across lines.

Well, that's too long to avoid the line break, so ok, I will revert to the function call being split across lines.
glen commented 2025-11-25 00:12:00 +00:00
Copy link

Oh, actually it fits on two lines like this

                    mean_frame_interval
                        .set((time - last_sample_time) / SAMPLE_PERIOD as f64);

which I presume you like better. Don't know why I didn't see that before. So will do that in the next revision.

Oh, actually it fits on two lines like this ``` mean_frame_interval .set((time - last_sample_time) / SAMPLE_PERIOD as f64); ``` which I presume you like better. Don't know why I didn't see that before. So will do that in the next revision.
frames_since_last_sample = 0;
}
@ -676,7 +708,8 @@ pub fn Display() -> View {
// set up the scene
state.assembly.elements.with_untracked(
|elts| for elt in elts {
let selected = state.selection.with(|sel| sel.contains(elt));
let selected =
state.selection.with(|sel| sel.contains(elt));
elt.show(&mut scene, selected);
}
);
@ -691,7 +724,8 @@ pub fn Display() -> View {
ctx.enable_vertex_attrib_array(viewport_position_attr);
// write the spheres in world coordinates
let sphere_reps_world: Vec<_> = scene.spheres.representations.into_iter().map(
let sphere_reps_world: Vec<_> =
scene.spheres.representations.into_iter().map(
|rep| (&asm_to_world * rep).cast::<f32>()
).collect();
Vectornaut commented 2025-11-22 08:01:33 +00:00
Outdated
Copy link

The revised version is pretty close to the one-per-line convention for chained method calls. I'd recommend going all the way there:

let sphere_reps_world: Vec<_> = scene.spheres.representations
    .into_iter()
    .map(|rep| (&asm_to_world * rep).cast::<f32>())
    .collect();
The revised version is pretty close to the one-per-line convention for chained method calls. I'd recommend going all the way there: ```rust let sphere_reps_world: Vec<_> = scene.spheres.representations .into_iter() .map(|rep| (&asm_to_world * rep).cast::<f32>()) .collect(); ```
@ -729,10 +763,12 @@ pub fn Display() -> View {
// bind the viewport vertex position buffer to the position
// attribute in the vertex shader
bind_to_attribute(&ctx, viewport_position_attr, SPACE_DIM as i32, &viewport_position_buffer);
bind_to_attribute(&ctx, viewport_position_attr,
SPACE_DIM as i32, &viewport_position_buffer);
// draw the scene
ctx.draw_arrays(WebGl2RenderingContext::TRIANGLES, 0, VERTEX_CNT as i32);
ctx.draw_arrays(WebGl2RenderingContext::TRIANGLES, 0,
VERTEX_CNT as i32);
// disable the sphere program's vertex attribute
ctx.disable_vertex_attrib_array(viewport_position_attr);
@ -760,13 +796,19 @@ pub fn Display() -> View {
// load the point positions and colors into new buffers and
// bind them to the corresponding attributes in the vertex
// shader
bind_new_buffer_to_attribute(&ctx, point_position_attr, SPACE_DIM as i32, point_positions.as_slice());
bind_new_buffer_to_attribute(&ctx, point_color_attr, (COLOR_SIZE + 1) as i32, scene.points.colors_with_opacity.concat().as_slice());
bind_new_buffer_to_attribute(&ctx, point_highlight_attr, 1 as i32, scene.points.highlights.as_slice());
bind_new_buffer_to_attribute(&ctx, point_selection_attr, 1 as i32, scene.points.selections.as_slice());
bind_new_buffer_to_attribute(&ctx, point_position_attr,
SPACE_DIM as i32, point_positions.as_slice());
bind_new_buffer_to_attribute(&ctx, point_color_attr,
(COLOR_SIZE + 1) as i32,
scene.points.colors_with_opacity.concat().as_slice());
bind_new_buffer_to_attribute(&ctx, point_highlight_attr,
1i32, scene.points.highlights.as_slice());
bind_new_buffer_to_attribute(&ctx, point_selection_attr,
1i32, scene.points.selections.as_slice());
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:03:19 +00:00
Outdated
Copy link

For consistency, I'd start the arguments on the next line here too.

bind_new_buffer_to_attribute(
    &ctx, point_color_attr,
    (COLOR_SIZE + 1) as i32,
    scene.points.colors_with_opacity.concat().as_slice());
For consistency, I'd start the arguments on the next line here too. ```rust bind_new_buffer_to_attribute( &ctx, point_color_attr, (COLOR_SIZE + 1) as i32, scene.points.colors_with_opacity.concat().as_slice()); ```
// draw the scene
ctx.draw_arrays(WebGl2RenderingContext::POINTS, 0, point_positions.ncols() as i32);
ctx.draw_arrays(WebGl2RenderingContext::POINTS, 0,
point_positions.ncols() as i32);
// disable the point program's vertex attributes
ctx.disable_vertex_attrib_array(point_position_attr);
@ -915,7 +957,9 @@ pub fn Display() -> View {
.into_iter()
.filter(|elt| !elt.ghost().get());
for elt in tangible_elts {
match assembly_to_world.with(|asm_to_world| elt.cast(dir, asm_to_world, pixel_size)) {
let target = assembly_to_world.with(
|asm_to_world| elt.cast(dir, asm_to_world, pixel_size));
match target {
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-12 19:06:58 +00:00
Outdated
Copy link

❌️ I'd call this value something like hit_depth rather than target, because it doesn't tell you what you hit: it only tells you whether you hit elt, and how far out along the click ray you hit it if you did.

❌️ I'd call this value something like `hit_depth` rather than `target`, because it doesn't tell you what you hit: it only tells you whether you hit `elt`, and how far out along the click ray you hit it if you did.
glen commented 2025-11-21 21:13:44 +00:00
Outdated
Copy link

Renamed to just hit, as the depth is immediately extracted on the next line, and then the None branch makes more sense: no hit at all, not just a missing depth of a hit.

Renamed to just `hit`, as the depth is immediately extracted on the next line, and then the `None` branch makes more sense: no hit at all, not just a missing depth of a hit.
Vectornaut commented 2025-11-22 07:25:37 +00:00
Outdated
Copy link

In ray-casting, I'd typically use hit to denote the full coordinates of the hit point. If that changes your mind about the clarity of hit here, we can workshop a different name. If you still think hit is clear enough here, I'm willing to accept it.

In ray-casting, I'd typically use `hit` to denote the full coordinates of the hit point. If that changes your mind about the clarity of `hit` here, we can workshop a different name. If you still think `hit` is clear enough here, I'm willing to accept it.
glen commented 2025-11-22 15:24:01 +00:00
Outdated
Copy link

check_hit?

check_hit?
Vectornaut commented 2025-11-23 06:35:21 +00:00
Outdated
Copy link

How about depth_if_hit? To me, this communicates that the value will be Some(depth) if we hit elt and None if we missed.

How about `depth_if_hit`? To me, this communicates that the value will be `Some(depth)` if we hit `elt` and `None` if we missed.
glen commented 2025-11-25 00:32:51 +00:00
Outdated
Copy link

since the method ultimately being called is named cast, I just called the result cast.

since the method ultimately being called is named `cast`, I just called the result `cast`.
Some(depth) => match clicked {
Some((_, best_depth)) => {
if depth < best_depth {

21
app-proto/src/components/outline.rs
View file

@ -63,8 +63,10 @@ fn RegulatorInput(regulator: Rc<dyn Regulator>) -> View {
placeholder = measurement.with(|result| result.to_string()),
bind:value = value,
on:change = move |_| {
let specification =
SpecifiedValue::try_from(value.get_clone_untracked());
Vectornaut commented 2025-11-22 08:07:40 +00:00
Outdated
Copy link

A SpecifiedValue consists of both a specification (spec) and a value (value), so calling the whole thing specification is confusing to me. I might use specified_value or spec_val.

A `SpecifiedValue` consists of both a specification (`spec`) and a value (`value`), so calling the whole thing `specification` is confusing to me. I might use `specified_value` or `spec_val`.
glen commented 2025-11-25 00:17:14 +00:00
Copy link

Since this is a single use temporary introduced just for the sake of an abbreviation, and its single use is on the very next line, I just called it val.

Since this is a single use temporary introduced just for the sake of an abbreviation, and its single use is on the very next line, I just called it `val`.
Vectornaut commented 2025-11-26 23:33:47 +00:00
Outdated
Copy link

To me, turning a variable called value into a variable called spec_val or specified_value seems more understandable than turning a variable called value into a variable called val. In the latter case, the variable names don't make it clear what the point of the processing was. Looking at the line where val is used, one might ask: why didn't we just use value here?

To me, turning a variable called `value` into a variable called `spec_val` or `specified_value` seems more understandable than turning a variable called `value` into a variable called `val`. In the latter case, the variable names don't make it clear what the point of the processing was. Looking at the line where `val` is used, one might ask: why didn't we just use `value` here?
glen commented 2025-11-26 23:48:30 +00:00
Outdated
Copy link

It's just an abbreviation. It doesn't need much in the way of semantics. temp? x? s? sv? If it wasn't the case that the variable is introduced solely for the sake of linebreaking and is used only once on the very next line, it might be worth this length of naming conversation. This is a throwaway.

It's just an abbreviation. It doesn't need much in the way of semantics. `temp`? `x`? `s`? `sv`? If it wasn't the case that the variable is introduced solely for the sake of linebreaking and is used only once on the very next line, it might be worth this length of naming conversation. This is a throwaway.
Vectornaut commented 2025-11-26 23:56:50 +00:00
Outdated
Copy link

It's just an abbreviation.

Oh, I see: you're now trying to find a name short enough to jam that expression into one line. Personally, I think the extra clarity is worth an extra line. However, among the names you proposed just now, sv seems the least confusing to me.

> It's just an abbreviation. Oh, I see: you're now trying to find a name short enough to jam that expression into one line. Personally, I think the extra clarity is worth an extra line. However, among the names you proposed just now, `sv` seems the least confusing to me.
valid.set(
match SpecifiedValue::try_from(value.get_clone_untracked()) {
match specification {
Ok(set_pt) => {
set_point.set(set_pt);
true
@ -141,7 +143,9 @@ fn ElementOutlineItem(element: Rc<dyn Element>) -> View {
let class = {
let element_for_class = element.clone();
state.selection.map(
move |sel| if sel.contains(&element_for_class) { "selected" } else { "" }
move |sel|
if sel.contains(&element_for_class) { "selected" }
else { "" }
)
};
let label = element.label().clone();
@ -175,7 +179,8 @@ fn ElementOutlineItem(element: Rc<dyn Element>) -> View {
move |event: KeyboardEvent| {
match event.key().as_str() {
"Enter" => {
state.select(&element_for_handler, event.shift_key());
state.select(&element_for_handler,
event.shift_key());
event.prevent_default();
},
"ArrowRight" if regulated.get() => {
@ -205,18 +210,22 @@ fn ElementOutlineItem(element: Rc<dyn Element>) -> View {
let state_for_handler = state.clone();
let element_for_handler = element.clone();
move |event: MouseEvent| {
state_for_handler.select(&element_for_handler, event.shift_key());
state_for_handler.select(&element_for_handler,
event.shift_key());
event.stop_propagation();
event.prevent_default();
}
}
) {
div(class = "element-label") { (label) }
div(class = "element-representation") { (rep_components) }
div(class = "element-representation") {
(rep_components)
}
input(
r#type = "checkbox",
bind:checked = element.ghost(),
on:click = |event: MouseEvent| event.stop_propagation()
on:click =
|event: MouseEvent| event.stop_propagation()
)
}
}

14
app-proto/src/components/spheres.frag
View file

@ -175,8 +175,9 @@ void main() {
if (debug_mode) {
// at the bottom of the screen, show the color scale instead of the
// layer count
if (gl_FragCoord.y < 10.) layer_cnt = int(16. * gl_FragCoord.x / resolution.x);
if (gl_FragCoord.y < 10.) {
layer_cnt = int(16. * gl_FragCoord.x / resolution.x);
}
// convert number to color
ivec3 bits = layer_cnt / ivec3(1, 2, 4);
vec3 color = mod(vec3(bits), 2.);
@ -217,14 +218,17 @@ void main() {
// highlight intersections
float ixn_dist = intersection_dist(frag, frag_next);
float max_highlight = max(highlight, highlight_next);
float ixn_highlight = 0.5 * max_highlight * (1. - smoothstep(2./3.*ixn_threshold, 1.5*ixn_threshold, ixn_dist));
float ixn_highlight = 0.5 * max_highlight * (1. - smoothstep(
2./3.*ixn_threshold, 1.5*ixn_threshold, ixn_dist));
frag.color = mix(frag.color, vec4(1.), ixn_highlight);
frag_next.color = mix(frag_next.color, vec4(1.), ixn_highlight);
// highlight cusps
float cusp_cos = abs(dot(dir, frag.normal));
float cusp_threshold = 2.*sqrt(ixn_threshold * sphere_list[hit.id].lt.s);
float cusp_highlight = highlight * (1. - smoothstep(2./3.*cusp_threshold, 1.5*cusp_threshold, cusp_cos));
float cusp_threshold = 2.*sqrt(
ixn_threshold * sphere_list[hit.id].lt.s);
float cusp_highlight = highlight * (1. - smoothstep(
2./3.*cusp_threshold, 1.5*cusp_threshold, cusp_cos));
frag.color = mix(frag.color, vec4(1.), cusp_highlight);
// composite the current fragment

129
app-proto/src/components/test_assembly_chooser.rs
View file

@ -167,29 +167,36 @@ fn load_low_curvature(assembly: &Assembly) {
let curvature = plane.regulators().with_untracked(
|regs| regs.first().unwrap().clone()
);
curvature.set_point().set(SpecifiedValue::try_from("0".to_string()).unwrap());
curvature.set_point().set(
SpecifiedValue::try_from("0".to_string()).unwrap());
}
let all_perpendicular = [central.clone()].into_iter()
.chain(sides.clone())
.chain(corners.clone());
for sphere in all_perpendicular {
// make each side and packed sphere perpendicular to the assembly plane
let right_angle = InversiveDistanceRegulator::new([sphere, assemb_plane.clone()]);
right_angle.set_point.set(SpecifiedValue::try_from("0".to_string()).unwrap());
let right_angle = InversiveDistanceRegulator::new(
[sphere, assemb_plane.clone()]);
right_angle.set_point.set(
SpecifiedValue::try_from("0".to_string()).unwrap());
assembly.insert_regulator(Rc::new(right_angle));
}
for sphere in sides.clone().chain(corners.clone()) {
// make each side and corner sphere tangent to the central sphere
let tangency = InversiveDistanceRegulator::new([sphere.clone(), central.clone()]);
tangency.set_point.set(SpecifiedValue::try_from("-1".to_string()).unwrap());
let tangency = InversiveDistanceRegulator::new(
[sphere.clone(), central.clone()]);
tangency.set_point.set(
SpecifiedValue::try_from("-1".to_string()).unwrap());
assembly.insert_regulator(Rc::new(tangency));
}
for (side_index, side) in sides.enumerate() {
// make each side tangent to the two adjacent corner spheres
for (corner_index, corner) in corners.clone().enumerate() {
if side_index != corner_index {
let tangency = InversiveDistanceRegulator::new([side.clone(), corner]);
tangency.set_point.set(SpecifiedValue::try_from("-1".to_string()).unwrap());
let tangency = InversiveDistanceRegulator::new(
[side.clone(), corner]);
tangency.set_point.set(
SpecifiedValue::try_from("-1".to_string()).unwrap());
assembly.insert_regulator(Rc::new(tangency));
}
}
@ -217,12 +224,15 @@ fn load_pointed(assembly: &Assembly) {
for index_y in 0..=1 {
let x = index_x as f64 - 0.5;
let y = index_y as f64 - 0.5;
let x32 = x as f32;
let y32 = y as f32;
let coords =
[0.5*(1.0 + x32), 0.5*(1.0 + y32), 0.5*(1.0 - x32*y32)];
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-12 20:50:32 +00:00
Outdated
Copy link

❌️ This is the sphere's color, not its representation coordinates. I'd call it color, matching the name of the Sphere::new argument that it gets plugged into later.

❌️ This is the sphere's color, not its representation coordinates. I'd call it `color`, matching the name of the `Sphere::new` argument that it gets plugged into later.
glen commented 2025-11-21 21:14:04 +00:00
Outdated
Copy link

Renamed to color.

Renamed to color.
👍 1
let _ = assembly.try_insert_element(
Sphere::new(
format!("sphere{index_x}{index_y}"),
format!("Sphere {index_x}{index_y}"),
[0.5*(1.0 + x) as f32, 0.5*(1.0 + y) as f32, 0.5*(1.0 - x*y) as f32],
coords,
engine::sphere(x, y, 0.0, 1.0),
)
);
@ -231,7 +241,7 @@ fn load_pointed(assembly: &Assembly) {
Point::new(
format!("point{index_x}{index_y}"),
format!("Point {index_x}{index_y}"),
[0.5*(1.0 + x) as f32, 0.5*(1.0 + y) as f32, 0.5*(1.0 - x*y) as f32],
coords,
engine::point(x, y, 0.0),
)
);
@ -320,19 +330,25 @@ fn load_tridiminished_icosahedron(assembly: &Assembly) {
"face1".to_string(),
"Face 1".to_string(),
COLOR_FACE,
engine::sphere_with_offset(frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, 0.0),
engine::sphere_with_offset(
frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6,
-frac_1_sqrt_6, 0.0),
),
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:16:46 +00:00
Outdated
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Sphere::new(
"face2".to_string(),
"Face 2".to_string(),
COLOR_FACE,
engine::sphere_with_offset(-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, 0.0),
engine::sphere_with_offset(
-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6,
-frac_1_sqrt_6, 0.0),
),
Sphere::new(
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:16:50 +00:00
Outdated
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
"face3".to_string(),
"Face 3".to_string(),
COLOR_FACE,
engine::sphere_with_offset(-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, 0.0),
engine::sphere_with_offset(
-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6,
-frac_1_sqrt_6, 0.0),
),
];
for face in faces {
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:16:55 +00:00
Outdated
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
@ -357,8 +373,10 @@ fn load_tridiminished_icosahedron(assembly: &Assembly) {
let vertex_a = assembly.elements_by_id.with_untracked(
|elts_by_id| elts_by_id[&format!("a{j}")].clone()
);
let incidence_a = InversiveDistanceRegulator::new([face.clone(), vertex_a.clone()]);
incidence_a.set_point.set(SpecifiedValue::try_from("0".to_string()).unwrap());
let incidence_a = InversiveDistanceRegulator::new(
[face.clone(), vertex_a.clone()]);
incidence_a.set_point.set(
SpecifiedValue::try_from("0".to_string()).unwrap());
assembly.insert_regulator(Rc::new(incidence_a));
// regulate the B-C vertex distances
@ -380,13 +398,16 @@ fn load_tridiminished_icosahedron(assembly: &Assembly) {
let vertex = assembly.elements_by_id.with_untracked(
|elts_by_id| elts_by_id[&format!("{series}{k}")].clone()
);
let incidence = InversiveDistanceRegulator::new([face.clone(), vertex.clone()]);
incidence.set_point.set(SpecifiedValue::try_from("0".to_string()).unwrap());
let incidence = InversiveDistanceRegulator::new(
[face.clone(), vertex.clone()]);
incidence.set_point.set(
SpecifiedValue::try_from("0".to_string()).unwrap());
assembly.insert_regulator(Rc::new(incidence));
// regulate the A-B and A-C vertex distances
assembly.insert_regulator(
Rc::new(InversiveDistanceRegulator::new([vertex_a.clone(), vertex]))
Rc::new(InversiveDistanceRegulator::new(
[vertex_a.clone(), vertex]))
);
}
}
@ -434,7 +455,8 @@ fn load_dodecahedral_packing(assembly: &Assembly) {
const COLOR_A: ElementColor = [1.00_f32, 0.25_f32, 0.00_f32];
const COLOR_B: ElementColor = [1.00_f32, 0.00_f32, 0.25_f32];
const COLOR_C: ElementColor = [0.25_f32, 0.00_f32, 1.00_f32];
let phi = 0.5 + 1.25_f64.sqrt(); /* TO DO */ // replace with std::f64::consts::PHI when that gets stabilized
/* TO DO */ // replace with std::f64::consts::PHI when that gets stabilized
let phi = 0.5 + 1.25_f64.sqrt();
let phi_inv = 1.0 / phi;
let coord_scale = (phi + 2.0).sqrt();
let face_scales = [phi_inv, (13.0 / 12.0) / coord_scale];
@ -501,13 +523,16 @@ fn load_dodecahedral_packing(assembly: &Assembly) {
// make each face sphere perpendicular to the substrate
for face in faces {
let right_angle = InversiveDistanceRegulator::new([face, substrate.clone()]);
right_angle.set_point.set(SpecifiedValue::try_from("0".to_string()).unwrap());
let right_angle = InversiveDistanceRegulator::new(
[face, substrate.clone()]);
right_angle.set_point.set(
SpecifiedValue::try_from("0".to_string()).unwrap());
assembly.insert_regulator(Rc::new(right_angle));
}
// set up the tangencies that define the packing
for [long_edge_plane, short_edge_plane] in [["a", "b"], ["b", "c"], ["c", "a"]] {
for [long_edge_plane, short_edge_plane]
in [["a", "b"], ["b", "c"], ["c", "a"]] {
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-13 05:34:47 +00:00
Outdated
Copy link

I strongly prefer to put the opening { at the same indentation level as the for, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Either of the following would look fine to me.

for [long_edge_plane, short_edge_plane]
    in [["a", "b"], ["b", "c"], ["c", "a"]]
{
    /* loop contents */
}

for [long_edge_plane, short_edge_plane] in [
    ["a", "b"], ["b", "c"], ["c", "a"]
] {
    /* loop contents */
}
I strongly prefer to put the opening `{` at the same indentation level as the `for`, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Either of the following would look fine to me. ```rust for [long_edge_plane, short_edge_plane] in [["a", "b"], ["b", "c"], ["c", "a"]] { /* loop contents */ } ``` ```rust for [long_edge_plane, short_edge_plane] in [ ["a", "b"], ["b", "c"], ["c", "a"] ] { /* loop contents */ } ```
glen commented 2025-11-21 21:15:02 +00:00
Outdated
Copy link

Tried to recast all of these accordingly. This is something we will have to give some thought to in Husht, how to delimit a multiline loop control portion from the loop body, if there are no braces.

Tried to recast all of these accordingly. This is something we will have to give some thought to in Husht, how to delimit a multiline loop control portion from the loop body, if there are no braces.
Vectornaut commented 2025-11-22 07:32:32 +00:00
Outdated
Copy link

The Python style guide has some recommendations for how to deal with this in if statements.

The [Python style guide](https://peps.python.org/pep-0008/#indentation) has some recommendations for how to deal with this in `if` statements.
glen commented 2025-11-22 16:08:12 +00:00
Outdated
Copy link

Right well lookjng there you find three options. One is do nothing, which necessitates a delimiter between condition and consequent, which doesn't seem feasible for husht (get rid of braces just to introduce a new delimiter?), another is introduce a comment, but i think we both dislike semantically significant comments ;-), and the third is the extra-indent-in-condition convention i tried but you didn't like. Hence how to do this in Husht will require real thought.

Right well lookjng there you find three options. One is do nothing, which necessitates a delimiter between condition and consequent, which doesn't seem feasible for husht (get rid of braces just to introduce a new delimiter?), another is introduce a comment, but i think we both dislike semantically significant comments ;-), and the third is the extra-indent-in-condition convention i tried but you didn't like. Hence how to do this in Husht will require real thought.
glen commented 2025-11-22 17:04:39 +00:00
Outdated
Copy link

I checked the civet documentation and based on that here are two options, not mutually exclusive, nor would have to go the same way for both conditionals and loops (civet uses the first for loops and the second for conditionals):

  1. Enclose a long condition/control in parentheses:
if (this_thing
     && that_thing)
    do_stuff()
always_do_this()

for (item
    in long_list)
    operate_on(item)
after_loop()
  1. Introduce an optional separator keyword which is always allowed but is necessary either if whole statement is on one line or if condition/control is on multiple lines:
if this_thing
    && that_thing
then do_stuff()

for item
    in long_list
do opererate_on(item)

Seems like some combination of these schemes would work for husht.

I checked the civet documentation and based on that here are two options, not mutually exclusive, nor would have to go the same way for both conditionals and loops (civet uses the first for loops and the second for conditionals): 1. Enclose a long condition/control in parentheses: ``` if (this_thing && that_thing) do_stuff() always_do_this() ``` ``` for (item in long_list) operate_on(item) after_loop() ``` 2. Introduce an optional separator keyword which is always allowed but is necessary either if whole statement is on one line or if condition/control is on multiple lines: ``` if this_thing && that_thing then do_stuff() ``` ``` for item in long_list do opererate_on(item) ``` Seems like some combination of these schemes would work for husht.
for k in 0..2 {
let long_edge_ids = [
format!("{long_edge_plane}{k}0"),
@ -526,9 +551,11 @@ fn load_dodecahedral_packing(assembly: &Assembly) {
);
// set up the short-edge tangency
let short_tangency = InversiveDistanceRegulator::new(short_edge.clone());
let short_tangency = InversiveDistanceRegulator::new(
short_edge.clone());
if k == 0 {
short_tangency.set_point.set(SpecifiedValue::try_from("-1".to_string()).unwrap());
short_tangency.set_point.set(
SpecifiedValue::try_from("-1".to_string()).unwrap());
}
assembly.insert_regulator(Rc::new(short_tangency));
@ -539,7 +566,9 @@ fn load_dodecahedral_packing(assembly: &Assembly) {
[long_edge[i].clone(), short_edge[j].clone()]
);
if i == 0 && k == 0 {
side_tangency.set_point.set(SpecifiedValue::try_from("-1".to_string()).unwrap());
side_tangency.set_point.set(
SpecifiedValue::try_from("-1".to_string()).unwrap()
);
}
assembly.insert_regulator(Rc::new(side_tangency));
}
@ -604,7 +633,8 @@ fn load_balanced(assembly: &Assembly) {
// initial configuration deliberately violates these constraints
for inner in [a, b] {
let tangency = InversiveDistanceRegulator::new([outer.clone(), inner]);
tangency.set_point.set(SpecifiedValue::try_from("1".to_string()).unwrap());
tangency.set_point.set(
SpecifiedValue::try_from("1".to_string()).unwrap());
assembly.insert_regulator(Rc::new(tangency));
}
}
@ -712,10 +742,14 @@ fn load_radius_ratio(assembly: &Assembly) {
[0.25_f32, 0.00_f32, 1.00_f32],
].into_iter(),
[
engine::sphere_with_offset(base_dir[0], base_dir[1], base_dir[2], offset, 0.0),
engine::sphere_with_offset(base_dir[0], -base_dir[1], -base_dir[2], offset, 0.0),
engine::sphere_with_offset(-base_dir[0], base_dir[1], -base_dir[2], offset, 0.0),
engine::sphere_with_offset(-base_dir[0], -base_dir[1], base_dir[2], offset, 0.0),
engine::sphere_with_offset(
base_dir[0], base_dir[1], base_dir[2], offset, 0.0),
engine::sphere_with_offset(
base_dir[0], -base_dir[1], -base_dir[2], offset, 0.0),
engine::sphere_with_offset(
-base_dir[0], base_dir[1], -base_dir[2], offset, 0.0),
engine::sphere_with_offset(
-base_dir[0], -base_dir[1], base_dir[2], offset, 0.0),
].into_iter()
).map(
|(k, color, representation)| {
@ -765,8 +799,10 @@ fn load_radius_ratio(assembly: &Assembly) {
}
// put the vertices on the faces
let incidence_regulator = InversiveDistanceRegulator::new([face_j.clone(), vertex_k.clone()]);
incidence_regulator.set_point.set(SpecifiedValue::try_from("0".to_string()).unwrap());
let incidence_regulator = InversiveDistanceRegulator::new(
[face_j.clone(), vertex_k.clone()]);
incidence_regulator.set_point.set(
SpecifiedValue::try_from("0".to_string()).unwrap());
assembly.insert_regulator(Rc::new(incidence_regulator));
}
}
@ -860,25 +896,33 @@ fn load_irisawa_hexlet(assembly: &Assembly) {
|elts_by_id| elts_by_id[&format!("chain{k}")].clone()
)
);
for (chain_sphere, chain_sphere_next) in chain.clone().zip(chain.cycle().skip(1)) {
for (chain_sphere, chain_sphere_next)
in chain.clone().zip(chain.cycle().skip(1)) {
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-13 05:36:18 +00:00
Outdated
Copy link

I strongly prefer to put the opening { at the same indentation level as the for, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Here's an example.

for (chain_sphere, chain_sphere_next)
    in chain.clone().zip(chain.cycle().skip(1))
{
    /* loop contents */
}
I strongly prefer to put the opening `{` at the same indentation level as the `for`, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Here's an example. ```rust for (chain_sphere, chain_sphere_next) in chain.clone().zip(chain.cycle().skip(1)) { /* loop contents */ } ```
glen commented 2025-11-21 21:15:33 +00:00
Outdated
Copy link

This is a ditto. There were several others as well.

This is a ditto. There were several others as well.
Vectornaut commented 2025-11-22 08:39:39 +00:00
Outdated
Copy link

There were several others as well.

Yes, looks like you got them all!

> There were several others as well. Yes, looks like you got them all!
for (other_sphere, inversive_distance) in [
(outer.clone(), "1"),
(sun.clone(), "-1"),
(moon.clone(), "-1"),
(chain_sphere_next.clone(), "-1"),
] {
let tangency = InversiveDistanceRegulator::new([chain_sphere.clone(), other_sphere]);
tangency.set_point.set(SpecifiedValue::try_from(inversive_distance.to_string()).unwrap());
let tangency = InversiveDistanceRegulator::new(
[chain_sphere.clone(), other_sphere]);
tangency.set_point.set(
SpecifiedValue::try_from(
inversive_distance.to_string()).unwrap());
assembly.insert_regulator(Rc::new(tangency));
}
}
let outer_sun_tangency = InversiveDistanceRegulator::new([outer.clone(), sun]);
outer_sun_tangency.set_point.set(SpecifiedValue::try_from("1".to_string()).unwrap());
let outer_sun_tangency = InversiveDistanceRegulator::new(
[outer.clone(), sun]);
outer_sun_tangency.set_point.set(
SpecifiedValue::try_from("1".to_string()).unwrap());
assembly.insert_regulator(Rc::new(outer_sun_tangency));
let outer_moon_tangency = InversiveDistanceRegulator::new([outer.clone(), moon]);
outer_moon_tangency.set_point.set(SpecifiedValue::try_from("1".to_string()).unwrap());
let outer_moon_tangency = InversiveDistanceRegulator::new(
[outer.clone(), moon]);
outer_moon_tangency.set_point.set(
SpecifiedValue::try_from("1".to_string()).unwrap());
assembly.insert_regulator(Rc::new(outer_moon_tangency));
}
@ -912,7 +956,8 @@ pub fn TestAssemblyChooser() -> View {
"general" => load_general(assembly),
"low-curvature" => load_low_curvature(assembly),
"pointed" => load_pointed(assembly),
"tridiminished-icosahedron" => load_tridiminished_icosahedron(assembly),
"tridiminished-icosahedron" =>
load_tridiminished_icosahedron(assembly),
"dodecahedral-packing" => load_dodecahedral_packing(assembly),
"balanced" => load_balanced(assembly),
"off-center" => load_off_center(assembly),
@ -929,7 +974,9 @@ pub fn TestAssemblyChooser() -> View {
option(value = "general") { "General" }
option(value = "low-curvature") { "Low-curvature" }
option(value = "pointed") { "Pointed" }
option(value = "tridiminished-icosahedron") { "Tridiminished icosahedron" }
option(value = "tridiminished-icosahedron") {
"Tridiminished icosahedron"
}
option(value = "dodecahedral-packing") { "Dodecahedral packing" }
option(value = "balanced") { "Balanced" }
option(value = "off-center") { "Off-center" }

111
app-proto/src/engine.rs
View file

@ -9,8 +9,11 @@ pub fn point(x: f64, y: f64, z: f64) -> DVector<f64> {
}
// the sphere with the given center and radius, with inward-pointing normals
pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVector<f64> {
let center_norm_sq = center_x * center_x + center_y * center_y + center_z * center_z;
pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)
-> DVector<f64>
{
let center_norm_sq =
Vectornaut marked this conversation as resolved
Vectornaut commented 2025-11-22 08:21:36 +00:00
Copy link

I'd match the convention used for sphere_with_offset:

pub fn sphere(
    center_x: f64, center_y: f64, center_z: f64, radius: f64
) -> DVector<f64> {
I'd match the convention used for `sphere_with_offset`: ```rust pub fn sphere( center_x: f64, center_y: f64, center_z: f64, radius: f64 ) -> DVector<f64> { ```
glen commented 2025-11-22 15:56:49 +00:00
Copy link

Here I very much don't see a need for a strict uniform "convention" but rather most readable layout on a case-by-case basis, you said you like function-call open and close on same line when possible, I really universally dislike paren on next line, and of all the places to break functionName(parameter1: Type1, parameter2: Type2) -> ReturnType { just before the arrow seems by far the cleanest to me: before an operator, clean semantic break between the calling convention on the one hand and the return type on the other, arrow makes it nice and clear this is a function. So I would advocate that breakpoint as the very first to use when needed, and only break the parameter list when it simply won't fit on one line.

Thoughts?

Here I very much don't see a need for a strict uniform "convention" but rather most readable layout on a case-by-case basis, you said you like function-call open and close on same line when possible, I really universally dislike paren on next line, and of all the places to break `functionName(parameter1: Type1, parameter2: Type2) -> ReturnType {` just before the arrow seems by far the cleanest to me: before an operator, clean semantic break between the calling convention on the one hand and the return type on the other, arrow makes it nice and clear this is a function. So I would advocate that breakpoint as the very first to use when needed, and only break the parameter list when it simply won't fit on one line. Thoughts?
Vectornaut commented 2025-11-23 07:53:46 +00:00
Copy link

The main thing I find confusing about the current formatting is that the -> DVector<f64> is part of the function declaration, but no indentation is used to show this. If you find bracket on next line more palatable than parenthesis on next line, I'd find something like this more readable than the current formatting:

pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)
    -> DVector<f64>
{
The main thing I find confusing about the current formatting is that the `-> DVector<f64>` is part of the function declaration, but no indentation is used to show this. If you find bracket on next line more palatable than parenthesis on next line, I'd find something like this more readable than the current formatting: ```rust pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVector<f64> { ```
glen commented 2025-11-25 00:27:44 +00:00
Copy link

I'll admit I am looking for a two-line solution here. I don't see any reason to indent the -> just as there is no reason to
indent the else of an if -- to me, the -> reads as a companion keyword to fn. So for the upcoming revision, I will try moving just the parenthesis to the next line to see if you like that better. As little as I like a linebreak before a paren, I like wasting lines less ;-)

I'll admit I am looking for a two-line solution here. I don't see any reason to indent the `->` just as there is no reason to indent the `else` of an `if` -- to me, the `->` reads as a companion keyword to `fn`. So for the upcoming revision, I will try moving just the parenthesis to the next line to see if you like that better. As little as I like a linebreak before a paren, I like wasting lines less ;-)
Vectornaut commented 2025-11-26 23:46:18 +00:00
Copy link

I'll admit I am looking for a two-line solution here.

All of the reasonable two-line solutions I can think of seem equally hard to read to me, and equally in violation of the Rust and Python style guides. Here's one last suggestion, which is inspired by (but in violation of) the Python style guide. You're welcome to choose between this formatting and the current one.

pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)
        -> DVector<f64> {
    /* function body */
> I'll admit I am looking for a two-line solution here. All of the reasonable two-line solutions I can think of seem equally hard to read to me, and equally in violation of the [Rust](https://doc.rust-lang.org/beta/style-guide/index.html) and [Python](https://peps.python.org/pep-0008/#indentation) style guides. Here's one last suggestion, which is inspired by (but in violation of) the Python style guide. You're welcome to choose between this formatting and the current one. ```rust pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVector<f64> { /* function body */ ```
glen commented 2025-11-26 23:59:10 +00:00
Copy link

Well, from my point of view we discarded double-indent patterns for if and for so I am loath to revive them here, so I think we are settling on

pub fn foo(long: stuff
) -> ReturnType {
    // body
}

Please confirm, thanks.

Well, from my point of view we discarded double-indent patterns for `if` and `for` so I am loath to revive them here, so I think we are settling on ``` pub fn foo(long: stuff ) -> ReturnType { // body } ``` Please confirm, thanks.
Vectornaut commented 2025-11-27 00:09:59 +00:00
Copy link

Please confirm, thanks.

Confirmed.

> Please confirm, thanks. Confirmed.
center_x * center_x + center_y * center_y + center_z * center_z;
DVector::from_column_slice(&[
center_x / radius,
center_y / radius,
@ -23,7 +26,9 @@ pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVect
// the sphere of curvature `curv` whose closest point to the origin has position
// `off * dir` and normal `dir`, where `dir` is a unit vector. setting the
// curvature to zero gives a plane
pub fn sphere_with_offset(dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64) -> DVector<f64> {
pub fn sphere_with_offset(
dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64) -> DVector<f64>
{
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-12 20:54:02 +00:00
Outdated
Copy link

Personally, I prefer the following organization, which takes up the same number of lines.

pub fn sphere_with_offset(
    dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64
) -> DVector<f64> {

I prefer it despite its potential inconsistency with the first of the following options for splitting function calls, which does save a line and which is used elsewhere in this pull request.

function_name(
    arg1, arg2, arg3, arg4);

function_name(
    arg1, arg2, arg3, arg4
);

If you strongly prefer consistency, though, I'll defer to that.

This comment also applies to several other function declarations, which I can highlight if you accept the change.

Personally, I prefer the following organization, which takes up the same number of lines. ```rust pub fn sphere_with_offset( dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64 ) -> DVector<f64> { ``` I prefer it despite its potential inconsistency with the first of the following options for splitting function calls, which does save a line and which is used elsewhere in this pull request. ```rust function_name( arg1, arg2, arg3, arg4); ``` ```rust function_name( arg1, arg2, arg3, arg4 ); ``` If you strongly prefer consistency, though, I'll defer to that. This comment also applies to several other function declarations, which I can highlight if you accept the change.
glen commented 2025-11-21 21:16:21 +00:00
Outdated
Copy link

Tried to go through and reformat accordingly. This may also be something that requires thought in Husht, along with multi-line "if" conditions.

Tried to go through and reformat accordingly. This may also be something that requires thought in Husht, along with multi-line "if" conditions.
Vectornaut commented 2025-11-22 08:42:15 +00:00
Outdated
Copy link

Tried to go through and reformat accordingly.

Thanks! I've flagged a few missed occurrences in the next review.

> Tried to go through and reformat accordingly. Thanks! I've flagged a few missed occurrences in the next review.
let norm_sp = 1.0 + off * curv;
DVector::from_column_slice(&[
norm_sp * dir_x,
@ -200,7 +205,9 @@ impl ConfigSubspace {
// with the given column index has velocity `v`. the velocity is given in
// projection coordinates, and the projection is done with respect to the
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:23:39 +00:00
Outdated
Copy link

I'd match the convention used for sphere_with_offset. It adds one line, but for me the gain in readability and familiarity are worth it.

pub fn proj(
    &self, v: &DVectorView<f64>, column_index: usize
) -> DMatrix<f64> {
I'd match the convention used for `sphere_with_offset`. It adds one line, but for me the gain in readability and familiarity are worth it. ```rust pub fn proj( &self, v: &DVectorView<f64>, column_index: usize ) -> DMatrix<f64> { ```
// projection inner product
pub fn proj(&self, v: &DVectorView<f64>, column_index: usize) -> DMatrix<f64> {
pub fn proj(&self, v: &DVectorView<f64>, column_index: usize)
-> DMatrix<f64>
{
if self.dim() == 0 {
const ELEMENT_DIM: usize = 5;
DMatrix::zeros(ELEMENT_DIM, self.assembly_dim)
@ -291,7 +298,9 @@ impl SearchState {
}
}
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:25:25 +00:00
Outdated
Copy link

I'd stick to the convention used for sphere_with_offset:

fn basis_matrix(
    index: (usize, usize), nrows: usize, ncols: usize
) -> DMatrix<f64> {
I'd stick to the convention used for `sphere_with_offset`: ```rust fn basis_matrix( index: (usize, usize), nrows: usize, ncols: usize ) -> DMatrix<f64> { ```
fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize) -> DMatrix<f64> {
fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize)
-> DMatrix<f64>
{
let mut result = DMatrix::<f64>::zeros(nrows, ncols);
result[index] = 1.0;
result
@ -414,7 +423,8 @@ pub fn realize_gram(
for _ in 0..max_descent_steps {
// find the negative gradient of the loss function
let neg_grad = 4.0 * &*Q * &state.config * &state.err_proj;
let mut neg_grad_stacked = neg_grad.clone().reshape_generic(Dyn(total_dim), Const::<1>);
let mut neg_grad_stacked =
neg_grad.clone().reshape_generic(Dyn(total_dim), Const::<1>);
history.neg_grad.push(neg_grad.clone());
// find the negative Hessian of the loss function
@ -431,7 +441,8 @@ pub fn realize_gram(
-&basis_mat * &state.err_proj
+ &state.config * &neg_d_err_proj
);
hess_cols.push(deriv_grad.reshape_generic(Dyn(total_dim), Const::<1>));
hess_cols.push(
deriv_grad.reshape_generic(Dyn(total_dim), Const::<1>));
}
}
hess = DMatrix::from_columns(hess_cols.as_slice());
@ -440,7 +451,8 @@ pub fn realize_gram(
let hess_eigvals = hess.symmetric_eigenvalues();
let min_eigval = hess_eigvals.min();
if min_eigval <= 0.0 {
hess -= reg_scale * min_eigval * DMatrix::identity(total_dim, total_dim);
hess -= reg_scale * min_eigval
* DMatrix::identity(total_dim, total_dim);
}
history.hess_eigvals.push(hess_eigvals);
@ -477,7 +489,8 @@ pub fn realize_gram(
},
};
let base_step_stacked = hess_cholesky.solve(&neg_grad_stacked);
let base_step = base_step_stacked.reshape_generic(Dyn(element_dim), Dyn(assembly_dim));
let base_step = base_step_stacked.reshape_generic(
Dyn(element_dim), Dyn(assembly_dim));
history.base_step.push(base_step.clone());
// use backtracking line search to find a better configuration
@ -507,9 +520,12 @@ pub fn realize_gram(
}
// find the kernel of the Hessian. give it the uniform inner product
let tangent = ConfigSubspace::symmetric_kernel(hess, unif_to_std, assembly_dim);
let tangent =
ConfigSubspace::symmetric_kernel(hess, unif_to_std, assembly_dim);
Ok(ConfigNeighborhood { #[cfg(feature = "dev")] config: state.config, nbhd: tangent })
Ok(ConfigNeighborhood {
#[cfg(feature = "dev")] config: state.config, nbhd: tangent
})
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-13 05:39:12 +00:00
Outdated
Copy link

If we're going to split this over multiple lines, I think it's worth an extra line to make the structure more readable.

Ok(ConfigNeighborhood {
    #[cfg(feature = "dev")] config: state.config,
    nbhd: tangent,
})
If we're going to split this over multiple lines, I think it's worth an extra line to make the structure more readable. ```rust Ok(ConfigNeighborhood { #[cfg(feature = "dev")] config: state.config, nbhd: tangent, }) ```
glen commented 2025-11-21 21:16:39 +00:00
Outdated
Copy link

Reformatted as suggested

Reformatted as suggested
👍 1
} else {
Err("Failed to reach target accuracy".to_string())
};
@ -608,7 +624,8 @@ pub mod examples {
for j in 0..2 {
// diagonal and hinge edges
for k in j..2 {
problem.gram.push_sym(block + j, block + k, if j == k { 0.0 } else { -0.5 });
problem.gram.push_sym(
Vectornaut marked this conversation as resolved
Vectornaut commented 2025-11-22 08:30:07 +00:00
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
block + j, block + k, if j == k { 0.0 } else { -0.5 });
}
// non-hinge edges
@ -702,7 +719,8 @@ mod tests {
]);
for j in 0..2 {
for k in j..2 {
problem.gram.push_sym(j, k, if (j, k) == (1, 1) { 1.0 } else { 0.0 });
problem.gram.push_sym(
j, k, if (j, k) == (1, 1) { 1.0 } else { 0.0 });
}
Vectornaut marked this conversation as resolved
Vectornaut commented 2025-11-22 08:30:23 +00:00
Copy link

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
}
problem.frozen.push(3, 0, problem.guess[(3, 0)]);
@ -729,7 +747,8 @@ mod tests {
// check against Irisawa's solution
let entry_tol = SCALED_TOL.sqrt();
let solution_diams = [30.0, 10.0, 6.0, 5.0, 15.0, 10.0, 3.75, 2.5, 2.0 + 8.0/11.0];
let solution_diams =
[30.0, 10.0, 6.0, 5.0, 15.0, 10.0, 3.75, 2.5, 2.0 + 8.0/11.0];
for (k, diam) in solution_diams.into_iter().enumerate() {
assert!((config[(3, k)] - 1.0 / diam).abs() < entry_tol);
}
@ -794,22 +813,29 @@ mod tests {
// confirm that the tangent space contains all the motions we expect it
// to. since we've already bounded the dimension of the tangent space,
// this confirms that the tangent space is what we expect it to be
let tol_sq = ((element_dim * assembly_dim) as f64) * SCALED_TOL * SCALED_TOL;
for (motion_unif, motion_std) in tangent_motions_unif.into_iter().zip(tangent_motions_std) {
let motion_proj: DMatrix<_> = motion_unif.column_iter().enumerate().map(
let tol_sq = ((element_dim * assembly_dim) as f64)
* SCALED_TOL * SCALED_TOL;
for (motion_unif, motion_std)
in tangent_motions_unif.into_iter().zip(tangent_motions_std) {
let motion_proj: DMatrix<_> =
motion_unif.column_iter().enumerate().map(
|(k, v)| tangent.proj(&v, k)
).sum();
assert!((motion_std - motion_proj).norm_squared() < tol_sq);
}
}
fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize) -> Vec<DVector<f64>> {
fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize)
-> Vec<DVector<f64>>
{
let mut elt_motion = DVector::zeros(4);
elt_motion.fixed_rows_mut::<3>(0).copy_from(vel);
iter::repeat(elt_motion).take(assembly_dim).collect()
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-22 08:33:22 +00:00
Outdated
Copy link

I'd stick to the convention used for sphere_with_offset. It adds one line, but for me the gain in readability and familiarity are worth it.

fn translation_motion_unif(
    vel: &Vector3<f64>, assembly_dim: usize
) -> Vec<DVector<f64>> {
I'd stick to the convention used for `sphere_with_offset`. It adds one line, but for me the gain in readability and familiarity are worth it. ```rust fn translation_motion_unif( vel: &Vector3<f64>, assembly_dim: usize ) -> Vec<DVector<f64>> { ```
}
fn rotation_motion_unif(ang_vel: &Vector3<f64>, points: Vec<DVectorView<f64>>) -> Vec<DVector<f64>> {
fn rotation_motion_unif(
ang_vel: &Vector3<f64>, points: Vec<DVectorView<f64>>
) -> Vec<DVector<f64>> {
points.into_iter().map(
|pt| {
let vel = ang_vel.cross(&pt.fixed_rows::<3>(0));
@ -840,9 +866,12 @@ mod tests {
translation_motion_unif(&Vector3::new(0.0, 0.0, 1.0), assembly_dim),
// the rotations about the coordinate axes
rotation_motion_unif(&Vector3::new(1.0, 0.0, 0.0), config.column_iter().collect()),
rotation_motion_unif(&Vector3::new(0.0, 1.0, 0.0), config.column_iter().collect()),
rotation_motion_unif(&Vector3::new(0.0, 0.0, 1.0), config.column_iter().collect()),
rotation_motion_unif(
&Vector3::new(1.0, 0.0, 0.0), config.column_iter().collect()),
rotation_motion_unif(
&Vector3::new(0.0, 1.0, 0.0), config.column_iter().collect()),
rotation_motion_unif(
&Vector3::new(0.0, 0.0, 1.0), config.column_iter().collect()),
// the twist motion. more precisely: a motion that keeps the center
// of mass stationary and preserves the distances between the
@ -859,8 +888,10 @@ mod tests {
[
DVector::from_column_slice(&[0.0, 0.0, 5.0, 0.0]),
DVector::from_column_slice(&[0.0, 0.0, 1.0, 0.0]),
DVector::from_column_slice(&[-vel_vert_x, -vel_vert_y, -3.0, 0.0]),
DVector::from_column_slice(&[vel_vert_x, vel_vert_y, -3.0, 0.0]),
DVector::from_column_slice(
&[-vel_vert_x, -vel_vert_y, -3.0, 0.0]),
DVector::from_column_slice(
&[vel_vert_x, vel_vert_y, -3.0, 0.0]),
]
}
).collect::<Vec<_>>(),
@ -880,9 +911,12 @@ mod tests {
// confirm that the tangent space contains all the motions we expect it
// to. since we've already bounded the dimension of the tangent space,
// this confirms that the tangent space is what we expect it to be
let tol_sq = ((element_dim * assembly_dim) as f64) * SCALED_TOL * SCALED_TOL;
for (motion_unif, motion_std) in tangent_motions_unif.into_iter().zip(tangent_motions_std) {
let motion_proj: DMatrix<_> = motion_unif.into_iter().enumerate().map(
let tol_sq = ((element_dim * assembly_dim) as f64)
* SCALED_TOL * SCALED_TOL;
for (motion_unif, motion_std)
in tangent_motions_unif.into_iter().zip(tangent_motions_std) {
let motion_proj: DMatrix<_> =
motion_unif.into_iter().enumerate().map(
|(k, v)| tangent.proj(&v.as_view(), k)
).sum();
assert!((motion_std - motion_proj).norm_squared() < tol_sq);
@ -913,10 +947,10 @@ mod tests {
problem_orig.gram.push_sym(0, 0, 1.0);
problem_orig.gram.push_sym(1, 1, 1.0);
problem_orig.gram.push_sym(0, 1, 0.5);
let Realization { result: result_orig, history: history_orig } = realize_gram(
&problem_orig, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110
);
let ConfigNeighborhood { config: config_orig, nbhd: tangent_orig } = result_orig.unwrap();
let Realization { result: result_orig, history: history_orig } =
realize_gram(&problem_orig, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110);
let ConfigNeighborhood { config: config_orig, nbhd: tangent_orig } =
result_orig.unwrap();
assert_eq!(config_orig, problem_orig.guess);
assert_eq!(history_orig.scaled_loss.len(), 1);
@ -934,10 +968,10 @@ mod tests {
frozen: problem_orig.frozen,
guess: guess_tfm,
};
let Realization { result: result_tfm, history: history_tfm } = realize_gram(
&problem_tfm, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110
);
let ConfigNeighborhood { config: config_tfm, nbhd: tangent_tfm } = result_tfm.unwrap();
let Realization { result: result_tfm, history: history_tfm } =
realize_gram(&problem_tfm, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110);
let ConfigNeighborhood { config: config_tfm, nbhd: tangent_tfm } =
result_tfm.unwrap();
Vectornaut marked this conversation as resolved Outdated
Vectornaut commented 2025-11-13 05:42:49 +00:00
Outdated
Copy link

Another option, which I find more readable:

let ConfigNeighborhood {
    config: config_tfm,
    nbhd: tangent_tfm,
} = result_tfm.unwrap();
Another option, which I find more readable: ```rust let ConfigNeighborhood { config: config_tfm, nbhd: tangent_tfm, } = result_tfm.unwrap(); ```
glen commented 2025-11-21 21:18:28 +00:00
Outdated
Copy link

Wasn't quite clear about what the layout/readability issue was here. I definitely prefer keeping the entity being assigned to (it's unfortunate it's just so verbose here) as compact as possible. I tried a different tweak.

Wasn't quite clear about what the layout/readability issue was here. I definitely prefer keeping the entity being assigned to (it's unfortunate it's just so verbose here) as compact as possible. I tried a different tweak.
Vectornaut commented 2025-11-22 08:45:06 +00:00
Outdated
Copy link

Your alternate tweak looks fine to me.

Your alternate tweak looks fine to me.
assert_eq!(config_tfm, problem_tfm.guess);
assert_eq!(history_tfm.scaled_loss.len(), 1);
@ -948,7 +982,8 @@ mod tests {
// project the equivalent nudge to the tangent space of the solution
// variety at the transformed solution
let motion_tfm = DVector::from_column_slice(&[FRAC_1_SQRT_2, 0.0, FRAC_1_SQRT_2, 0.0]);
let motion_tfm = DVector::from_column_slice(
&[FRAC_1_SQRT_2, 0.0, FRAC_1_SQRT_2, 0.0]);
let motion_tfm_proj = tangent_tfm.proj(&motion_tfm.as_view(), 0);
// take the transformation that sends the original solution to the
@ -969,7 +1004,9 @@ mod tests {
// the comparison tolerance because the transformation seems to
// introduce some numerical error
const SCALED_TOL_TFM: f64 = 1.0e-9;
let tol_sq = ((problem_orig.guess.nrows() * problem_orig.guess.ncols()) as f64) * SCALED_TOL_TFM * SCALED_TOL_TFM;
let tol_sq = ((problem_orig.guess.nrows()
* problem_orig.guess.ncols()) as f64)
* SCALED_TOL_TFM * SCALED_TOL_TFM;
assert!((motion_proj_tfm - motion_tfm_proj).norm_squared() < tol_sq);
}
}