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

Add trailing commas and clean up formatting #108

Merged
glen merged 5 commits from Vectornaut/dyna3:trailing-commas into main 2025-08-04 23:34:35 +00:00
Conversation 3 Commits 5 Files changed 12 +310 -297
12 changed files with 310 additions and 297 deletions
Download patch file Download diff file Expand all files Collapse all files

2
app-proto/examples/kaleidocycle.rs
View file

@ -23,7 +23,7 @@ fn main() {
let twist_motion: DMatrix<_> = (0..N_POINTS).step_by(4).flat_map(
|n| [
tangent.proj(&up.as_view(), n),
tangent.proj(&down.as_view(), n+1)
tangent.proj(&down.as_view(), n+1),
]
).sum();
let normalization = 5.0 / twist_motion[(2, 0)];

2
app-proto/examples/point-on-sphere.rs
View file

@ -6,7 +6,7 @@ use dyna3::engine::{
realize_gram,
sphere,
ConfigNeighborhood,
ConstraintProblem
ConstraintProblem,
};
fn main() {

4
app-proto/examples/three-spheres.rs
View file

@ -5,7 +5,7 @@ use dyna3::engine::{
realize_gram,
sphere,
ConfigNeighborhood,
ConstraintProblem
ConstraintProblem,
};
fn main() {
@ -14,7 +14,7 @@ fn main() {
&[
sphere(1.0, 0.0, 0.0, 1.0),
sphere(-0.5, a, 0.0, 1.0),
sphere(-0.5, -a, 0.0, 1.0)
sphere(-0.5, -a, 0.0, 1.0),
]
});
for j in 0..3 {

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

@ -1,13 +1,13 @@
use nalgebra::{DMatrix, DVector, DVectorView};
use std::{
cell::Cell,
collections::{BTreeMap, BTreeSet},
cmp::Ordering,
collections::{BTreeMap, BTreeSet},
fmt,
fmt::{Debug, Formatter},
hash::{Hash, Hasher},
rc::Rc,
sync::{atomic, atomic::AtomicU64}
sync::{atomic, atomic::AtomicU64},
};
use sycamore::prelude::*;
use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */
@ -26,9 +26,9 @@ use crate::{
ConfigSubspace,
ConstraintProblem,
DescentHistory,
Realization
Realization,
},
specified::SpecifiedValue
specified::SpecifiedValue,
};
pub type ElementColor = [f32; 3];
@ -164,7 +164,7 @@ pub struct Sphere {
pub ghost: Signal<bool>,
pub regulators: Signal<BTreeSet<Rc<dyn Regulator>>>,
serial: u64,
column_index: Cell<Option<usize>>
column_index: Cell<Option<usize>>,
}
impl Sphere {
@ -174,17 +174,17 @@ impl Sphere {
id: String,
label: String,
color: ElementColor,
representation: DVector<f64>
representation: DVector<f64>,
) -> Sphere {
Sphere {
id: id,
label: label,
color: color,
id,
label,
color,
representation: create_signal(representation),
ghost: create_signal(false),
regulators: create_signal(BTreeSet::new()),
serial: Self::next_serial(),
column_index: None.into()
column_index: None.into(),
}
}
}
@ -199,7 +199,7 @@ impl Element for Sphere {
id,
format!("Sphere {id_num}"),
[0.75_f32, 0.75_f32, 0.75_f32],
sphere(0.0, 0.0, 0.0, 1.0)
sphere(0.0, 0.0, 0.0, 1.0),
)
}
@ -264,7 +264,7 @@ pub struct Point {
pub ghost: Signal<bool>,
pub regulators: Signal<BTreeSet<Rc<dyn Regulator>>>,
serial: u64,
column_index: Cell<Option<usize>>
column_index: Cell<Option<usize>>,
}
impl Point {
@ -274,7 +274,7 @@ impl Point {
id: String,
label: String,
color: ElementColor,
representation: DVector<f64>
representation: DVector<f64>,
) -> Point {
Point {
id,
@ -284,7 +284,7 @@ impl Point {
ghost: create_signal(false),
regulators: create_signal(BTreeSet::new()),
serial: Self::next_serial(),
column_index: None.into()
column_index: None.into(),
}
}
}
@ -299,7 +299,7 @@ impl Element for Point {
id,
format!("Point {id_num}"),
[0.75_f32, 0.75_f32, 0.75_f32],
point(0.0, 0.0, 0.0)
point(0.0, 0.0, 0.0),
)
}
@ -389,7 +389,7 @@ pub struct InversiveDistanceRegulator {
pub subjects: [Rc<dyn Element>; 2],
pub measurement: ReadSignal<f64>,
pub set_point: Signal<SpecifiedValue>,
serial: u64
serial: u64,
}
impl InversiveDistanceRegulator {
@ -449,7 +449,7 @@ pub struct HalfCurvatureRegulator {
pub subject: Rc<dyn Element>,
pub measurement: ReadSignal<f64>,
pub set_point: Signal<SpecifiedValue>,
serial: u64
serial: u64,
}
impl HalfCurvatureRegulator {
@ -501,7 +501,7 @@ impl ProblemPoser for HalfCurvatureRegulator {
// the velocity is expressed in uniform coordinates
pub struct ElementMotion<'a> {
pub element: Rc<dyn Element>,
pub velocity: DVectorView<'a, f64>
pub velocity: DVectorView<'a, f64>,
}
type AssemblyMotion<'a> = Vec<ElementMotion<'a>>;
@ -533,7 +533,7 @@ pub struct Assembly {
// realization diagnostics
pub realization_status: Signal<Result<(), String>>,
pub descent_history: Signal<DescentHistory>
pub descent_history: Signal<DescentHistory>,
}
impl Assembly {
@ -546,7 +546,7 @@ impl Assembly {
elements_by_id: create_signal(BTreeMap::default()),
realization_trigger: create_signal(()),
realization_status: create_signal(Ok(())),
descent_history: create_signal(DescentHistory::new())
descent_history: create_signal(DescentHistory::new()),
};
// realize the assembly whenever the element list, the regulator list,
@ -724,7 +724,7 @@ impl Assembly {
// `Err(message)` we received from the match: we're changing the
// `Ok` type from `Realization` to `()`
self.realization_status.set(Err(message))
}
},
}
}
@ -807,7 +807,7 @@ impl Assembly {
},
None => {
console_log!("No velocity to unpack for fresh element \"{}\"", elt.id())
}
},
};
});
}
@ -867,7 +867,7 @@ mod tests {
String::from(sphere_id),
String::from("Sphere 0"),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(0.0, 0.0, 0.0, INITIAL_RADIUS)
engine::sphere(0.0, 0.0, 0.0, INITIAL_RADIUS),
)
);
@ -881,7 +881,7 @@ mod tests {
vec![
ElementMotion {
element: sphere.clone(),
velocity: velocity.as_view()
velocity: velocity.as_view(),
}
]
);

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

@ -4,7 +4,7 @@ use sycamore::prelude::*;
use super::test_assembly_chooser::TestAssemblyChooser;
use crate::{
AppState,
assembly::{InversiveDistanceRegulator, Point, Sphere}
assembly::{InversiveDistanceRegulator, Point, Sphere},
};
#[component]

16
app-proto/src/components/diagnostics.rs
View file

@ -11,14 +11,12 @@ use crate::AppState;
#[derive(Clone)]
struct DiagnosticsState {
active_tab: Signal<String>
active_tab: Signal<String>,
}
impl DiagnosticsState {
fn new(initial_tab: String) -> DiagnosticsState {
DiagnosticsState {
active_tab: create_signal(initial_tab)
}
DiagnosticsState { active_tab: create_signal(initial_tab) }
glen marked this conversation as resolved Outdated
glen commented 2025-08-01 21:45:51 +00:00
Outdated
Copy link

As a mostly uninformed question about Rust syntax, why is/must DiagnosticsState mentioned four times in quick succession here? The first is to declare a new data type, so is of course unavoidable. I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common? Is the third a type annotation and the fourth a constructor? If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of new? (If I recall, new has no special status in Rust, so we can't hope Rust will just know its return type should be the struct being impled. But the type is patent from the only exit from the function body...)

It would be great to reduce this redundancy; of course we may ultimately just have to introduce some mechanisms for it in Husht, if there's little to be done within Rust...

As a mostly uninformed question about Rust syntax, why is/must DiagnosticsState mentioned _four_ times in quick succession here? The first is to declare a new data type, so is of course unavoidable. I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common? Is the third a type annotation and the fourth a constructor? If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of `new`? (If I recall, `new` has no special status in Rust, so we can't hope Rust will just know its return type should be the struct being `impl`ed. But the type is patent from the only exit from the function body...) It would be great to reduce this redundancy; of course we may ultimately just have to introduce some mechanisms for it in Husht, if there's little to be done within Rust...
Vectornaut commented 2025-08-02 09:23:08 +00:00
Outdated
Copy link

I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common?

I don't know of any more compact syntax. If there were one, it would probably be mentioned here. I think the current syntax is quite usable and readable. I also think there's an advantage to having only one syntax for an implementation block: you can find all of the implementation blocks for a non-parametric structure Sandwich by skimming or searching for blocks of the forms shown here:

impl Sandwich
impl Sandwich</* concrete type */>
impl</* generic type: perhaps with bounds */> Sandwich</* generic type */>

Is the third a type annotation and the fourth a constructor?

Yes.

If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of new?

I don't think so. The thing that comes before the braces in a struct expression is something called a path in expression, which looks like it's supposed to be a specific type specifier like Sandwich or snack::Sandwich or super::snack::Sandwich or Sandwich<Cucumber>.

The code below shows that Rust can infer type parameters in a struct expression, but I don't think there's syntax to request inference for the whole type.

struct NamedValue<T> {
    name: String,
    value: T,
}

fn the_answer() -> NamedValue<i32> {
    NamedValue {
        name: "The answer".to_string(),
        value: 42
    }
}

fn main() {
    let NamedValue { name, value } = the_answer();
    print!("{name} is {value}");
}

I guess it could be reasonable to use _ as a placeholder for inferred types:

let _ { name, value } = the_answer();

After all, it can already be used as a placeholder for inferred type parameters in expressions like this:

let list: Vec<_> = [4, 6, 8, 12, 20]
    .into_iter()
    .map(|n| format!("{n}-hedron"))
    .collect();

(In this example, the compiler can't infer what type of collection we want, because collect<B> is a generic function that can return collections of various types. Once we tell the compiler that we want a Vec, though, we can ask it to infer the type parameter that specifies the element type.)

> I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common? I don't know of any more compact syntax. If there were one, it would probably be mentioned [here](https://doc.rust-lang.org/stable/book/ch05-03-method-syntax.html). I think the current syntax is quite usable and readable. I also think there's an advantage to having only one syntax for an implementation block: you can find all of the implementation blocks for a non-parametric structure `Sandwich` by skimming or searching for blocks of the forms shown [here](https://doc.rust-lang.org/rust-by-example/generics/impl.html): ```rust impl Sandwich impl Sandwich</* concrete type */> impl</* generic type: perhaps with bounds */> Sandwich</* generic type */> ``` > Is the third a type annotation and the fourth a constructor? Yes. > If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of new? I don't think so. The thing that comes before the braces in a [struct expression](https://doc.rust-lang.org/reference/expressions/struct-expr.html) is something called a [*path in expression*](https://doc.rust-lang.org/reference/paths.html#paths-in-expressions), which looks like it's supposed to be a specific type specifier like `Sandwich` or `snack::Sandwich` or `super::snack::Sandwich` or `Sandwich<Cucumber>`. The code below shows that Rust can infer type parameters in a struct expression, but I don't think there's syntax to request inference for the whole type. ```rust struct NamedValue<T> { name: String, value: T, } fn the_answer() -> NamedValue<i32> { NamedValue { name: "The answer".to_string(), value: 42 } } fn main() { let NamedValue { name, value } = the_answer(); print!("{name} is {value}"); } ``` I guess it could be reasonable to use `_` as a placeholder for inferred types: ```rust let _ { name, value } = the_answer(); ``` After all, it can already be used as a placeholder for inferred type parameters in expressions like this: ```rust let list: Vec<_> = [4, 6, 8, 12, 20] .into_iter() .map(|n| format!("{n}-hedron")) .collect(); ``` (In this example, the compiler can't infer what type of collection we want, because [`collect<B>`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.collect) is a generic function that can return collections of various types. Once we tell the compiler that we want a `Vec`, though, we can ask it to infer the type parameter that specifies the element type.)
glen commented 2025-08-02 16:39:48 +00:00
Outdated
Copy link

I guess it could be reasonable to use _ as a placeholder for inferred types:

let _ { name, value } = the_answer();

This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say?

On the page you link, it does mention that in an impl block, Self is an abbreviation for the type being impled. So that suggests we could write

struct DiagnosticsState {
    active_tab: Signal<String>,
}

impl DiagnosticsState {
    fn new(initial_tab: String) -> Self {
        Self { active_tab: create_signal(initial_tab) }
    }
}

What do you think? I'm just struggling with the virtue of design decisions on the part of Rust that would lead to the type name being written out four time in such a short bit of code. I don't see anything misleading or ambiguous about syntax like:

class DiagnosticsState  // transform to a struct declaration
   active_tab: Signal<String>

   fn new(initial_tab: String)  // fns in a class automatically get put in a impl block; return type inferred
      DiagnosticsState { active_tab: create_signal(initial_tab) } // or Self { ... }

as a possible Husht rendering of this. Anyhow, seems like the only action item to consider here is whether to use Self as an abbreviation, presuming it works fine. Seems pretty sensible for a "new" function, at least.

> I guess it could be reasonable to use _ as a placeholder for inferred types: > `let _ { name, value } = the_answer();` This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say? On the page you link, it does mention that in an `impl` block, `Self` is an abbreviation for the type being impled. So that suggests we could write ``` struct DiagnosticsState { active_tab: Signal<String>, } impl DiagnosticsState { fn new(initial_tab: String) -> Self { Self { active_tab: create_signal(initial_tab) } } } ``` What do you think? I'm just struggling with the virtue of design decisions on the part of Rust that would lead to the type name being written out four time in such a short bit of code. I don't see anything misleading or ambiguous about syntax like: ``` class DiagnosticsState // transform to a struct declaration active_tab: Signal<String> fn new(initial_tab: String) // fns in a class automatically get put in a impl block; return type inferred DiagnosticsState { active_tab: create_signal(initial_tab) } // or Self { ... } ``` as a possible Husht rendering of this. Anyhow, seems like the only action item to consider here is whether to use Self as an abbreviation, presuming it works fine. Seems pretty sensible for a "new" function, at least.
Vectornaut commented 2025-08-04 03:04:28 +00:00
Outdated
Copy link

On the page you link, it does mention that in an impl block, Self is an abbreviation for the type being impled. So that suggests we could write […]. What do you think?

Oh, yes, we could do that. I mostly see Self used in implementations of traits, like Clone and From, but nothing would stop us from using it in the basic implementation of a structure. This style seems to be used to some extent in the source code for the Rust standard library and for nalgebra. For example:

  • It's used for the return type, but not the returned structure, in Vec::new.
  • It's used for the return type, but not the returned structure, in ColPivQR::new from nalgebra.
  • It's not used at all in BTreeSet::new_in—which is newly added code, and therefore might reflect current conventions.

The main downside I see to this style is that it can make the source harder to navigate, because it's context-dependent.

If we use this style, I think we should use it consistently for every structure. Let me know if you'd like me to proceed with reformatting all our structure implementations in this way.

I guess it could be reasonable to use _ as a placeholder for inferred types:

This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say?

Correct.

> On the page you link, it does mention that in an impl block, Self is an abbreviation for the type being impled. So that suggests we could write […]. What do you think? Oh, yes, we could do that. I mostly see `Self` used in implementations of traits, like `Clone` and `From`, but nothing would stop us from using it in the basic implementation of a structure. This style seems to be used to some extent in the source code for the Rust standard library and for nalgebra. For example: - It's used for the return type, but not the returned structure, in [`Vec::new`](https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#434). - It's used for the return type, but not the returned structure, in [`ColPivQR::new`](https://docs.rs/nalgebra/latest/src/nalgebra/linalg/col_piv_qr.rs.html#59-92`) from nalgebra. - It's not used at all in [`BTreeSet::new_in`](https://doc.rust-lang.org/src/alloc/collections/btree/set.rs.html#364)—which is newly added code, and therefore might reflect current conventions. The main downside I see to this style is that it can make the source harder to navigate, because it's context-dependent. If we use this style, I think we should use it consistently for every structure. Let me know if you'd like me to proceed with reformatting all our structure implementations in this way. >> I guess it could be reasonable to use _ as a placeholder for inferred types: > > This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say? Correct.
glen commented 2025-08-04 09:03:33 +00:00
Outdated
Copy link

Well, I like the reduced rendundancy of Self, especially for long struct names. So if you're comfortable with it, yes please let's use that abbreviation, either in this PR or the next as seems better to you.

Well, I like the reduced rendundancy of Self, especially for long struct names. So if you're comfortable with it, yes please let's use that abbreviation, either in this PR or the next as seems better to you.
Vectornaut commented 2025-08-04 17:27:11 +00:00
Outdated
Copy link

Let's do it in the next PR, along with renaming the test assembly loader functions. I think those two mass name changes are more similar to each other than to the changes this PR was intended for.

Let's do it in the next PR, along with renaming the test assembly loader functions. I think those two mass name changes are more similar to each other than to the changes this PR was intended for.
glen commented 2025-08-04 21:25:09 +00:00
Outdated
Copy link

OK, again, please put it in whatever new issue you made/are making for the function name changes. Resolving, and will merge this PR as soon as I have a moment to run-test it.

OK, again, please put it in whatever new issue you made/are making for the function name changes. Resolving, and will merge this PR as soon as I have a moment to run-test it.
Vectornaut commented 2025-08-04 21:55:06 +00:00
Outdated
Copy link

This is now issue #110. (To help with organization, I opened a separate issue instead of mashing it together with #109).

This is now issue #110. (To help with organization, I opened a separate issue instead of mashing it together with #109).
}
}
glen marked this conversation as resolved
glen commented 2025-08-01 21:48:24 +00:00
Copy link

As a mild convention, should we prefer things like

DiagnosticsState {active_tab: create_signal(initial_tab)}

when they easily fit on one line? As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium...

p.s. I see now that in fact you have re-single-lined several such constructors in other files, so it seems to me this one should be as well, right?

As a mild convention, should we prefer things like ``` DiagnosticsState {active_tab: create_signal(initial_tab)} ``` when they easily fit on one line? As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium... p.s. I see now that in fact you have re-single-lined several such constructors in other files, so it seems to me this one should be as well, right?
Vectornaut commented 2025-08-02 05:42:17 +00:00
Copy link

As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium...

Oh, yeah, I remember you mentioning that. I've collapsed the structure expression onto one line in commit ebad512. I had put active_tab: /* ... */ on its own line to make the create_signal call more readable, but it's fine if that's not worth the cost in vertical space.

> As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium... Oh, yeah, I remember you mentioning that. I've collapsed the structure expression onto one line in commit ebad512. I had put `active_tab: /* ... */` on its own line to make the `create_signal` call more readable, but it's fine if that's not worth the cost in vertical space.
@ -33,7 +31,7 @@ fn RealizationStatus() -> View {
class = realization_status.with(
|status| match status {
Ok(_) => "",
Err(_) => "invalid"
Err(_) => "invalid",
}
)
) {
glen marked this conversation as resolved
glen commented 2025-08-01 21:53:00 +00:00
Copy link

Confused; I thought match branches didn't need commas... seems inconsistent with the vicinity of Err(Message) => in assembly.rs above.

If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces, then I would mildly urge keeping commas after every match branch for consistency. I don't see why match branches should have different punctuation depending on whether or not they happen to be expressions or whatever brace-enclosed things are. Perhaps we shall have to make the commas unnecessary in both contexts in Husht...

p.s. I contemplate another option below which is to always use the braces, and no commas, which might be fine as well until Husht can uniformize this...

Confused; I thought match branches didn't need commas... seems inconsistent with the vicinity of `Err(Message) =>` in assembly.rs above. If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces, then I would mildly urge keeping commas after every match branch for consistency. I don't see why match branches should have different punctuation depending on whether or not they happen to be expressions or whatever brace-enclosed things are. Perhaps we shall have to make the commas unnecessary in both contexts in Husht... p.s. I contemplate another option below which is to always use the braces, and no commas, which might be fine as well until Husht can uniformize this...
Vectornaut commented 2025-08-02 06:04:53 +00:00
Copy link

If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces […]

Yes, that's the point.

[…] then I would mildly urge keeping commas after every match branch for consistency.

Done, in commit bfd5d8e. I've updated the pull request description to reflect this. Rustfmt can be configured to use this style by setting match_block_trailing_comma to true.

> If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces […] Yes, that's the point. > […] then I would mildly urge keeping commas after every match branch for consistency. Done, in commit bfd5d8e. I've updated the pull request description to reflect this. Rustfmt can be configured to use this style by setting [`match_block_trailing_comma`](https://rust-lang.github.io/rustfmt/?version=v1.8.0&search=#match_block_trailing_comma) to `true`.
@ -42,7 +40,7 @@ fn RealizationStatus() -> View {
(realization_status.with(
|status| match status {
Ok(_) => "Target accuracy achieved".to_string(),
Err(message) => message.clone()
Err(message) => message.clone(),
}
))
}
@ -53,7 +51,7 @@ fn RealizationStatus() -> View {
fn into_log10_time_point((step, value): (usize, f64)) -> Vec<Option<f64>> {
vec![
Some(step as f64),
if value == 0.0 { None } else { Some(value.abs().log10()) }
if value == 0.0 { None } else { Some(value.abs().log10()) },
]
}
@ -122,7 +120,7 @@ fn SpectrumHistory() -> View {
// positive, negative, and strictly-zero parts
let (
hess_eigvals_zero,
hess_eigvals_nonzero
hess_eigvals_nonzero,
): (Vec<_>, Vec<_>) = state.assembly.descent_history.with(
|history| history.hess_eigvals
.iter()
@ -143,7 +141,7 @@ fn SpectrumHistory() -> View {
.unwrap_or(1.0);
let (
hess_eigvals_pos,
hess_eigvals_neg
hess_eigvals_neg,
): (Vec<_>, Vec<_>) = hess_eigvals_nonzero
.into_iter()
.partition(|&(_, val)| val > 0.0);

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

@ -12,12 +12,12 @@ use web_sys::{
WebGlProgram,
WebGlShader,
WebGlUniformLocation,
wasm_bindgen::{JsCast, JsValue}
wasm_bindgen::{JsCast, JsValue},
};
use crate::{
AppState,
assembly::{Element, ElementColor, ElementMotion, Point, Sphere}
assembly::{Element, ElementColor, ElementMotion, Point, Sphere},
};
// --- color ---
@ -37,7 +37,7 @@ fn combine_channels(color: ElementColor, opacity: f32) -> ColorWithOpacity {
struct SceneSpheres {
representations: Vec<DVector<f64>>,
colors_with_opacity: Vec<ColorWithOpacity>,
highlights: Vec<f32>
highlights: Vec<f32>,
}
impl SceneSpheres {
@ -45,7 +45,7 @@ impl SceneSpheres {
SceneSpheres {
representations: Vec::new(),
colors_with_opacity: Vec::new(),
highlights: Vec::new()
highlights: Vec::new(),
}
}
@ -53,7 +53,10 @@ impl SceneSpheres {
self.representations.len().try_into().expect("Number of spheres must fit in a 32-bit integer")
}
fn push(&mut self, representation: DVector<f64>, color: ElementColor, opacity: f32, highlight: f32) {
fn push(
&mut self, representation: DVector<f64>,
color: ElementColor, opacity: f32, highlight: f32,
) {
self.representations.push(representation);
self.colors_with_opacity.push(combine_channels(color, opacity));
self.highlights.push(highlight);
glen marked this conversation as resolved Outdated
glen commented 2025-08-01 21:57:28 +00:00
Outdated
Copy link

These options will (a) clearly be subject to change and expansion as dyna3 matures, and (b) soon be too numerous to reiterate on every concrete function that needs such options, and so will have to be folded into some flexible options data structure that can specify none, some, or all of the options and fall back on defaults for the unspecified ones. But perhaps this PR is not the time to do it? I just mention it because with respecting the 80-char line limit (which I heartily approve), the multiple signatures with this same list of arguments is now burning a lot of vertical space, slightly non-DRYly.

p.s. but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line, i.e. I don't see a particular reason to be wedded to one parameter/argument per line just because a call is broken onto more than one line, and indeed in other places in the code we do bunch some arguments on a single line in multi-line calls.

These options will (a) clearly be subject to change and expansion as dyna3 matures, and (b) soon be too numerous to reiterate on every concrete function that needs such options, and so will have to be folded into some flexible options data structure that can specify none, some, or all of the options and fall back on defaults for the unspecified ones. But perhaps this PR is not the time to do it? I just mention it because with respecting the 80-char line limit (which I heartily approve), the multiple signatures with this same list of arguments is now burning a lot of vertical space, slightly non-DRYly. p.s. but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line, i.e. I don't see a particular reason to be wedded to one parameter/argument per line just because a call is broken onto more than one line, and indeed in other places in the code we do bunch some arguments on a single line in multi-line calls.
Vectornaut commented 2025-08-02 06:20:31 +00:00
Outdated
Copy link

Yes, I think that revising the SceneSpheres and ScenePoints structures at some point would be a good idea.

But perhaps this PR is not the time to do it?

I agree. Like I said in the description, I'd like to limit this PR to cosmetic changes which are unlikely to affect the abstract syntax tree.

Yes, I think that revising the `SceneSpheres` and `ScenePoints` structures at some point would be a good idea. > But perhaps this PR is not the time to do it? I agree. Like I said in the description, I'd like to limit this PR to cosmetic changes which are unlikely to affect the abstract syntax tree.
Vectornaut commented 2025-08-02 06:28:01 +00:00
Outdated
Copy link

but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line

Sure. Do either of the following groupings look good to you?

fn push(
    &mut self, representation: DVector<f64>,
    color: ElementColor, opacity: f32, highlight: f32, selected: bool,
)

fn push(
    &mut self,
    representation: DVector<f64>,
    color: ElementColor, opacity: f32, highlight: f32, selected: bool,
)

Note that I'm using the parameter list for the analogous ScenePoints::push function, which includes the selected parameter.

> but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line Sure. Do either of the following groupings look good to you? ```rust fn push( &mut self, representation: DVector<f64>, color: ElementColor, opacity: f32, highlight: f32, selected: bool, ) ``` ```rust fn push( &mut self, representation: DVector<f64>, color: ElementColor, opacity: f32, highlight: f32, selected: bool, ) ``` Note that I'm using the parameter list for the analogous `ScenePoints::push` function, which includes the `selected` parameter.
glen commented 2025-08-02 23:46:53 +00:00
Outdated
Copy link

Either of your two suggestions is fine; I agree on the sensibility of clustering the "appearance options" parameters/arguments, as those are the ones that will likely be collected into some sort of Style object in the future.

Either of your two suggestions is fine; I agree on the sensibility of clustering the "appearance options" parameters/arguments, as those are the ones that will likely be collected into some sort of Style object in the future.
Vectornaut commented 2025-08-04 03:11:51 +00:00
Outdated
Copy link

Done in commit e67a658.

Done in commit e67a658.
@ -64,7 +67,7 @@ struct ScenePoints {
representations: Vec<DVector<f64>>,
colors_with_opacity: Vec<ColorWithOpacity>,
highlights: Vec<f32>,
selections: Vec<f32>
selections: Vec<f32>,
}
impl ScenePoints {
@ -73,11 +76,14 @@ impl ScenePoints {
representations: Vec::new(),
colors_with_opacity: Vec::new(),
highlights: Vec::new(),
selections: Vec::new()
selections: Vec::new(),
}
}
fn push(&mut self, representation: DVector<f64>, color: ElementColor, opacity: f32, highlight: f32, selected: bool) {
fn push(
&mut self, representation: DVector<f64>,
color: ElementColor, opacity: f32, highlight: f32, selected: bool,
) {
self.representations.push(representation);
self.colors_with_opacity.push(combine_channels(color, opacity));
self.highlights.push(highlight);
@ -87,14 +93,14 @@ impl ScenePoints {
pub struct Scene {
spheres: SceneSpheres,
points: ScenePoints
points: ScenePoints,
}
impl Scene {
fn new() -> Scene {
Scene {
spheres: SceneSpheres::new(),
points: ScenePoints::new()
points: ScenePoints::new(),
}
}
}
@ -105,7 +111,12 @@ pub trait DisplayItem {
// the smallest positive depth, represented as a multiple of `dir`, where
// the line generated by `dir` hits the element. returns `None` if the line
// misses the element
fn cast(&self, dir: Vector3<f64>, assembly_to_world: &DMatrix<f64>, pixel_size: f64) -> Option<f64>;
fn cast(
&self,
dir: Vector3<f64>,
assembly_to_world: &DMatrix<f64>,
pixel_size: f64,
) -> Option<f64>;
}
impl DisplayItem for Sphere {
@ -124,7 +135,12 @@ impl DisplayItem for Sphere {
// this method should be kept synchronized with `sphere_cast` in
// `spheres.frag`, which does essentially the same thing on the GPU side
fn cast(&self, dir: Vector3<f64>, assembly_to_world: &DMatrix<f64>, _pixel_size: f64) -> Option<f64> {
fn cast(
&self,
dir: Vector3<f64>,
assembly_to_world: &DMatrix<f64>,
_pixel_size: f64,
) -> Option<f64> {
// if `a/b` is less than this threshold, we approximate
// `a*u^2 + b*u + c` by the linear function `b*u + c`
const DEG_THRESHOLD: f64 = 1e-9;
@ -177,7 +193,12 @@ impl DisplayItem for Point {
}
/* SCAFFOLDING */
fn cast(&self, dir: Vector3<f64>, assembly_to_world: &DMatrix<f64>, pixel_size: f64) -> Option<f64> {
fn cast(
&self,
dir: Vector3<f64>,
assembly_to_world: &DMatrix<f64>,
pixel_size: f64,
) -> Option<f64> {
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`
@ -220,7 +241,7 @@ fn compile_shader(
fn set_up_program(
context: &WebGl2RenderingContext,
vertex_shader_source: &str,
fragment_shader_source: &str
fragment_shader_source: &str,
) -> WebGlProgram {
// compile the shaders
let vertex_shader = compile_shader(
@ -260,12 +281,12 @@ fn get_uniform_array_locations<const N: usize>(
context: &WebGl2RenderingContext,
program: &WebGlProgram,
var_name: &str,
member_name_opt: Option<&str>
member_name_opt: Option<&str>,
) -> [Option<WebGlUniformLocation>; N] {
array::from_fn(|n| {
let name = match member_name_opt {
Some(member_name) => format!("{var_name}[{n}].{member_name}"),
None => format!("{var_name}[{n}]")
None => format!("{var_name}[{n}]"),
};
context.get_uniform_location(&program, name.as_str())
})
@ -276,7 +297,7 @@ fn bind_to_attribute(
context: &WebGl2RenderingContext,
attr_index: u32,
attr_size: i32,
buffer: &Option<WebGlBuffer>
buffer: &Option<WebGlBuffer>,
) {
context.bind_buffer(WebGl2RenderingContext::ARRAY_BUFFER, buffer.as_ref());
context.vertex_attrib_pointer_with_i32(
@ -292,7 +313,7 @@ fn bind_to_attribute(
// load the given data into a new vertex buffer object
fn load_new_buffer(
context: &WebGl2RenderingContext,
data: &[f32]
data: &[f32],
) -> Option<WebGlBuffer> {
// create a buffer and bind it to ARRAY_BUFFER
let buffer = context.create_buffer();
@ -319,7 +340,7 @@ fn bind_new_buffer_to_attribute(
context: &WebGl2RenderingContext,
attr_index: u32,
attr_size: i32,
data: &[f32]
data: &[f32],
) {
let buffer = load_new_buffer(context, data);
bind_to_attribute(context, attr_index, attr_size, &buffer);
@ -341,9 +362,9 @@ fn event_dir(event: &MouseEvent) -> (Vector3<f64>, f64) {
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,
-1.0
-1.0,
),
FOCAL_SLOPE * 2.0 / shortdim
FOCAL_SLOPE * 2.0 / shortdim,
)
}
@ -443,14 +464,14 @@ pub fn Display() -> View {
let sphere_program = set_up_program(
&ctx,
include_str!("identity.vert"),
include_str!("spheres.frag")
include_str!("spheres.frag"),
);
// set up the point rendering program
let point_program = set_up_program(
&ctx,
include_str!("point.vert"),
include_str!("point.frag")
include_str!("point.frag"),
);
/* DEBUG */
@ -467,7 +488,7 @@ pub fn Display() -> View {
// capped at 1024 elements
console::log_2(
&ctx.get_parameter(WebGl2RenderingContext::MAX_FRAGMENT_UNIFORM_VECTORS).unwrap(),
&JsValue::from("uniform vectors available")
&JsValue::from("uniform vectors available"),
);
// find the sphere program's vertex attribute
@ -503,7 +524,7 @@ pub fn Display() -> View {
// southeast triangle
-1.0, -1.0, 0.0,
1.0, 1.0, 0.0,
1.0, -1.0, 0.0
1.0, -1.0, 0.0,
];
let viewport_position_buffer = load_new_buffer(&ctx, &viewport_positions);
@ -596,7 +617,7 @@ pub fn Display() -> View {
vec![
ElementMotion {
element: sel,
velocity: elt_motion.as_view()
velocity: elt_motion.as_view(),
}
]
);
@ -629,7 +650,7 @@ pub fn Display() -> View {
0.0, 1.0, 0.0, 0.0, 0.0,
0.0, 0.0, 1.0, 0.0, u,
0.0, 0.0, 2.0*u, 1.0, u*u,
0.0, 0.0, 0.0, 0.0, 1.0
0.0, 0.0, 0.0, 0.0, 1.0,
])
};
let asm_to_world = &location * &orientation;
@ -668,19 +689,19 @@ pub fn Display() -> View {
let v = &sphere_reps_world[n];
ctx.uniform3fv_with_f32_array(
sphere_sp_locs[n].as_ref(),
v.rows(0, 3).as_slice()
v.rows(0, 3).as_slice(),
);
ctx.uniform2fv_with_f32_array(
sphere_lt_locs[n].as_ref(),
v.rows(3, 2).as_slice()
v.rows(3, 2).as_slice(),
);
ctx.uniform4fv_with_f32_array(
sphere_color_locs[n].as_ref(),
&scene.spheres.colors_with_opacity[n]
&scene.spheres.colors_with_opacity[n],
);
ctx.uniform1f(
sphere_highlight_locs[n].as_ref(),
scene.spheres.highlights[n]
scene.spheres.highlights[n],
);
}
@ -773,7 +794,7 @@ pub fn Display() -> View {
"ArrowLeft" if shift => roll_ccw.set(value),
"ArrowRight" => yaw_right.set(value),
"ArrowLeft" => yaw_left.set(value),
_ => navigating = false
_ => navigating = false,
};
if navigating {
scene_changed.set(true);
@ -793,7 +814,7 @@ pub fn Display() -> View {
"s" | "S" => translate_neg_y.set(value),
"]" | "}" => shrink_neg.set(value),
"[" | "{" => shrink_pos.set(value),
_ => manipulating = false
_ => manipulating = false,
};
if manipulating {
event.prevent_default();
@ -883,18 +904,18 @@ pub fn Display() -> View {
clicked = Some((elt, depth))
}
},
None => clicked = Some((elt, depth))
}
None => ()
None => clicked = Some((elt, depth)),
},
None => (),
};
}
// if we clicked something, select it
match clicked {
Some((elt, _)) => state.select(&elt, event.shift_key()),
None => state.selection.update(|sel| sel.clear())
None => state.selection.update(|sel| sel.clear()),
};
}
},
)
}
}

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

@ -1,11 +1,7 @@
use itertools::Itertools;
use std::rc::Rc;
use sycamore::prelude::*;
use web_sys::{
KeyboardEvent,
MouseEvent,
wasm_bindgen::JsCast
};
use web_sys::{KeyboardEvent, MouseEvent, wasm_bindgen::JsCast};
use crate::{
AppState,
@ -13,7 +9,7 @@ use crate::{
Element,
HalfCurvatureRegulator,
InversiveDistanceRegulator,
Regulator
Regulator,
},
specified::SpecifiedValue
};
@ -71,8 +67,8 @@ fn RegulatorInput(regulator: Rc<dyn Regulator>) -> View {
Ok(set_pt) => {
set_point.set(set_pt);
true
}
Err(_) => false
},
Err(_) => false,
}
)
},
@ -80,10 +76,10 @@ fn RegulatorInput(regulator: Rc<dyn Regulator>) -> View {
move |event: KeyboardEvent| {
match event.key().as_str() {
"Escape" => reset_value(),
_ => ()
}
_ => (),
}
}
},
)
}
}
@ -179,7 +175,7 @@ fn ElementOutlineItem(element: Rc<dyn Element>) -> View {
.unchecked_into::<web_sys::Element>()
.remove_attribute("open");
},
_ => ()
_ => (),
glen marked this conversation as resolved
glen commented 2025-08-01 22:15:39 +00:00
Copy link

this new comma looks particularly nutty when all of the other commas are being removed. Can we just leave it out, especially given that a null default return will presumably be forever the final branch here? Indeed, isn't the case that there could never be subsequent code here that would have any runtime effect, because the _ branch would always fire, cutting off any chance that subsequent branches would? This latter consideration obviates the need for the comma to avoid the "spurious git diff" phenomenon, since no useful code could ever be added subsequent to this branch. So please leave out this comma, thanks.

this new comma looks particularly nutty when all of the other commas are being removed. Can we just leave it out, especially given that a null default return will presumably be forever the final branch here? Indeed, isn't the case that there could never be subsequent code here that would have any runtime effect, because the `_` branch would always fire, cutting off any chance that subsequent branches would? This latter consideration obviates the need for the comma to avoid the "spurious git diff" phenomenon, since no useful code could ever be added subsequent to this branch. So please leave out this comma, thanks.
Vectornaut commented 2025-08-02 06:17:47 +00:00
Copy link

Now that we've switched to using commas after match arms wrapped in blocks, should I still remove the commas after catch-all match arms? That would make them the only match arms without commas, which makes them look weird to me.

Now that we've [switched](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3152) to using commas after match arms wrapped in blocks, should I still remove the commas after catch-all match arms? That would make them the only match arms without commas, which makes them look weird to me.
glen commented 2025-08-02 23:52:41 +00:00
Copy link

Yes, agreed, if we are always using commas, they should be here too. Resolving.

Yes, agreed, if we are always using commas, they should be here too. Resolving.
}
}
}

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

@ -6,17 +6,17 @@ use web_sys::{console, wasm_bindgen::JsValue};
use crate::{
AppState,
engine,
engine::DescentHistory,
assembly::{
Assembly,
Element,
ElementColor,
InversiveDistanceRegulator,
Point,
Sphere
Sphere,
},
specified::SpecifiedValue
engine,
engine::DescentHistory,
specified::SpecifiedValue,
};
// --- loaders ---
@ -32,7 +32,7 @@ fn load_gen_assemb(assembly: &Assembly) {
String::from("gemini_a"),
String::from("Castor"),
[1.00_f32, 0.25_f32, 0.00_f32],
engine::sphere(0.5, 0.5, 0.0, 1.0)
engine::sphere(0.5, 0.5, 0.0, 1.0),
)
);
let _ = assembly.try_insert_element(
@ -40,7 +40,7 @@ fn load_gen_assemb(assembly: &Assembly) {
String::from("gemini_b"),
String::from("Pollux"),
[0.00_f32, 0.25_f32, 1.00_f32],
engine::sphere(-0.5, -0.5, 0.0, 1.0)
engine::sphere(-0.5, -0.5, 0.0, 1.0),
)
);
let _ = assembly.try_insert_element(
@ -48,7 +48,7 @@ fn load_gen_assemb(assembly: &Assembly) {
String::from("ursa_major"),
String::from("Ursa major"),
[0.25_f32, 0.00_f32, 1.00_f32],
engine::sphere(-0.5, 0.5, 0.0, 0.75)
engine::sphere(-0.5, 0.5, 0.0, 0.75),
)
);
let _ = assembly.try_insert_element(
@ -56,7 +56,7 @@ fn load_gen_assemb(assembly: &Assembly) {
String::from("ursa_minor"),
String::from("Ursa minor"),
[0.25_f32, 1.00_f32, 0.00_f32],
engine::sphere(0.5, -0.5, 0.0, 0.5)
engine::sphere(0.5, -0.5, 0.0, 0.5),
)
);
let _ = assembly.try_insert_element(
@ -64,7 +64,7 @@ fn load_gen_assemb(assembly: &Assembly) {
String::from("moon_deimos"),
String::from("Deimos"),
[0.75_f32, 0.75_f32, 0.00_f32],
engine::sphere(0.0, 0.15, 1.0, 0.25)
engine::sphere(0.0, 0.15, 1.0, 0.25),
)
);
let _ = assembly.try_insert_element(
@ -72,7 +72,7 @@ fn load_gen_assemb(assembly: &Assembly) {
String::from("moon_phobos"),
String::from("Phobos"),
[0.00_f32, 0.75_f32, 0.50_f32],
engine::sphere(0.0, -0.15, -1.0, 0.25)
engine::sphere(0.0, -0.15, -1.0, 0.25),
)
);
}
@ -85,7 +85,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"central".to_string(),
"Central".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(0.0, 0.0, 0.0, 1.0)
engine::sphere(0.0, 0.0, 0.0, 1.0),
)
);
let _ = assembly.try_insert_element(
@ -93,7 +93,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"assemb_plane".to_string(),
"Assembly plane".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere_with_offset(0.0, 0.0, 1.0, 0.0, 0.0)
engine::sphere_with_offset(0.0, 0.0, 1.0, 0.0, 0.0),
)
);
let _ = assembly.try_insert_element(
@ -101,7 +101,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"side1".to_string(),
"Side 1".to_string(),
[1.00_f32, 0.00_f32, 0.25_f32],
engine::sphere_with_offset(1.0, 0.0, 0.0, 1.0, 0.0)
engine::sphere_with_offset(1.0, 0.0, 0.0, 1.0, 0.0),
)
);
let _ = assembly.try_insert_element(
@ -109,7 +109,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"side2".to_string(),
"Side 2".to_string(),
[0.25_f32, 1.00_f32, 0.00_f32],
engine::sphere_with_offset(-0.5, a, 0.0, 1.0, 0.0)
engine::sphere_with_offset(-0.5, a, 0.0, 1.0, 0.0),
)
);
let _ = assembly.try_insert_element(
@ -117,7 +117,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"side3".to_string(),
"Side 3".to_string(),
[0.00_f32, 0.25_f32, 1.00_f32],
engine::sphere_with_offset(-0.5, -a, 0.0, 1.0, 0.0)
engine::sphere_with_offset(-0.5, -a, 0.0, 1.0, 0.0),
)
);
let _ = assembly.try_insert_element(
@ -125,7 +125,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"corner1".to_string(),
"Corner 1".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(-4.0/3.0, 0.0, 0.0, 1.0/3.0)
engine::sphere(-4.0/3.0, 0.0, 0.0, 1.0/3.0),
)
);
let _ = assembly.try_insert_element(
@ -133,7 +133,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
"corner2".to_string(),
"Corner 2".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(2.0/3.0, -4.0/3.0 * a, 0.0, 1.0/3.0)
engine::sphere(2.0/3.0, -4.0/3.0 * a, 0.0, 1.0/3.0),
)
);
let _ = assembly.try_insert_element(
@ -141,7 +141,7 @@ fn load_low_curv_assemb(assembly: &Assembly) {
String::from("corner3"),
String::from("Corner 3"),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(2.0/3.0, 4.0/3.0 * a, 0.0, 1.0/3.0)
engine::sphere(2.0/3.0, 4.0/3.0 * a, 0.0, 1.0/3.0),
)
);
@ -202,7 +202,7 @@ fn load_pointed_assemb(assembly: &Assembly) {
format!("point_front"),
format!("Front point"),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::point(0.0, 0.0, FRAC_1_SQRT_2)
engine::point(0.0, 0.0, FRAC_1_SQRT_2),
)
);
let _ = assembly.try_insert_element(
@ -210,7 +210,7 @@ fn load_pointed_assemb(assembly: &Assembly) {
format!("point_back"),
format!("Back point"),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::point(0.0, 0.0, -FRAC_1_SQRT_2)
engine::point(0.0, 0.0, -FRAC_1_SQRT_2),
)
);
for index_x in 0..=1 {
@ -223,7 +223,7 @@ fn load_pointed_assemb(assembly: &Assembly) {
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],
engine::sphere(x, y, 0.0, 1.0)
engine::sphere(x, y, 0.0, 1.0),
)
);
@ -232,7 +232,7 @@ fn load_pointed_assemb(assembly: &Assembly) {
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],
engine::point(x, y, 0.0)
engine::point(x, y, 0.0),
)
);
}
@ -256,56 +256,56 @@ fn load_tridim_icosahedron_assemb(assembly: &Assembly) {
"a1".to_string(),
"A₁".to_string(),
COLOR_A,
engine::point(0.25, 0.75, 0.75)
engine::point(0.25, 0.75, 0.75),
),
Point::new(
"a2".to_string(),
"A₂".to_string(),
COLOR_A,
engine::point(0.75, 0.25, 0.75)
engine::point(0.75, 0.25, 0.75),
),
Point::new(
"a3".to_string(),
"A₃".to_string(),
COLOR_A,
engine::point(0.75, 0.75, 0.25)
engine::point(0.75, 0.75, 0.25),
),
Point::new(
"b1".to_string(),
"B₁".to_string(),
COLOR_B,
engine::point(0.75, -0.25, -0.25)
engine::point(0.75, -0.25, -0.25),
),
Point::new(
"b2".to_string(),
"B₂".to_string(),
COLOR_B,
engine::point(-0.25, 0.75, -0.25)
engine::point(-0.25, 0.75, -0.25),
),
Point::new(
"b3".to_string(),
"B₃".to_string(),
COLOR_B,
engine::point(-0.25, -0.25, 0.75)
engine::point(-0.25, -0.25, 0.75),
),
Point::new(
"c1".to_string(),
"C₁".to_string(),
COLOR_C,
engine::point(0.0, -1.0, -1.0)
engine::point(0.0, -1.0, -1.0),
),
Point::new(
"c2".to_string(),
"C₂".to_string(),
COLOR_C,
engine::point(-1.0, 0.0, -1.0)
engine::point(-1.0, 0.0, -1.0),
),
Point::new(
"c3".to_string(),
"C₃".to_string(),
COLOR_C,
engine::point(-1.0, -1.0, 0.0)
)
engine::point(-1.0, -1.0, 0.0),
),
];
for vertex in vertices {
let _ = assembly.try_insert_element(vertex);
@ -320,20 +320,20 @@ fn load_tridim_icosahedron_assemb(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),
),
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(
"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 {
face.ghost().set(true);
@ -416,7 +416,7 @@ fn load_dodeca_packing_assemb(assembly: &Assembly) {
"substrate".to_string(),
"Substrate".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(0.0, 0.0, 0.0, 1.0)
engine::sphere(0.0, 0.0, 0.0, 1.0),
)
);
let substrate = assembly.elements_by_id.with_untracked(
@ -456,7 +456,7 @@ fn load_dodeca_packing_assemb(assembly: &Assembly) {
id_a.clone(),
format!("A{label_sub}"),
COLOR_A,
engine::sphere(0.0, small_coord, big_coord, face_radii[k])
engine::sphere(0.0, small_coord, big_coord, face_radii[k]),
)
);
faces.push(
@ -472,7 +472,7 @@ fn load_dodeca_packing_assemb(assembly: &Assembly) {
id_b.clone(),
format!("B{label_sub}"),
COLOR_B,
engine::sphere(small_coord, big_coord, 0.0, face_radii[k])
engine::sphere(small_coord, big_coord, 0.0, face_radii[k]),
)
);
faces.push(
@ -488,7 +488,7 @@ fn load_dodeca_packing_assemb(assembly: &Assembly) {
id_c.clone(),
format!("C{label_sub}"),
COLOR_C,
engine::sphere(big_coord, 0.0, small_coord, face_radii[k])
engine::sphere(big_coord, 0.0, small_coord, face_radii[k]),
)
);
faces.push(
@ -559,19 +559,19 @@ fn load_balanced_assemb(assembly: &Assembly) {
"outer".to_string(),
"Outer".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(0.0, 0.0, 0.0, R_OUTER)
engine::sphere(0.0, 0.0, 0.0, R_OUTER),
),
Sphere::new(
"a".to_string(),
"A".to_string(),
[1.00_f32, 0.00_f32, 0.25_f32],
engine::sphere(0.0, 4.0, 0.0, R_INNER)
engine::sphere(0.0, 4.0, 0.0, R_INNER),
),
Sphere::new(
"b".to_string(),
"B".to_string(),
[0.00_f32, 0.25_f32, 1.00_f32],
engine::sphere(0.0, -4.0, 0.0, R_INNER)
engine::sphere(0.0, -4.0, 0.0, R_INNER),
),
];
for sphere in spheres {
@ -589,7 +589,7 @@ fn load_balanced_assemb(assembly: &Assembly) {
for (sphere, radius) in [
(outer.clone(), R_OUTER),
(a.clone(), R_INNER),
(b.clone(), R_INNER)
(b.clone(), R_INNER),
] {
let curvature_regulator = sphere.regulators().with_untracked(
|regs| regs.first().unwrap().clone()
@ -618,7 +618,7 @@ fn load_off_center_assemb(assembly: &Assembly) {
"point".to_string(),
"Point".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::point(1e-9, 0.0, 0.0)
engine::point(1e-9, 0.0, 0.0),
),
);
let _ = assembly.try_insert_element(
@ -626,7 +626,7 @@ fn load_off_center_assemb(assembly: &Assembly) {
"sphere".to_string(),
"Sphere".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(0.0, 0.0, 0.0, 1.0)
engine::sphere(0.0, 0.0, 0.0, 1.0),
),
);
@ -658,14 +658,14 @@ fn load_radius_ratio_assemb(assembly: &Assembly) {
"sphere_faces".to_string(),
"Insphere".to_string(),
GRAY,
engine::sphere(0.0, 0.0, 0.0, 0.5)
engine::sphere(0.0, 0.0, 0.0, 0.5),
),
Sphere::new(
"sphere_vertices".to_string(),
"Circumsphere".to_string(),
GRAY,
engine::sphere(0.0, 0.0, 0.0, 0.25)
)
engine::sphere(0.0, 0.0, 0.0, 0.25),
),
];
for sphere in spheres {
let _ = assembly.try_insert_element(sphere);
@ -678,13 +678,13 @@ fn load_radius_ratio_assemb(assembly: &Assembly) {
[1.00_f32, 0.50_f32, 0.75_f32],
[1.00_f32, 0.75_f32, 0.50_f32],
[1.00_f32, 1.00_f32, 0.50_f32],
[0.75_f32, 0.50_f32, 1.00_f32]
[0.75_f32, 0.50_f32, 1.00_f32],
].into_iter(),
[
engine::point(-0.6, -0.8, -0.6),
engine::point(-0.6, 0.8, 0.6),
engine::point(0.6, -0.8, 0.6),
engine::point(0.6, 0.8, -0.6)
engine::point(0.6, 0.8, -0.6),
].into_iter()
).map(
|(k, color, representation)| {
@ -692,7 +692,7 @@ fn load_radius_ratio_assemb(assembly: &Assembly) {
format!("v{k}"),
format!("Vertex {k}"),
color,
representation
representation,
)
}
);
@ -709,13 +709,13 @@ fn load_radius_ratio_assemb(assembly: &Assembly) {
[1.00_f32, 0.00_f32, 0.25_f32],
[1.00_f32, 0.25_f32, 0.00_f32],
[0.75_f32, 0.75_f32, 0.00_f32],
[0.25_f32, 0.00_f32, 1.00_f32]
[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),
].into_iter()
).map(
|(k, color, representation)| {
@ -723,7 +723,7 @@ fn load_radius_ratio_assemb(assembly: &Assembly) {
format!("f{k}"),
format!("Face {k}"),
color,
representation
representation,
)
}
);
@ -736,7 +736,7 @@ fn load_radius_ratio_assemb(assembly: &Assembly) {
for j in index_range.clone() {
let [face_j, vertex_j] = [
format!("f{j}"),
format!("v{j}")
format!("v{j}"),
].map(
|id| assembly.elements_by_id.with_untracked(
|elts_by_id| elts_by_id[&id].clone()
@ -797,7 +797,7 @@ fn load_irisawa_hexlet_assemb(assembly: &Assembly) {
[0.75_f32, 0.75_f32, 0.00_f32],
[0.25_f32, 1.00_f32, 0.00_f32],
[0.00_f32, 0.25_f32, 1.00_f32],
[0.25_f32, 0.00_f32, 1.00_f32]
[0.25_f32, 0.00_f32, 1.00_f32],
].into_iter();
// create the spheres
@ -806,19 +806,19 @@ fn load_irisawa_hexlet_assemb(assembly: &Assembly) {
"outer".to_string(),
"Outer".to_string(),
[0.5_f32, 0.5_f32, 0.5_f32],
engine::sphere(0.0, 0.0, 0.0, 1.5)
engine::sphere(0.0, 0.0, 0.0, 1.5),
),
Sphere::new(
"sun".to_string(),
"Sun".to_string(),
[0.75_f32, 0.75_f32, 0.75_f32],
engine::sphere(0.0, -0.75, 0.0, 0.75)
engine::sphere(0.0, -0.75, 0.0, 0.75),
),
Sphere::new(
"moon".to_string(),
"Moon".to_string(),
[0.25_f32, 0.25_f32, 0.25_f32],
engine::sphere(0.0, 0.75, 0.0, 0.75)
engine::sphere(0.0, 0.75, 0.0, 0.75),
),
].into_iter().chain(
index_range.clone().zip(colors).map(
@ -828,7 +828,7 @@ fn load_irisawa_hexlet_assemb(assembly: &Assembly) {
format!("chain{k}"),
format!("Chain {k}"),
color,
engine::sphere(1.0 * ang.sin(), 0.0, 1.0 * ang.cos(), 0.5)
engine::sphere(1.0 * ang.sin(), 0.0, 1.0 * ang.cos(), 0.5),
)
}
)
@ -865,7 +865,7 @@ fn load_irisawa_hexlet_assemb(assembly: &Assembly) {
(outer.clone(), "1"),
(sun.clone(), "-1"),
(moon.clone(), "-1"),
(chain_sphere_next.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());
@ -918,7 +918,7 @@ pub fn TestAssemblyChooser() -> View {
"off-center" => load_off_center_assemb(assembly),
glen marked this conversation as resolved
glen commented 2025-08-01 22:24:55 +00:00
Copy link

Please either:

  • Put the "ly"s into the last word of these function names -- there's no real value in using a non-word abbreviation for a plain word when it only saves two characters, or
  • elide the _assemb part, and quite possibly all the commonload_ parts, of these function names altogether. I'd find
    "off-center" => off_center(assembly), ...

no problem to read, and in any case it seems that

    "off-center" => load_off_center(assembly), ...

should certainly suffice if you find just "off_center" to be a bit too compressed.

I prefer some variation of the second bullet, as you might imagine.

Please either: * Put the "ly"s into the last word of these function names -- there's no real value in using a non-word abbreviation for a plain word when it only saves two characters, or * elide the `_assemb` part, and quite possibly all the common`load_` parts, of these function names altogether. I'd find ``` "off-center" => off_center(assembly), ... ``` no problem to read, and in any case it seems that ``` "off-center" => load_off_center(assembly), ... ``` should certainly suffice if you find just "off_center" to be a bit too compressed. I prefer some variation of the second bullet, as you might imagine.
Vectornaut commented 2025-08-02 07:22:44 +00:00
Copy link

elide the _assemb part, and quite possibly all the common load_ parts, of these function names altogether

I agree that dropping the _assemb suffix makes sense now that we've split the test assembly chooser off into its own module. I've added this to my list of simplifications that are more likely to affect the abstract syntax tree, and therefore more appropriate for a separate PR.

> elide the `_assemb` part, and quite possibly all the common `load_` parts, of these function names altogether I agree that dropping the `_assemb` suffix makes sense now that we've split the test assembly chooser off into its own module. I've added this to my list of simplifications that are more likely to affect the abstract syntax tree, and therefore more appropriate for a separate PR.
glen commented 2025-08-02 23:56:08 +00:00
Copy link

I don't personally see any problem with folding wholesale identifier substitutions (every baz becomes quux, for example) into this PR, but if you prefer to do it in a separate PR immediately following this one, that's fine too. We should just get through all the current "typographical" changes before returning to semantic ones...

I don't personally see any problem with folding wholesale identifier substitutions (every baz becomes quux, for example) into this PR, but if you prefer to do it in a separate PR immediately following this one, that's fine too. We should just get through all the current "typographical" changes before returning to semantic ones...
Vectornaut commented 2025-08-04 03:20:44 +00:00
Copy link

Okay, let's do that in the next pull request.

Okay, let's do that in the next pull request.
glen commented 2025-08-04 09:01:07 +00:00
Copy link

OK, please file an issue for it so we don't forget. Resolving this thread.

OK, please file an issue for it so we don't forget. Resolving this thread.
Vectornaut commented 2025-08-04 21:45:42 +00:00
Copy link

This is now issue #109.

This is now issue #109.
"radius-ratio" => load_radius_ratio_assemb(assembly),
"irisawa-hexlet" => load_irisawa_hexlet_assemb(assembly),
_ => ()
_ => (),
};
glen marked this conversation as resolved
glen commented 2025-08-01 22:20:52 +00:00
Copy link

this comma looks more uniform, but hopefully we can get rid of all of the commas? In fact, if commas are at the moment required in standard Rust syntax when the branch is an expression, I might even prefer (until Husht helps us curtail brace-o-rama)

   "off-center" => {load_off_center_assemb(assembly)}
   "radius-ratio" => {load_radius_ratio_assemb(assembly)}
   _ => ()

What do you think?

this comma looks more uniform, but hopefully we can get rid of all of the commas? In fact, if commas are at the moment required in standard Rust syntax when the branch is an expression, I might even prefer (until Husht helps us curtail brace-o-rama) ``` "off-center" => {load_off_center_assemb(assembly)} "radius-ratio" => {load_radius_ratio_assemb(assembly)} _ => () ``` What do you think?
Vectornaut commented 2025-08-02 07:25:45 +00:00
Copy link

What do you think?

I think that commas on every arm (as we now have) are much more readable than braces on every arm.

> What do you think? I think that commas on every arm (as we [now have](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3152)) are much more readable than braces on every arm.
});
});

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

@ -16,7 +16,7 @@ pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVect
center_y / radius,
center_z / radius,
0.5 / radius,
0.5 * (center_norm_sq / radius - radius)
0.5 * (center_norm_sq / radius - radius),
])
}
@ -30,7 +30,7 @@ pub fn sphere_with_offset(dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f6
norm_sp * dir_y,
norm_sp * dir_z,
0.5 * curv,
off * (1.0 + 0.5 * off * curv)
off * (1.0 + 0.5 * off * curv),
])
}
@ -53,7 +53,7 @@ pub fn project_point_to_normalized(rep: &mut DVector<f64>) {
pub struct MatrixEntry {
index: (usize, usize),
value: f64
value: f64,
}
pub struct PartialMatrix(Vec<MatrixEntry>);
@ -65,7 +65,7 @@ impl PartialMatrix {
pub fn push(&mut self, row: usize, col: usize, value: f64) {
let PartialMatrix(entries) = self;
entries.push(MatrixEntry { index: (row, col), value: value });
entries.push(MatrixEntry { index: (row, col), value });
}
pub fn push_sym(&mut self, row: usize, col: usize, value: f64) {
@ -135,22 +135,26 @@ impl<'a> IntoIterator for &'a PartialMatrix {
pub struct ConfigSubspace {
assembly_dim: usize,
basis_std: Vec<DMatrix<f64>>,
basis_proj: Vec<DMatrix<f64>>
basis_proj: Vec<DMatrix<f64>>,
}
impl ConfigSubspace {
pub fn zero(assembly_dim: usize) -> ConfigSubspace {
ConfigSubspace {
assembly_dim: assembly_dim,
assembly_dim,
basis_proj: Vec::new(),
basis_std: Vec::new()
basis_std: Vec::new(),
}
}
// approximate the kernel of a symmetric endomorphism of the configuration
// space for `assembly_dim` elements. we consider an eigenvector to be part
// of the kernel if its eigenvalue is smaller than the constant `THRESHOLD`
fn symmetric_kernel(a: DMatrix<f64>, proj_to_std: DMatrix<f64>, assembly_dim: usize) -> ConfigSubspace {
fn symmetric_kernel(
a: DMatrix<f64>,
proj_to_std: DMatrix<f64>,
assembly_dim: usize,
) -> ConfigSubspace {
// find a basis for the kernel. the basis is expressed in the projection
// coordinates, and it's orthonormal with respect to the projection
// inner product
@ -170,7 +174,7 @@ impl ConfigSubspace {
const ELEMENT_DIM: usize = 5;
const UNIFORM_DIM: usize = 4;
ConfigSubspace {
assembly_dim: assembly_dim,
assembly_dim,
basis_std: basis_std.column_iter().map(
|v| Into::<DMatrix<f64>>::into(
v.reshape_generic(Dyn(ELEMENT_DIM), Dyn(assembly_dim))
@ -180,7 +184,7 @@ impl ConfigSubspace {
|v| Into::<DMatrix<f64>>::into(
v.reshape_generic(Dyn(UNIFORM_DIM), Dyn(assembly_dim))
)
).collect()
).collect(),
}
}
@ -214,9 +218,9 @@ pub struct DescentHistory {
pub config: Vec<DMatrix<f64>>,
pub scaled_loss: Vec<f64>,
pub neg_grad: Vec<DMatrix<f64>>,
pub hess_eigvals: Vec::<DVector<f64>>,
pub hess_eigvals: Vec<DVector<f64>>,
pub base_step: Vec<DMatrix<f64>>,
pub backoff_steps: Vec<i32>
pub backoff_steps: Vec<i32>,
}
impl DescentHistory {
@ -246,7 +250,7 @@ impl ConstraintProblem {
ConstraintProblem {
gram: PartialMatrix::new(),
frozen: PartialMatrix::new(),
guess: DMatrix::<f64>::zeros(ELEMENT_DIM, element_count)
guess: DMatrix::<f64>::zeros(ELEMENT_DIM, element_count),
}
}
@ -255,7 +259,7 @@ impl ConstraintProblem {
ConstraintProblem {
gram: PartialMatrix::new(),
frozen: PartialMatrix::new(),
guess: DMatrix::from_columns(guess_columns)
guess: DMatrix::from_columns(guess_columns),
}
}
}
@ -269,25 +273,21 @@ lazy_static! {
0.0, 1.0, 0.0, 0.0, 0.0,
0.0, 0.0, 1.0, 0.0, 0.0,
0.0, 0.0, 0.0, 0.0, -2.0,
0.0, 0.0, 0.0, -2.0, 0.0
0.0, 0.0, 0.0, -2.0, 0.0,
]);
}
struct SearchState {
config: DMatrix<f64>,
err_proj: DMatrix<f64>,
loss: f64
loss: f64,
}
impl SearchState {
fn from_config(gram: &PartialMatrix, config: DMatrix<f64>) -> SearchState {
let err_proj = gram.sub_proj(&(config.tr_mul(&*Q) * &config));
let loss = err_proj.norm_squared();
SearchState {
config: config,
err_proj: err_proj,
loss: loss
}
SearchState { config, err_proj, loss }
}
}
@ -314,7 +314,7 @@ pub fn local_unif_to_std(v: DVectorView<f64>) -> DMatrix<f64> {
curv, 0.0, 0.0, 0.0, v[0],
0.0, curv, 0.0, 0.0, v[1],
0.0, 0.0, curv, 0.0, v[2],
0.0, 0.0, 0.0, 0.0, 1.0
0.0, 0.0, 0.0, 0.0, 1.0,
])
} else {
// `v` represents a sphere. the normalization condition says that the
@ -323,7 +323,7 @@ pub fn local_unif_to_std(v: DVectorView<f64>) -> DMatrix<f64> {
curv, 0.0, 0.0, 0.0, v[0],
0.0, curv, 0.0, 0.0, v[1],
0.0, 0.0, curv, 0.0, v[2],
curv*v[0], curv*v[1], curv*v[2], curv*v[3], curv*v[4] + 1.0
curv*v[0], curv*v[1], curv*v[2], curv*v[3], curv*v[4] + 1.0,
])
}
}
@ -336,7 +336,7 @@ fn seek_better_config(
base_target_improvement: f64,
min_efficiency: f64,
backoff: f64,
max_backoff_steps: i32
max_backoff_steps: i32,
) -> Option<(SearchState, i32)> {
let mut rate = 1.0;
for backoff_steps in 0..max_backoff_steps {
@ -354,12 +354,12 @@ fn seek_better_config(
// a first-order neighborhood of a configuration
pub struct ConfigNeighborhood {
pub config: DMatrix<f64>,
pub nbhd: ConfigSubspace
pub nbhd: ConfigSubspace,
}
pub struct Realization {
pub result: Result<ConfigNeighborhood, String>,
pub history: DescentHistory
pub history: DescentHistory,
}
// seek a matrix `config` that matches the partial matrix `problem.frozen` and
@ -373,12 +373,10 @@ pub fn realize_gram(
backoff: f64,
reg_scale: f64,
max_descent_steps: i32,
max_backoff_steps: i32
max_backoff_steps: i32,
) -> Realization {
// destructure the problem data
let ConstraintProblem {
gram, guess, frozen
} = problem;
let ConstraintProblem { gram, guess, frozen } = problem;
// start the descent history
let mut history = DescentHistory::new();
@ -391,10 +389,10 @@ pub fn realize_gram(
let result = Ok(
ConfigNeighborhood {
config: guess.clone(),
nbhd: ConfigSubspace::zero(0)
nbhd: ConfigSubspace::zero(0),
}
);
return Realization { result, history }
return Realization { result, history };
}
glen marked this conversation as resolved
glen commented 2025-08-01 22:33:17 +00:00
Copy link

Why is an explicit return needed here, rather than just a final expression? And why the semicolon now -- is it added just because of the return, or is a no-spurious-git-diff thing, too, because it would be necessary if another statement were added? But as that's impossible since code after an unqualified return can't be executed, can we leave out the semicolon even if there's a reason return is needed? Thanks for clarifying.

Why is an explicit return needed here, rather than just a final expression? And why the semicolon now -- is it added just because of the return, or is a no-spurious-git-diff thing, too, because it would be necessary if another statement were added? But as that's impossible since code after an unqualified return can't be executed, can we leave out the semicolon even if there's a reason return is needed? Thanks for clarifying.
Vectornaut commented 2025-08-02 08:14:46 +00:00
Copy link

why the semicolon now

The semicolon helps communicate the intent of the code, as explained below. I'm adding it now because I didn't notice it was missing until now, when I ran Rustfmt in check mode and saw its absence flagged as a formatting error.

Why is an explicit return needed here, rather than just a final expression?

Let's look at a simpler example.

fn factorial(n: u128) -> u128 {
    if n == 0 {
        return 1;
    }
    n * factorial(n-1)
}

The expression

if n == 0 {
    return 1;
}

has the unit value (), which is discarded. It also has the important side effect of causing the function to stop and return 1 when n == 0. The trailing semicolon emphasizes that the important thing about this expression is its side effect, not its value.

The alternate expression

if n == 0 {
    return 1
}

has the same value and side effect. However, it gets its value in a different and more confusing way: it evalutes to the value of the expression return 1, which happens to be (). When the last expression in a block has no semicolon, that expression lends its value to the block, so leaving the semicolon off the last expression suggests (misleadingly, in this case) that its value is important.

The alternate expression

if n == 0 {
    1
}

gives a type error, because an if with a non-unit value has to come with an else that evaluates to the same type.

In our simple example, we could rewrite the whole function like this, which I find easier to understand:

fn factorial(n: u128) -> u128 {
    if n == 0 {
        1
    } else {
        n * factorial(n-1)
    }
}

If we rewrote realize_gram like this, however, we'd end up with over a hundred lines of code inside the else block, which I think would make the whole function less readable.

> why the semicolon now The semicolon helps communicate the intent of the code, as explained below. I'm adding it now because I didn't notice it was missing until now, when I ran Rustfmt in check mode and saw its absence flagged as a formatting error. > Why is an explicit return needed here, rather than just a final expression? Let's look at a simpler example. ```rust fn factorial(n: u128) -> u128 { if n == 0 { return 1; } n * factorial(n-1) } ``` The expression ```rust if n == 0 { return 1; } ``` has the unit value `()`, which is discarded. It also has the important side effect of causing the function to stop and return `1` when `n == 0`. The trailing semicolon emphasizes that the important thing about this expression is its side effect, not its value. The alternate expression ```rust if n == 0 { return 1 } ``` has the same value and side effect. However, it gets its value in a different and more confusing way: it evalutes to the value of the expression `return 1`, which happens to be `()`. When the last expression in a block has no semicolon, that expression lends its value to the block, so leaving the semicolon off the last expression suggests (misleadingly, in this case) that its value is important. The alternate expression ```rust if n == 0 { 1 } ``` gives a type error, because an `if` with a non-unit value has to come with an `else` that evaluates to the same type. In our simple example, we could rewrite the whole function like this, which I find easier to understand: ```rust fn factorial(n: u128) -> u128 { if n == 0 { 1 } else { n * factorial(n-1) } } ``` If we rewrote `realize_gram` like this, however, we'd end up with over a hundred lines of code inside the `else` block, which I think would make the whole function less readable.
glen commented 2025-08-03 17:10:37 +00:00
Copy link

OK, so let me see if I understand -- there is a return here because it is not the end of a function, but just of an if-branch; and you are using a semicolon to signify that the return value of the return statement is ignored. I am not a fan of the latter, but I am not going to worry about it at all as semicolons should be handled in a mutually agreeable way in Husht. So resolving.

OK, so let me see if I understand -- there is a return here because it is not the end of a function, but just of an if-branch; and you are using a semicolon to signify that the return value of the return statement is ignored. I am not a fan of the latter, but I am not going to worry about it at all as semicolons should be handled in a mutually agreeable way in Husht. So resolving.
👍 1
// find the dimension of the search space
@ -475,8 +473,8 @@ pub fn realize_gram(
Some(cholesky) => cholesky,
None => return Realization {
result: Err("Cholesky decomposition failed".to_string()),
history
}
history,
},
};
let base_step_stacked = hess_cholesky.solve(&neg_grad_stacked);
let base_step = base_step_stacked.reshape_generic(Dyn(element_dim), Dyn(assembly_dim));
@ -485,17 +483,17 @@ pub fn realize_gram(
// use backtracking line search to find a better configuration
if let Some((better_state, backoff_steps)) = seek_better_config(
gram, &state, &base_step, neg_grad.dot(&base_step),
min_efficiency, backoff, max_backoff_steps
min_efficiency, backoff, max_backoff_steps,
glen marked this conversation as resolved
glen commented 2025-08-01 22:37:30 +00:00
Copy link

OK, so here's an interesting point: here you include an arbitrary varying number of arguments on each line of the call, grouped/formatted as you see fit for compactness/readability, whereas other places function declarations and calls got exploded to single parameters/arguments per line. I am actually a fan of the approach here, of coder's discretion as to how many parameters/arguments go on each line on a local basis for compactness/readability. Should/could we use multiple parameters/arguments on one line in some of the other places to beneficial effect? I am not recommending this location be changed, and I am in favor of the new final comma.

OK, so here's an interesting point: here you include an arbitrary varying number of arguments on each line of the call, grouped/formatted as you see fit for compactness/readability, whereas other places function declarations and calls got exploded to single parameters/arguments per line. I am actually a fan of the approach here, of coder's discretion as to how many parameters/arguments go on each line on a local basis for compactness/readability. Should/could we use multiple parameters/arguments on one line in some of the other places to beneficial effect? I am not recommending this location be changed, and I am in favor of the new final comma.
Vectornaut commented 2025-08-02 08:18:47 +00:00
Copy link

That sounds fine. I've suggested some groupings for SceneSpheres::push and ScenePoints::push. Are there any other function declarations that you'd like to see grouped in this way?

That sounds fine. I've [suggested](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3155) some groupings for `SceneSpheres::push` and `ScenePoints::push`. Are there any other function declarations that you'd like to see grouped in this way?
glen commented 2025-08-03 17:12:29 +00:00
Copy link

Not specifically that I am seeing at the moment. Just want to establish such grouping as a reasonable practice, rather than some other style guides that insist on exactly one argument per line as soon as the entire call is not on one line (such as the one we are using for Numberscope, which bugs me).
So resolving.

Not specifically that I am seeing at the moment. Just want to establish such grouping as a reasonable practice, rather than some other style guides that insist on exactly one argument per line as soon as the entire call is not on one line (such as the one we are using for Numberscope, which bugs me). So resolving.
👍 1
) {
state = better_state;
history.backoff_steps.push(backoff_steps);
} else {
return Realization {
result: Err("Line search failed".to_string()),
history
}
history,
};
}
}
let result = if state.loss < tol {
// express the uniform basis in the standard basis
const UNIFORM_DIM: usize = 4;
@ -539,7 +537,7 @@ pub mod examples {
[
sphere(0.0, 0.0, 0.0, 15.0),
sphere(0.0, 0.0, -9.0, 5.0),
sphere(0.0, 0.0, 11.0, 3.0)
sphere(0.0, 0.0, 11.0, 3.0),
].into_iter().chain(
(1..=6).map(
|k| {
@ -598,7 +596,7 @@ pub mod examples {
point(0.0, 0.0, 0.0),
point(ang_hor.cos(), ang_hor.sin(), 0.0),
point(x_vert, y_vert, -0.5),
point(x_vert, y_vert, 0.5)
point(x_vert, y_vert, 0.5),
]
}
).collect::<Vec<_>>().as_slice()
@ -641,15 +639,15 @@ mod tests {
MatrixEntry { index: (0, 0), value: 14.0 },
MatrixEntry { index: (0, 2), value: 28.0 },
MatrixEntry { index: (1, 1), value: 42.0 },
MatrixEntry { index: (1, 2), value: 49.0 }
MatrixEntry { index: (1, 2), value: 49.0 },
]);
let config = DMatrix::<f64>::from_row_slice(2, 3, &[
1.0, 2.0, 3.0,
4.0, 5.0, 6.0
4.0, 5.0, 6.0,
]);
let expected_result = DMatrix::<f64>::from_row_slice(2, 3, &[
14.0, 2.0, 28.0,
4.0, 42.0, 49.0
4.0, 42.0, 49.0,
]);
assert_eq!(frozen.freeze(&config), expected_result);
}
@ -660,15 +658,15 @@ mod tests {
MatrixEntry { index: (0, 0), value: 19.0 },
MatrixEntry { index: (0, 2), value: 39.0 },
MatrixEntry { index: (1, 1), value: 59.0 },
MatrixEntry { index: (1, 2), value: 69.0 }
MatrixEntry { index: (1, 2), value: 69.0 },
]);
let attempt = DMatrix::<f64>::from_row_slice(2, 3, &[
1.0, 2.0, 3.0,
4.0, 5.0, 6.0
4.0, 5.0, 6.0,
]);
let expected_result = DMatrix::<f64>::from_row_slice(2, 3, &[
18.0, 0.0, 36.0,
0.0, 54.0, 63.0
0.0, 54.0, 63.0,
]);
assert_eq!(target.sub_proj(&attempt), expected_result);
}
@ -686,7 +684,7 @@ mod tests {
DMatrix::from_columns(&[
sphere(1.0, 0.0, 0.0, a),
sphere(-0.5, a, 0.0, a),
sphere(-0.5, -a, 0.0, a)
sphere(-0.5, -a, 0.0, a),
])
};
let state = SearchState::from_config(&gram, config);
@ -700,7 +698,7 @@ mod tests {
fn frozen_entry_test() {
let mut problem = ConstraintProblem::from_guess(&[
point(0.0, 0.0, 2.0),
sphere(0.0, 0.0, 0.0, 0.95)
sphere(0.0, 0.0, 0.0, 0.95),
]);
for j in 0..2 {
for k in j..2 {
@ -744,7 +742,7 @@ mod tests {
let mut problem = ConstraintProblem::from_guess(&[
sphere(0.0, 0.0, 0.0, -2.0),
sphere(0.0, 0.0, 1.0, 1.0),
sphere(0.0, 0.0, -1.0, 1.0)
sphere(0.0, 0.0, -1.0, 1.0),
]);
for j in 0..3 {
for k in j..3 {
@ -774,8 +772,8 @@ mod tests {
DMatrix::<f64>::from_column_slice(UNIFORM_DIM, assembly_dim, &[
0.0, 0.0, 0.0, 0.0,
0.0, 0.0, -0.5, -0.5,
0.0, 0.0, -0.5, 0.5
])
0.0, 0.0, -0.5, 0.5,
]),
];
let tangent_motions_std = vec![
basis_matrix((0, 1), element_dim, assembly_dim),
@ -785,8 +783,8 @@ mod tests {
DMatrix::<f64>::from_column_slice(element_dim, assembly_dim, &[
0.0, 0.0, 0.0, 0.00, 0.0,
0.0, 0.0, -1.0, -0.25, -1.0,
0.0, 0.0, -1.0, 0.25, 1.0
])
0.0, 0.0, -1.0, 0.25, 1.0,
]),
];
// confirm that the dimension of the tangent space is no greater than
@ -862,10 +860,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]),
]
}
).collect::<Vec<_>>()
).collect::<Vec<_>>(),
];
let tangent_motions_std = tangent_motions_unif.iter().map(
|motion| DMatrix::from_columns(
@ -898,7 +896,7 @@ mod tests {
0.0, 1.0, 0.0, 0.0, dis[1],
0.0, 0.0, 1.0, 0.0, dis[2],
2.0*dis[0], 2.0*dis[1], 2.0*dis[2], 1.0, dis.norm_squared(),
0.0, 0.0, 0.0, 0.0, 1.0
0.0, 0.0, 0.0, 0.0, 1.0,
])
}
@ -910,7 +908,7 @@ mod tests {
const SCALED_TOL: f64 = 1.0e-12;
let mut problem_orig = ConstraintProblem::from_guess(&[
sphere(0.0, 0.0, 0.5, 1.0),
sphere(0.0, 0.0, -0.5, 1.0)
sphere(0.0, 0.0, -0.5, 1.0),
]);
problem_orig.gram.push_sym(0, 0, 1.0);
problem_orig.gram.push_sym(1, 1, 1.0);
@ -928,13 +926,13 @@ mod tests {
let a = 0.5 * FRAC_1_SQRT_2;
DMatrix::from_columns(&[
sphere(a, 0.0, 7.0 + a, 1.0),
sphere(-a, 0.0, 7.0 - a, 1.0)
sphere(-a, 0.0, 7.0 - a, 1.0),
])
};
let problem_tfm = ConstraintProblem {
gram: problem_orig.gram,
frozen: problem_orig.frozen,
guess: guess_tfm,
frozen: problem_orig.frozen
};
let Realization { result: result_tfm, history: history_tfm } = realize_gram(
&problem_tfm, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110
@ -962,7 +960,7 @@ mod tests {
0.0, 1.0, 0.0, 0.0, 0.0,
FRAC_1_SQRT_2, 0.0, FRAC_1_SQRT_2, 0.0, 0.0,
0.0, 0.0, 0.0, 1.0, 0.0,
0.0, 0.0, 0.0, 0.0, 1.0
0.0, 0.0, 0.0, 0.0, 1.0,
]);
let transl = translation(Vector3::new(0.0, 0.0, 7.0));
let motion_proj_tfm = transl * rot * motion_orig_proj;

6
app-proto/src/main.rs
View file

@ -14,20 +14,20 @@ use components::{
add_remove::AddRemove,
diagnostics::Diagnostics,
display::Display,
outline::Outline
outline::Outline,
};
#[derive(Clone)]
struct AppState {
assembly: Assembly,
selection: Signal<BTreeSet<Rc<dyn Element>>>
selection: Signal<BTreeSet<Rc<dyn Element>>>,
}
impl AppState {
fn new() -> AppState {
AppState {
assembly: Assembly::new(),
selection: create_signal(BTreeSet::default())
selection: create_signal(BTreeSet::default()),
}
}

4
app-proto/src/specified.rs
View file

@ -13,7 +13,7 @@ use std::num::ParseFloatError;
#[readonly::make]
pub struct SpecifiedValue {
pub spec: String,
pub value: Option<f64>
pub value: Option<f64>,
}
impl SpecifiedValue {
@ -37,7 +37,7 @@ impl TryFrom<String> for SpecifiedValue {
Ok(SpecifiedValue::from_empty_spec())
} else {
spec.parse::<f64>().map(
|value| SpecifiedValue { spec: spec, value: Some(value) }
|value| SpecifiedValue { spec, value: Some(value) }
)
}
}