feat: Point coordinate regulators #118
64
README.md
|
|
@ -12,11 +12,11 @@ Note that currently this is just the barest beginnings of the project, more of a
|
||||||
|
|
||||||
### Implementation goals
|
### Implementation goals
|
||||||
|
|
||||||
* Comfortable, intuitive UI
|
* Provide a comfortable, intuitive UI
|
||||||
|
Vectornaut marked this conversation as resolved
|
|||||||
|
|
||||||
* Able to run in browser (so implemented in WASM-compatible language)
|
* Allow execution in browser (so implemented in WASM-compatible language)
|
||||||
|
|
||||||
* Produce scalable graphics of 3D diagrams, and maybe STL files (or other fabricatable file format) as well.
|
* Produce scalable graphics of 3D diagrams, and maybe STL files (or other fabricatable file format) as well
|
||||||
|
|
||||||
## Prototype
|
## Prototype
|
||||||
|
|
||||||
|
|
@ -24,38 +24,40 @@ The latest prototype is in the folder `app-proto`. It includes both a user inter
|
||||||
|
|
||||||
### Install the prerequisites
|
### Install the prerequisites
|
||||||
|
|
||||||
1. Install [`rustup`](https://rust-lang.github.io/rustup/): the officially recommended Rust toolchain manager
|
1. Install [`rustup`](https://rust-lang.github.io/rustup/): the officially recommended Rust toolchain manager.
|
||||||
- It's available on Ubuntu as a [Snap](https://snapcraft.io/rustup)
|
- It's available on Ubuntu as a [Snap](https://snapcraft.io/rustup).
|
||||||
2. Call `rustup default stable` to "download the latest stable release of Rust and set it as your default toolchain"
|
2. Call `rustup default stable` to "download the latest stable release of Rust and set it as your default toolchain".
|
||||||
- If you forget, the `rustup` [help system](https://github.com/rust-lang/rustup/blob/d9b3601c3feb2e88cf3f8ca4f7ab4fdad71441fd/src/errors.rs#L109-L112) will remind you
|
- If you forget, the `rustup` [help system](https://github.com/rust-lang/rustup/blob/d9b3601c3feb2e88cf3f8ca4f7ab4fdad71441fd/src/errors.rs#L109-L112) will remind you.
|
||||||
3. Call `rustup target add wasm32-unknown-unknown` to add the [most generic 32-bit WebAssembly target](https://doc.rust-lang.org/nightly/rustc/platform-support/wasm32-unknown-unknown.html)
|
3. Call `rustup target add wasm32-unknown-unknown` to add the [most generic 32-bit WebAssembly target](https://doc.rust-lang.org/nightly/rustc/platform-support/wasm32-unknown-unknown.html).
|
||||||
4. Call `cargo install wasm-pack` to install the [WebAssembly toolchain](https://rustwasm.github.io/docs/wasm-pack/)
|
4. Call `cargo install wasm-pack` to install the [WebAssembly toolchain](https://rustwasm.github.io/docs/wasm-pack/).
|
||||||
5. Call `cargo install trunk` to install the [Trunk](https://trunkrs.dev/) web-build tool
|
5. Call `cargo install trunk` to install the [Trunk](https://trunkrs.dev/) web-build tool.
|
||||||
|
- In the future, `trunk` can be updated with the same command. (You may need the `--locked` flag if your ambient version of `rustc` does not match that required by `trunk`.)
|
||||||
|
Vectornaut
commented
Very minor: the rest of the list is formatted as one sentence per list item, with no final period. It would be nice to keep that consistent. Very minor: the rest of the list is formatted as one sentence per list item, with no final period. It would be nice to keep that consistent.
glen
commented
OK, I have properly punctuated the whole file, so that complete sentences have periods, and fragments do not. OK, I have properly punctuated the whole file, so that complete sentences have periods, and fragments do not.
|
|||||||
6. Add the `.cargo/bin` folder in your home directory to your executable search path
|
6. Add the `.cargo/bin` folder in your home directory to your executable search path
|
||||||
- This lets you call Trunk, and other tools installed by Cargo, without specifying their paths
|
- This lets you call Trunk, and other tools installed by Cargo, without specifying their paths.
|
||||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
The information here looks accurate to me, although I've never installed a global tool with the The existing style for this file does not use manual line-wrapping. I'd prefer to keep the new text consistent with that style by putting it all on one line. The information here looks accurate to me, although I've never installed a global tool with the `--locked` flag, and I don't know whether it's recommended.
The existing style for this file does not use manual line-wrapping. I'd prefer to keep the new text consistent with that style by putting it all on one line.
glen
commented
Ok, I will go ahead and remove the newlines in the additions to this file, but i will say that trying to impose formatting rules like this on markdown files feels rather contradictory to the spirit/genesis of markdown: it's an explicit reaction to various markup languages to be more free-form and lightweight. Ok, I will go ahead and remove the newlines in the additions to this file, but i will say that trying to impose formatting rules like this on markdown files feels rather contradictory to the spirit/genesis of markdown: it's an explicit reaction to various markup languages to be more free-form and lightweight.
glen
commented
`--locked` is in fact now part of the installation command that trunk itself recommends, so i feel pretty safe recommending it here.
|
|||||||
- On POSIX systems, the search path is stored in the `PATH` environment variable
|
- On POSIX systems, the search path is stored in the `PATH` environment variable.
|
||||||
|
- Alternatively, if you don't want to adjust your `PATH`, you can install `trunk` in another directory `DIR` via `cargo install --root DIR trunk`.
|
||||||
|
|
||||||
### Play with the prototype
|
### Play with the prototype
|
||||||
|
|
||||||
|
Vectornaut
commented
I'll trust your experience with installing Cargo tools at custom locations, since I've never tried it! The request above about manual line-wrapping also applies here. I'll trust your experience with installing Cargo tools at custom locations, since I've never tried it!
The [request](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3346) above about manual line-wrapping also applies here.
|
|||||||
1. From the `app-proto` folder, call `trunk serve --release` to build and serve the prototype
|
1. From the `app-proto` folder, call `trunk serve --release` to build and serve the prototype.
|
||||||
- The crates the prototype depends on will be downloaded and served automatically
|
- The crates the prototype depends on will be downloaded and served automatically.
|
||||||
- For a faster build, at the expense of a much slower prototype, you can call `trunk serve` without the `--release` flag
|
- For a faster build, at the expense of a much slower prototype, you can call `trunk serve` without the `--release` flag.
|
||||||
- If you want to stay in the top-level folder, you can call `trunk serve --config app-proto [--release]` from there instead.
|
- If you want to stay in the top-level folder, you can call `trunk serve --config app-proto [--release]` from there instead.
|
||||||
3. In a web browser, visit one of the URLs listed under the message `INFO 📡 server listening at:`
|
3. In a web browser, visit one of the URLs listed under the message `INFO 📡 server listening at:`.
|
||||||
- Touching any file in the `app-proto` folder will make Trunk rebuild and live-reload the prototype
|
- Touching any file in the `app-proto` folder will make Trunk rebuild and live-reload the prototype.
|
||||||
4. Press *ctrl+C* in the shell where Trunk is running to stop serving the prototype
|
4. Press *ctrl+C* in the shell where Trunk is running to stop serving the prototype.
|
||||||
|
|
||||||
### Run the engine on some example problems
|
### Run the engine on some example problems
|
||||||
|
|
||||||
1. Use `sh` to run the script `tools/run-examples.sh`
|
1. Use `sh` to run the script `tools/run-examples.sh`.
|
||||||
- The script is location-independent, so you can do this from anywhere in the dyna3 repository
|
- The script is location-independent, so you can do this from anywhere in the dyna3 repository.
|
||||||
- The call from the top level of the repository is:
|
- The call from the top level of the repository is:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
sh tools/run-examples.sh
|
sh tools/run-examples.sh
|
||||||
```
|
```
|
||||||
- For each example problem, the engine will print the value of the loss function at each optimization step
|
- For each example problem, the engine will print the value of the loss function at each optimization step.
|
||||||
- The first example that prints is the same as the Irisawa hexlet example from the Julia version of the engine prototype. If you go into `engine-proto/gram-test`, launch Julia, and then
|
- The first example that prints is the same as the Irisawa hexlet example from the Julia version of the engine prototype. If you go into `engine-proto/gram-test`, launch Julia, and then execute
|
||||||
|
|
||||||
```julia
|
```julia
|
||||||
include("irisawa-hexlet.jl")
|
include("irisawa-hexlet.jl")
|
||||||
|
|
@ -64,24 +66,24 @@ The latest prototype is in the folder `app-proto`. It includes both a user inter
|
||||||
end
|
end
|
||||||
```
|
```
|
||||||
|
|
||||||
you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show
|
you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show.
|
||||||
|
|
||||||
### Run the automated tests
|
### Run the automated tests
|
||||||
|
|
||||||
1. Go into the `app-proto` folder
|
1. Go into the `app-proto` folder.
|
||||||
2. Call `cargo test`
|
2. Call `cargo test`.
|
||||||
|
|
||||||
### Deploy the prototype
|
### Deploy the prototype
|
||||||
|
|
||||||
1. From the `app-proto` folder, call `trunk build --release`
|
1. From the `app-proto` folder, call `trunk build --release`.
|
||||||
- Building in [release mode](https://doc.rust-lang.org/cargo/reference/profiles.html#release) produces an executable which is smaller and often much faster, but harder to debug and more time-consuming to build
|
- Building in [release mode](https://doc.rust-lang.org/cargo/reference/profiles.html#release) produces an executable which is smaller and often much faster, but harder to debug and more time-consuming to build.
|
||||||
- If you want to stay in the top-level folder, you can call `trunk build --config app-proto --release` from there instead
|
- If you want to stay in the top-level folder, you can call `trunk build --config app-proto --release` from there instead.
|
||||||
2. Use `sh` to run the packaging script `tools/package-for-deployment.sh`.
|
2. Use `sh` to run the packaging script `tools/package-for-deployment.sh`.
|
||||||
- The script is location-independent, so you can do this from anywhere in the dyna3 repository
|
- The script is location-independent, so you can do this from anywhere in the dyna3 repository.
|
||||||
- The call from the top level of the repository is:
|
- The call from the top level of the repository is:
|
||||||
```bash
|
```bash
|
||||||
sh tools/package-for-deployment.sh
|
sh tools/package-for-deployment.sh
|
||||||
```
|
```
|
||||||
- This will overwrite or replace the files in `deploy/dyna3`
|
- This will overwrite or replace the files in `deploy/dyna3`.
|
||||||
3. Put the contents of `deploy/dyna3` in the folder on your server that the prototype will be served from.
|
3. Put the contents of `deploy/dyna3` in the folder on your server that the prototype will be served from.
|
||||||
- To simplify uploading, you might want to combine these files into an archive called `deploy/dyna3.zip`. Git has been set to ignore this path
|
- To simplify uploading, you might want to combine these files into an archive called `deploy/dyna3.zip`. Git has been set to ignore this path.
|
||||||
|
|
|
||||||
21
app-proto/Cargo.lock
generated
|
|
@ -255,6 +255,7 @@ dependencies = [
|
||||||
"charming",
|
"charming",
|
||||||
"console_error_panic_hook",
|
"console_error_panic_hook",
|
||||||
"dyna3",
|
"dyna3",
|
||||||
|
"enum-iterator",
|
||||||
"itertools",
|
"itertools",
|
||||||
"js-sys",
|
"js-sys",
|
||||||
"lazy_static",
|
"lazy_static",
|
||||||
|
|
@ -271,6 +272,26 @@ version = "1.13.0"
|
||||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||||
checksum = "60b1af1c220855b6ceac025d3f6ecdd2b7c4894bfe9cd9bda4fbb4bc7c0d4cf0"
|
checksum = "60b1af1c220855b6ceac025d3f6ecdd2b7c4894bfe9cd9bda4fbb4bc7c0d4cf0"
|
||||||
|
|
||||||
|
[[package]]
|
||||||
|
name = "enum-iterator"
|
||||||
|
version = "2.3.0"
|
||||||
|
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||||
|
checksum = "a4549325971814bda7a44061bf3fe7e487d447cba01e4220a4b454d630d7a016"
|
||||||
|
dependencies = [
|
||||||
|
"enum-iterator-derive",
|
||||||
|
]
|
||||||
|
|
||||||
|
[[package]]
|
||||||
|
name = "enum-iterator-derive"
|
||||||
|
version = "1.5.0"
|
||||||
|
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||||
|
checksum = "685adfa4d6f3d765a26bc5dbc936577de9abf756c1feeb3089b01dd395034842"
|
||||||
|
dependencies = [
|
||||||
|
"proc-macro2",
|
||||||
|
"quote",
|
||||||
|
"syn",
|
||||||
|
]
|
||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
name = "equivalent"
|
name = "equivalent"
|
||||||
version = "1.0.1"
|
version = "1.0.1"
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ default = ["console_error_panic_hook"]
|
||||||
dev = []
|
dev = []
|
||||||
|
|
||||||
[dependencies]
|
[dependencies]
|
||||||
|
enum-iterator = "2.3.0"
|
||||||
itertools = "0.13.0"
|
itertools = "0.13.0"
|
||||||
js-sys = "0.3.70"
|
js-sys = "0.3.70"
|
||||||
lazy_static = "1.5.0"
|
lazy_static = "1.5.0"
|
||||||
|
|
|
||||||
|
|
@ -1,10 +1,11 @@
|
||||||
|
use enum_iterator::{all, Sequence};
|
||||||
use nalgebra::{DMatrix, DVector, DVectorView};
|
use nalgebra::{DMatrix, DVector, DVectorView};
|
||||||
use std::{
|
use std::{
|
||||||
cell::Cell,
|
cell::Cell,
|
||||||
cmp::Ordering,
|
cmp::Ordering,
|
||||||
collections::{BTreeMap, BTreeSet},
|
collections::{BTreeMap, BTreeSet},
|
||||||
fmt,
|
fmt,
|
||||||
fmt::{Debug, Formatter},
|
fmt::{Debug, Display, Formatter},
|
||||||
hash::{Hash, Hasher},
|
hash::{Hash, Hasher},
|
||||||
rc::Rc,
|
rc::Rc,
|
||||||
sync::{atomic, atomic::AtomicU64},
|
sync::{atomic, atomic::AtomicU64},
|
||||||
|
|
@ -26,6 +27,7 @@ use crate::{
|
||||||
ConfigSubspace,
|
ConfigSubspace,
|
||||||
ConstraintProblem,
|
ConstraintProblem,
|
||||||
DescentHistory,
|
DescentHistory,
|
||||||
|
MatrixEntry,
|
||||||
Realization,
|
Realization,
|
||||||
},
|
},
|
||||||
specified::SpecifiedValue,
|
specified::SpecifiedValue,
|
||||||
|
|
@ -84,6 +86,14 @@ impl Ord for dyn Serial {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Small helper function to generate consistent errors when there
|
||||||
|
// are indexing issues in a ProblemPoser
|
||||||
|
fn indexing_error(item: &str, name: &str, actor: &str) -> String {
|
||||||
|
format!(
|
||||||
|
"{item} \"{name}\" must be indexed before {actor} writes problem data"
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
pub trait ProblemPoser {
|
pub trait ProblemPoser {
|
||||||
fn pose(&self, problem: &mut ConstraintProblem);
|
fn pose(&self, problem: &mut ConstraintProblem);
|
||||||
}
|
}
|
||||||
|
|
@ -125,8 +135,8 @@ pub trait Element: Serial + ProblemPoser + DisplayItem {
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Debug for dyn Element {
|
impl Debug for dyn Element {
|
||||||
fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), fmt::Error> {
|
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
|
||||||
self.id().fmt(f)
|
Debug::fmt(&self.id(), f)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -249,8 +259,7 @@ impl Serial for Sphere {
|
||||||
impl ProblemPoser for Sphere {
|
impl ProblemPoser for Sphere {
|
||||||
fn pose(&self, problem: &mut ConstraintProblem) {
|
fn pose(&self, problem: &mut ConstraintProblem) {
|
||||||
let index = self.column_index().expect(
|
let index = self.column_index().expect(
|
||||||
format!("Sphere \"{}\" should be indexed before writing problem data", self.id).as_str()
|
indexing_error("Sphere", &self.id, "it").as_str());
|
||||||
);
|
|
||||||
problem.gram.push_sym(index, index, 1.0);
|
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());
|
||||||
}
|
}
|
||||||
|
|
@ -269,6 +278,7 @@ pub struct Point {
|
||||||
|
|
||||||
impl Point {
|
impl Point {
|
||||||
const WEIGHT_COMPONENT: usize = 3;
|
const WEIGHT_COMPONENT: usize = 3;
|
||||||
|
const NORM_COMPONENT: usize = 4;
|
||||||
|
|
||||||
pub fn new(
|
pub fn new(
|
||||||
id: String,
|
id: String,
|
||||||
|
|
@ -302,6 +312,15 @@ impl Element for Point {
|
||||||
point(0.0, 0.0, 0.0),
|
point(0.0, 0.0, 0.0),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn default_regulators(self: Rc<Self>) -> Vec<Rc<dyn Regulator>> {
|
||||||
|
all::<Axis>()
|
||||||
|
.map(|axis| {
|
||||||
|
Rc::new(PointCoordinateRegulator::new(self.clone(), axis))
|
||||||
|
as Rc::<dyn Regulator>
|
||||||
|
})
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
fn id(&self) -> &String {
|
fn id(&self) -> &String {
|
||||||
&self.id
|
&self.id
|
||||||
|
|
@ -345,8 +364,7 @@ impl Serial for Point {
|
||||||
impl ProblemPoser for Point {
|
impl ProblemPoser for Point {
|
||||||
fn pose(&self, problem: &mut ConstraintProblem) {
|
fn pose(&self, problem: &mut ConstraintProblem) {
|
||||||
let index = self.column_index().expect(
|
let index = self.column_index().expect(
|
||||||
format!("Point \"{}\" should be indexed before writing problem data", self.id).as_str()
|
indexing_error("Point", &self.id, "it").as_str());
|
||||||
);
|
|
||||||
problem.gram.push_sym(index, index, 0.0);
|
problem.gram.push_sym(index, index, 0.0);
|
||||||
problem.frozen.push(Self::WEIGHT_COMPONENT, index, 0.5);
|
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());
|
||||||
|
|
@ -436,8 +454,8 @@ impl ProblemPoser for InversiveDistanceRegulator {
|
||||||
if let Some(val) = set_pt.value {
|
if let Some(val) = set_pt.value {
|
||||||
let [row, col] = self.subjects.each_ref().map(
|
let [row, col] = self.subjects.each_ref().map(
|
||||||
|subj| subj.column_index().expect(
|
|subj| subj.column_index().expect(
|
||||||
"Subjects should be indexed before inversive distance regulator writes problem data"
|
indexing_error("Subject", subj.id(),
|
||||||
)
|
"inversive distance regulator").as_str())
|
||||||
);
|
);
|
||||||
problem.gram.push_sym(row, col, val);
|
problem.gram.push_sym(row, col, val);
|
||||||
}
|
}
|
||||||
|
|
@ -446,14 +464,14 @@ impl ProblemPoser for InversiveDistanceRegulator {
|
||||||
}
|
}
|
||||||
|
|
||||||
pub struct HalfCurvatureRegulator {
|
pub struct HalfCurvatureRegulator {
|
||||||
pub subject: Rc<dyn Element>,
|
pub subject: Rc<Sphere>,
|
||||||
pub measurement: ReadSignal<f64>,
|
pub measurement: ReadSignal<f64>,
|
||||||
pub set_point: Signal<SpecifiedValue>,
|
pub set_point: Signal<SpecifiedValue>,
|
||||||
serial: u64,
|
serial: u64,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl HalfCurvatureRegulator {
|
impl HalfCurvatureRegulator {
|
||||||
pub fn new(subject: Rc<dyn Element>) -> Self {
|
pub fn new(subject: Rc<Sphere>) -> Self {
|
||||||
let measurement = subject.representation().map(
|
let measurement = subject.representation().map(
|
||||||
|rep| rep[Sphere::CURVATURE_COMPONENT]
|
|rep| rep[Sphere::CURVATURE_COMPONENT]
|
||||||
);
|
);
|
||||||
|
|
@ -490,14 +508,85 @@ impl ProblemPoser for HalfCurvatureRegulator {
|
||||||
self.set_point.with_untracked(|set_pt| {
|
self.set_point.with_untracked(|set_pt| {
|
||||||
if let Some(val) = set_pt.value {
|
if let Some(val) = set_pt.value {
|
||||||
let col = self.subject.column_index().expect(
|
let col = self.subject.column_index().expect(
|
||||||
"Subject should be indexed before half-curvature regulator writes problem data"
|
indexing_error("Subject", &self.subject.id,
|
||||||
);
|
"half-curvature regulator").as_str());
|
||||||
problem.frozen.push(Sphere::CURVATURE_COMPONENT, col, val);
|
problem.frozen.push(Sphere::CURVATURE_COMPONENT, col, val);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
glen marked this conversation as resolved
Outdated
glen
commented
I generally prefer to punctuate code as one would text, when possible. Can we remove the two extraneous spaces you just added here? They don't seem to improve readability (as we are all used to no space between brackets and content). I generally prefer to punctuate code as one would text, when possible. Can we remove the two extraneous spaces you just added here? They don't seem to improve readability (as we are all used to no space between brackets and content).
Vectornaut
commented
This spacing convention is widely used in Rust code, and it's recommended in the Rust style guide:
I like it a lot, and I've used it in hundreds of places in the dyna3 source code. If you're adamant about changing it, I'd prefer to do it throughout the source code in a formatting-only pull request. This spacing convention is widely used in Rust code, and it's [recommended](https://doc.rust-lang.org/beta/style-guide/expressions.html#blocks) in the Rust style guide:
> For a single-line block, put spaces after the opening brace and before the closing brace.
I like it a lot, and I've used it in hundreds of places in the dyna3 source code. If you're adamant about changing it, I'd prefer to do it throughout the source code in a formatting-only pull request.
glen
commented
We don't have to fix it now. I don't care at all about the Rust style guide, it's an assembly of weird rigidities from my point of view (what business does a language have semi-enforcing snake_case identifiers? I thought typography reiterating semantic category went into the trashbin with Perl a decade or more ago...). I very much like the convention of punctuating code as close as possible to regular text. I get it that you are used to these spaces that look very extra to me. We'll figure out how to resolve this at some point. No need to spin our wheels on it at the moment. We don't have to fix it now. I don't care at all about the Rust style guide, it's an assembly of weird rigidities from my point of view (what business does a _language_ have semi-enforcing snake_case identifiers? I thought typography reiterating semantic category went into the trashbin with Perl a decade or more ago...). I very much like the convention of punctuating code as close as possible to regular text. I get it that you are used to these spaces that look very extra to me. We'll figure out how to resolve this at some point. No need to spin our wheels on it at the moment.
|
|||||||
}
|
}
|
||||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
The associated constant The associated constant `Axis::N_AXIS`, which is defined by hand here, has the same value as the associated constant [`Axis::CARDINALITY`](https://docs.rs/enum-iterator/latest/enum_iterator/trait.Sequence.html#associatedconstant.CARDINALITY), which is defined automatically when we derive the `Sequence` trait. I'd recommend removing `N_AXIS` and using `CARDINALITY` in its place. I'm confident that `CARDINALITY` is currently 3, because I get a compilation error if I replace it with a value different from 3 in the definition of `Axis::NAME`.
|
|||||||
|
|
||||||
|
#[derive(Clone, Copy, Sequence)]
|
||||||
|
pub enum Axis { X = 0, Y = 1, Z = 2 }
|
||||||
|
|
||||||
|
impl Axis {
|
||||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
Whoops, here's a formatting mistake from the revision I made. I should've just written and changed the relevant Would you mind including that change in your next round of revisions? Whoops, here's a formatting mistake from the revision I made. I should've just written
```rust
impl Display for Axis
```
and changed the relevant `use` expression as follows:
```diff
- fmt::{Debug, Formatter}
+ fmt::{Debug, Display, Formatter}
```
Would you mind including that change in your next round of revisions?
glen
commented
Yes of course, and this makes sense. Yes of course, and this makes sense.
|
|||||||
|
fn name(&self) -> &'static str {
|
||||||
|
match self { Axis::X => "X", Axis::Y => "Y", Axis::Z => "Z" }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
impl Display for Axis {
|
||||||
|
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||||
|
f.write_str(self.name())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
pub struct PointCoordinateRegulator {
|
||||||
|
pub subject: Rc<Point>,
|
||||||
|
pub axis: Axis,
|
||||||
|
pub measurement: ReadSignal<f64>,
|
||||||
|
pub set_point: Signal<SpecifiedValue>,
|
||||||
|
serial: u64
|
||||||
|
}
|
||||||
|
|
||||||
|
impl PointCoordinateRegulator {
|
||||||
|
pub fn new(subject: Rc<Point>, axis: Axis) -> Self {
|
||||||
|
let measurement = subject.representation().map(
|
||||||
|
move |rep| rep[axis as usize]
|
||||||
|
);
|
||||||
|
let set_point = create_signal(SpecifiedValue::from_empty_spec());
|
||||||
|
Self { subject, axis, measurement, set_point, serial: Self::next_serial() }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
impl Serial for PointCoordinateRegulator {
|
||||||
|
fn serial(&self) -> u64 { self.serial }
|
||||||
|
}
|
||||||
|
|
||||||
|
impl Regulator for PointCoordinateRegulator {
|
||||||
|
fn subjects(&self) -> Vec<Rc<dyn Element>> { vec![self.subject.clone()] }
|
||||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
DesignThis kind of interaction between problem posers is something we didn't anticipate when designing the TestingTo test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call with I tried this out locally, and it confirms that the interaction system is working in at least some cases. CommentingI'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the
### Design
This kind of interaction between problem posers is something we didn't anticipate when designing the `ProblemPoser` trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a `/* KLUDGE */` flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.
### Testing
To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call
```rust
// log the Gram matrix
console_log!("Gram matrix:\n{}", problem.gram);
```
with
```rust
// log the Gram matrix and frozen entries
console_log!("Gram matrix:\n{}", problem.gram);
console_log!("Frozen entries:\n{}", problem.frozen);
```
I tried this out locally, and it confirms that the interaction system is working in at least some cases.
### Commenting
I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the `/* KLUDGE */` flag suggested above and uses the capitalization convention that most other comments follow.
```rust
// if the three representation components that specify the
// point's Euclidean coordinates have been frozen, then freeze
// the norm component too
/* KLUDGE */
// we didn't anticipate this kind of interaction when we
// designed the problem poser system, so the implementation
// feels awkward
```
glen
commented
@Vectornaut wrote in #118 (comment):
What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks. @Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3259:
> the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.
What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.
glen
commented
OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place. OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.
Vectornaut
commented
I agree that the comment in issue #90 is enough to remind us about this.
We discussed this a little during our check-in meeting, and I jotted down a comment about our conclusions on issue #90. I agree that the comment in issue #90 is enough to remind us about this.
> What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.
We discussed this a little during our check-in meeting, and I jotted down a [comment](https://code.studioinfinity.org/StudioInfinity/dyna3/issues/90#issuecomment-3288) about our conclusions on issue #90.
|
|||||||
|
fn measurement(&self) -> ReadSignal<f64> { self.measurement }
|
||||||
|
fn set_point(&self) -> Signal<SpecifiedValue> { self.set_point }
|
||||||
|
}
|
||||||
|
|
||||||
|
Vectornaut
commented
For consistency with
For consistency with `InversiveDistanceRegulator`, I'd change the panic message to:
> Subject should be indexed before point coordinate regulator writes problem data
glen
commented
As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data". As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".
glen
commented
Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now). Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).
Vectornaut
commented
The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes.
Agreed. The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes.
- Add more detail to the helper name: maybe something like `index_required_message`, `missing_index_panic_message`, or `index_before_posing_message`?
- Rename the `thing` argument to something like `role` or `kind_of_thing`. Or, more broadly, name the arguments something like `object_kind`, `object_name`, `actor`?
> As far as I can see from the code, "must" is correct.
Agreed.
Vectornaut
commented
To me, the placement of the new helper is kind of weird to me. It's right in the middle of the To me, the placement of the new helper is kind of weird to me. It's right in the middle of the `Sphere` definition and implementation section, which makes it seem associated with `Sphere`, when in fact it's relevant to many problem-posers. It might fit better above the definition of the `ProblemPoser` trait, although I'd be open to other ideas about where to put it.
glen
commented
Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder. Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.
Vectornaut
commented
Great!
The main thing I'd like to clarify, if possible, is that > Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it.
Great!
> Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject.
The main thing I'd like to clarify, if possible, is that `thing` and `name` go together, and that `thing` is a type or role, while `name` is an identifier.
|
|||||||
|
impl ProblemPoser for PointCoordinateRegulator {
|
||||||
|
fn pose(&self, problem: &mut ConstraintProblem) {
|
||||||
|
self.set_point.with_untracked(|set_pt| {
|
||||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed. I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment. I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like. (I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.) I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed.
I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment.
I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like.
```
// if all three of the subject's spatial coordinates have been
// frozen, then freeze its norm component too
```
(I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)
glen
commented
I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line. I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.
glen
commented
Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them. Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.
Vectornaut
commented
Thanks for reflowing and rewording the comment!
I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style. The other reflows in commit Thanks for reflowing and rewording the comment!
> Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.
I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style.
The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the [line width](https://doc.rust-lang.org/beta/style-guide/index.html#indentation-and-line-width) and [comment width](https://doc.rust-lang.org/beta/style-guide/index.html#comments) rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.
glen
commented
I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok? I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?
Vectornaut
commented
Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit. > I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?
Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.
glen
commented
The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks. As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment:
The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks.
As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment:
* switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot.
* switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g.
```
fn myfn(arg: Type) {
if condition {
take_action(arg, other, long,
arguments, in, list)}}
fn my otherfn() -> ReturnType {
...
```
Vectornaut
commented
Sounds good!
Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now. > So I'd like to just stick with 80, thanks.
Sounds good!
> In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment
Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.
|
|||||||
|
if let Some(val) = set_pt.value {
|
||||||
|
let col = self.subject.column_index().expect(
|
||||||
|
indexing_error("Subject", &self.subject.id,
|
||||||
|
"point-coordinate regulator").as_str());
|
||||||
|
problem.frozen.push(self.axis as usize, col, val);
|
||||||
|
// If all three of the subject's spatial coordinates have been
|
||||||
|
// frozen, then freeze its norm component:
|
||||||
|
let mut coords = [0.0; Axis::CARDINALITY];
|
||||||
|
let mut nset: usize = 0;
|
||||||
|
for &MatrixEntry {index, value} in &(problem.frozen) {
|
||||||
|
Vectornaut marked this conversation as resolved
Vectornaut
commented
Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as `{ index, value }` elsewhere, but during review we discussed potentially changing this convention in the future.
glen
commented
It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool. It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.
|
|||||||
|
if index.1 == col && index.0 < Axis::CARDINALITY {
|
||||||
|
nset += 1;
|
||||||
|
coords[index.0] = value
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if nset == Axis::CARDINALITY {
|
||||||
|
let [x, y, z] = coords;
|
||||||
|
problem.frozen.push(
|
||||||
|
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// the velocity is expressed in uniform coordinates
|
// the velocity is expressed in uniform coordinates
|
||||||
pub struct ElementMotion<'a> {
|
pub struct ElementMotion<'a> {
|
||||||
pub element: Rc<dyn Element>,
|
pub element: Rc<dyn Element>,
|
||||||
|
|
@ -698,6 +787,7 @@ impl Assembly {
|
||||||
/* DEBUG */
|
/* DEBUG */
|
||||||
// log the Gram matrix
|
// log the Gram matrix
|
||||||
console_log!("Gram matrix:\n{}", problem.gram);
|
console_log!("Gram matrix:\n{}", problem.gram);
|
||||||
|
console_log!("Frozen entries:\n{}", problem.frozen);
|
||||||
|
|
||||||
/* DEBUG */
|
/* DEBUG */
|
||||||
// log the initial configuration matrix
|
// log the initial configuration matrix
|
||||||
|
|
@ -857,7 +947,8 @@ mod tests {
|
||||||
use crate::engine;
|
use crate::engine;
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[should_panic(expected = "Sphere \"sphere\" should be indexed before writing problem data")]
|
#[should_panic(expected =
|
||||||
|
"Sphere \"sphere\" must be indexed before it writes problem data")]
|
||||||
fn unindexed_element_test() {
|
fn unindexed_element_test() {
|
||||||
let _ = create_root(|| {
|
let _ = create_root(|| {
|
||||||
let elt = Sphere::default("sphere".to_string(), 0);
|
let elt = Sphere::default("sphere".to_string(), 0);
|
||||||
|
|
@ -866,7 +957,8 @@ mod tests {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
Vectornaut
commented
I'd indent this line by two tabs to match the convention you've used for other multi-line expressions. This won't affect the string, because the string continuation escape absorbs all subsequent whitespace, resuming the string at the next non-whitespace character. I'd indent this line by two tabs to match the convention you've used for other multi-line expressions. This won't affect the string, because the [string continuation escape](https://doc.rust-lang.org/reference/expressions/literal-expr.html#string-continuation-escapes) absorbs all subsequent whitespace, resuming the string at the next non-whitespace character.
glen
commented
Ah, didn't know that \ at end of line (a convention I hate, but apparently the only one available in this context) sucked up leading whitespace on the next line as well. Will do. Ah, didn't know that \ at end of line (a convention I hate, but apparently the only one available in this context) sucked up leading whitespace on the next line as well. Will do.
|
|||||||
#[should_panic(expected = "Subjects should be indexed before inversive distance regulator writes problem data")]
|
#[should_panic(expected = "Subject \"sphere1\" must be indexed before \
|
||||||
|
inversive distance regulator writes problem data")]
|
||||||
|
Vectornaut marked this conversation as resolved
Vectornaut
commented
When I originally wrote this test, it bothered me that the identifiers When I originally wrote this test, it bothered me that the identifiers `sphere0` and `sphere1` were assigned but never tested. The new, more informative indexing error message makes them part of the test, which is very satisfying.
|
|||||||
fn unindexed_subject_test_inversive_distance() {
|
fn unindexed_subject_test_inversive_distance() {
|
||||||
let _ = create_root(|| {
|
let _ = create_root(|| {
|
||||||
let subjects = [0, 1].map(
|
let subjects = [0, 1].map(
|
||||||
|
|
@ -927,4 +1019,4 @@ mod tests {
|
||||||
assert!((final_half_curv / INITIAL_HALF_CURV - 1.0).abs() < DRIFT_TOL);
|
assert!((final_half_curv / INITIAL_HALF_CURV - 1.0).abs() < DRIFT_TOL);
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -9,6 +9,7 @@ use crate::{
|
||||||
Element,
|
Element,
|
||||||
HalfCurvatureRegulator,
|
HalfCurvatureRegulator,
|
||||||
InversiveDistanceRegulator,
|
InversiveDistanceRegulator,
|
||||||
|
PointCoordinateRegulator,
|
||||||
Regulator,
|
Regulator,
|
||||||
},
|
},
|
||||||
specified::SpecifiedValue
|
specified::SpecifiedValue
|
||||||
|
|
@ -119,6 +120,20 @@ impl OutlineItem for HalfCurvatureRegulator {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl OutlineItem for PointCoordinateRegulator {
|
||||||
|
fn outline_item(self: Rc<Self>, _element: &Rc<dyn Element>) -> View {
|
||||||
|
let name = format!("{} coordinate", self.axis);
|
||||||
|
view! {
|
||||||
|
li(class = "regulator") {
|
||||||
|
div(class = "regulator-label") // for spacing
|
||||||
|
div(class = "regulator-type") { (name) }
|
||||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
In the outline items for previously implemented regulators, we use the I'm following the code organization we used for the half-curvature regulator, which is the only previously implemented single-subject regulator. In the outline items for previously implemented regulators, we use the `regulator-label` div to say which other element, if any, is subject to the regulator, and we use the `regulator-type` label to say what's being regulated. To bring the outline item for the point coordinate regulator in line with this convention, I'd rewrite it like this.
```rust
let reg_type = format!("{} coordinate", Axis::NAME[self.axis as usize]);
view! {
li(class = "regulator") {
div(class = "regulator-label") // for spacing
div(class = "regulator-type") { (reg_type) }
RegulatorInput(regulator = self)
div(class = "status")
}
}
```
I'm following the code organization we used for the half-curvature regulator, which is the only previously implemented single-subject regulator.
glen
commented
done in latest commit done in latest commit
Vectornaut
commented
The code and the resulting interface both look like what I expect, so I'd say this is resolved. The code and the resulting interface both look like what I expect, so I'd say this is resolved.
|
|||||||
|
RegulatorInput(regulator = self)
|
||||||
|
div(class = "status")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// a list item that shows an element in an outline view of an assembly
|
// a list item that shows an element in an outline view of an assembly
|
||||||
#[component(inline_props)]
|
#[component(inline_props)]
|
||||||
fn ElementOutlineItem(element: Rc<dyn Element>) -> View {
|
fn ElementOutlineItem(element: Rc<dyn Element>) -> View {
|
||||||
|
|
|
||||||
|
|
@ -52,8 +52,8 @@ pub fn project_point_to_normalized(rep: &mut DVector<f64>) {
|
||||||
// --- partial matrices ---
|
// --- partial matrices ---
|
||||||
|
|
||||||
pub struct MatrixEntry {
|
pub struct MatrixEntry {
|
||||||
index: (usize, usize),
|
pub index: (usize, usize),
|
||||||
value: f64,
|
pub value: f64,
|
||||||
}
|
}
|
||||||
|
|
||||||
pub struct PartialMatrix(Vec<MatrixEntry>);
|
pub struct PartialMatrix(Vec<MatrixEntry>);
|
||||||
|
|
|
||||||
I like the way you've added explicit verbs to these bullet points!
Just tryin' to keep things parallel :)