glen/dyna3
1
0
Fork 0
Code Issues 11 Pull Requests 1 Actions Projects Releases 2 Wiki Activity

Integrate engine into application prototype #15

Merged
glen merged 24 commits from engine-integration into main 2024-11-12 00:46:16 +00:00
Conversation 15 Commits 24 Files Changed 14 +10 -6
3 changed files with 10 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ced001bbfe - Show all commits

10
app-proto/src/assembly.rs
View File

@ -7,13 +7,17 @@ use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */
use crate::engine::{realize_gram, PartialMatrix}; use crate::engine::{realize_gram, PartialMatrix};
// the types of the keys we use to access an assembly's elements and constraints
pub type ElementKey = usize;
pub type ConstraintKey = usize;
#[derive(Clone, PartialEq)] #[derive(Clone, PartialEq)]
glen marked this conversation as resolved Outdated
glen commented 2024-11-09 23:59:52 +00:00
Outdated
Review
Copy Link

Shouldn't we have at least a typedef somewhere for color representations, in case we use a different color space at some point? For example, we are almost certain to want to add an opacity level to our colors. (Certainly the final product will allow you to set the opacity of any element.)

Shouldn't we have at least a typedef somewhere for color representations, in case we use a different color space at some point? For example, we are almost certain to want to add an opacity level to our colors. (Certainly the final product will allow you to set the opacity of any element.)
👍 1
Vectornaut commented 2024-11-11 08:06:01 +00:00
Outdated
Review
Copy Link

Done, with the type alias ElementColor.

Done, with the type alias `ElementColor`.
glen commented 2024-11-11 15:54:05 +00:00
Outdated
Review
Copy Link

OK, we will use that for now; the name implies we might have a different type for the color of something else, like the axes in a view (which aren't part of the Assembly). If we do start using this type for the colors of other things, we should rename it, I think.

OK, we will use that for now; the name implies we might have a different type for the color of something else, like the axes in a view (which aren't part of the Assembly). If we do start using this type for the colors of other things, we should rename it, I think.
pub struct Element { pub struct Element {
glen marked this conversation as resolved Outdated
glen commented 2024-11-09 23:57:26 +00:00
Outdated
Review
Copy Link

Same comments go for rep here as in Constraint. Maybe it's "representative"? If your worry is that a full word will be too long, look for a shorter word, or pick a unique short acronym for this sort of thing in both places, and then clearly document what that acronym means and why you are using it. Maybe for now "document" just means "comment" until we have a decent doc system in place.

Same comments go for rep here as in Constraint. Maybe it's "representative"? If your worry is that a full word will be too long, look for a shorter word, or pick a unique short acronym for this sort of thing in both places, and then clearly document what that acronym means and why you are using it. Maybe for now "document" just means "comment" until we have a decent doc system in place.
Vectornaut commented 2024-11-11 03:29:35 +00:00
Outdated
Review
Copy Link

Same comments go for rep here as in Constraint. Maybe it's "representative"?

It's for "representation"—a vector representation of the element, acted on by a linear representation of the 3d Möbius group. When I update the PR, I'll expand the property name to representation (in commit da008fd).

> Same comments go for rep here as in Constraint. Maybe it's "representative"? It's for "representation"—a vector representation of the element, acted on by a linear representation of the 3d Möbius group. When I update the PR, I'll expand the property name to `representation` (in commit da008fd).
glen commented 2024-11-11 19:57:27 +00:00
Outdated
Review
Copy Link

One other comment: some of the stuff in the assembly is intrinsic, i.e. there is a red sphere, constrained to have radius 1, containing a point with id A labeled A_{init}, say. Other stuff is contingent: the current realization of the red sphere has such-and-such representation, and point id A is selected. (I think the selection needs to be constant and shared across all views, so may as well be in the Assembly.) There may turn out to be practical reasons to separate those types of things in some way, but I certainly think there are conceptual advantages to doing so. For now, we may just want to keep in mind to choose names that suggest the intrinsic/contingent divide. It may be that "representation" is sufficiently suggestive in this way, or perhaps a name like "realization" would be better, and "currentRealization" is even stronger in this regard. Similarly, "selection" might already feel contingent enough; "currentlySelected" certainly would. This is not a request for any particular change to the current PR, just an item to keep in mind, although if you are moved to make any changes that's OK too.

One other comment: some of the stuff in the assembly is intrinsic, i.e. there _is_ a red sphere, constrained to have radius 1, containing a point with id A labeled `A_{init}`, say. Other stuff is contingent: the current realization of the red sphere has such-and-such representation, and point id A is selected. (I think the selection needs to be constant and shared across all views, so may as well be in the Assembly.) There may turn out to be practical reasons to separate those types of things in some way, but I certainly think there are conceptual advantages to doing so. For now, we may just want to keep in mind to choose names that suggest the intrinsic/contingent divide. It may be that "representation" is sufficiently suggestive in this way, or perhaps a name like "realization" would be better, and "currentRealization" is even stronger in this regard. Similarly, "selection" might already feel contingent enough; "currentlySelected" certainly would. This is not a request for any particular change to the current PR, just an item to keep in mind, although if you are moved to make any changes that's OK too.
pub id: String, pub id: String,
pub label: String, pub label: String,
pub color: [f32; 3], pub color: [f32; 3],
pub representation: DVector<f64>, pub representation: DVector<f64>,
glen marked this conversation as resolved Outdated
glen commented 2024-11-10 00:14:32 +00:00
Outdated
Review
Copy Link

I am confused; if this is internal, why is it pub? If it is important that this property never be reflected in a view (i.e., it is not really an intrinsic property of the element, just a contingent value for programming convenience) then there should be a way to enforce that views don't see it. Does Rust have anything like C++ friends or protected or something? Ideally, anything that shouldn't be accessible in a view would not be exposed in an interface to the assembly that views can get a hold of.

Also, I am guessing that this "index" is the same value that will appear for example in the pair in a Constraint that identifies the elements that are being constrained? If so, then the same typedef should be used here, and the names should be harmonized: if you land on ElementHandle, say, then this should be handle.

And then to soften my first paragraph: maybe we are encouraging views to use these handles for efficiency? Then we should not describe them as "internal" but instead document that they are not intrinsic properties of an element and not to be relied on across a reload of an assembly, but can be used in XXX circumstances and will be invariant over YYY operations, that sort of thing.

Oh, and then I see that Assembly has an elements_by_id property. Once you've settled on names for the relevant concept, say it's an ElementHandle, that should be renamed to something like handleOfId or elementHandleOfId.

Note I have been typing camelCase and your code is using snake_case. The former is more compact, and I am quite used to it. But I am not stuck on it. We should discuss and pick a casing convention that suits us both. I give zero weight to Rust's casing conventions; as I said, the rigidity of that community does not seem helpful, and we will be writing in husht anyway.

I am confused; if this is internal, why is it `pub`? If it is important that this property never be reflected in a view (i.e., it is not really an intrinsic property of the element, just a contingent value for programming convenience) then there should be a way to enforce that views don't see it. Does Rust have anything like C++ `friend`s or `protected` or something? Ideally, anything that shouldn't be accessible in a view would not be exposed in an interface to the assembly that views can get a hold of. Also, I am guessing that this "index" is the same value that will appear for example in the pair in a Constraint that identifies the elements that are being constrained? If so, then the same typedef should be used here, and the names should be harmonized: if you land on `ElementHandle`, say, then this should be `handle`. And then to soften my first paragraph: maybe we are encouraging views to use these handles for efficiency? Then we should not describe them as "internal" but instead document that they are not intrinsic properties of an element and not to be relied on across a reload of an assembly, but can be used in XXX circumstances and will be invariant over YYY operations, that sort of thing. Oh, and then I see that Assembly has an `elements_by_id` property. Once you've settled on names for the relevant concept, say it's an `ElementHandle`, that should be renamed to something like `handleOfId` or `elementHandleOfId`. Note I have been typing camelCase and your code is using snake_case. The former is more compact, and I am quite used to it. But I am not stuck on it. We should discuss and pick a casing convention that suits us both. I give **zero** weight to Rust's casing conventions; as I said, the rigidity of that community does not seem helpful, and we will be writing in husht anyway.
👍 1
Vectornaut commented 2024-11-11 04:53:35 +00:00
Outdated
Review
Copy Link

When an assembly with n elements is sent to the engine, each element gets an index from 1 to n, which tells you which column of the configuration matrix it goes in. The index is assigned and used during the realization routine, Assembly::realize; it can change each time realize is called. I've put it in the Element structure, as the index field, because that feels like the most foolproof way to ensure consistent indexing of the configuration matrix and the Gram matrix.

if this is internal, why is it pub?

Good point—it shouldn't be. I made it public in this pull request as a kludge, because I hadn't written a constructor for Element yet. The outline cleanup pull request (#16) introduces an Element constructor, allowing us to make index private with no changes to any other code. I propose making this an issue and then addressing it in #16.

When an assembly with *n* elements is sent to the engine, each element gets an index from 1 to *n*, which tells you which column of the configuration matrix it goes in. The index is assigned and used during the realization routine, `Assembly::realize`; it can change each time `realize` is called. I've put it in the `Element` structure, as the `index` field, because that feels like the most foolproof way to ensure consistent indexing of the configuration matrix and the Gram matrix. > if this is internal, why is it `pub`? Good point—it shouldn't be. I made it public in this pull request as a kludge, because I hadn't written a constructor for `Element` yet. The outline cleanup pull request (#16) introduces an `Element` constructor, allowing us to make `index` private with no changes to any other code. I propose making this an issue and then addressing it in #16.
glen commented 2024-11-11 15:56:35 +00:00
Outdated
Review
Copy Link

Fine, given that #16 is around the corner.

Fine, given that #16 is around the corner.
pub constraints: BTreeSet<usize>, pub constraints: BTreeSet<ConstraintKey>,
// internal properties, not reflected in any view // internal properties, not reflected in any view
pub index: usize pub index: usize
@ -21,7 +25,7 @@ pub struct Element {
glen marked this conversation as resolved Outdated
glen commented 2024-11-09 23:53:39 +00:00
Outdated
Review
Copy Link

Let's get into a habit of using full words in interfaces/public property names. Looking at this, I don't know if "rep" is a representation, the number of repetitions, something that is being reported, or what... Please rename this suite of properties. Thanks!

Let's get into a habit of using full words in interfaces/public property names. Looking at this, I don't know if "rep" is a representation, the number of repetitions, something that is being reported, or what... Please rename this suite of properties. Thanks!
👍 1
Vectornaut commented 2024-11-11 08:30:32 +00:00
Outdated
Review
Copy Link

I've replaced rep with lorentz_prod in all the Constraint field names (commit 5839882). We can streamline the names later if they get too unwieldy.

I've replaced `rep` with `lorentz_prod` in all the `Constraint` field names (commit 5839882). We can streamline the names later if they get too unwieldy.
#[derive(Clone)] #[derive(Clone)]
pub struct Constraint { pub struct Constraint {
pub subjects: (usize, usize), pub subjects: (ElementKey, ElementKey),
pub rep: Signal<f64>, pub rep: Signal<f64>,
pub rep_text: Signal<String>, pub rep_text: Signal<String>,
pub rep_valid: Signal<bool>, pub rep_valid: Signal<bool>,
@ -36,7 +40,7 @@ pub struct Assembly {
pub constraints: Signal<Slab<Constraint>>, pub constraints: Signal<Slab<Constraint>>,
// indexing // indexing
pub elements_by_id: Signal<FxHashMap<String, usize>> pub elements_by_id: Signal<FxHashMap<String, ElementKey>>
} }
impl Assembly { impl Assembly {

4
app-proto/src/main.rs
View File

@ -8,14 +8,14 @@ use rustc_hash::FxHashSet;
use sycamore::prelude::*; use sycamore::prelude::*;
use add_remove::AddRemove; use add_remove::AddRemove;
use assembly::Assembly; use assembly::{Assembly, ElementKey};
use display::Display; use display::Display;
use outline::Outline; use outline::Outline;
#[derive(Clone)] #[derive(Clone)]
struct AppState { struct AppState {
assembly: Assembly, assembly: Assembly,
selection: Signal<FxHashSet<usize>> selection: Signal<FxHashSet<ElementKey>>
} }
impl AppState { impl AppState {

2
app-proto/src/outline.rs
View File

@ -150,7 +150,7 @@ pub fn Outline() -> View {
ul(class="constraints") { ul(class="constraints") {
Keyed( Keyed(
list=elt.constraints.into_iter().collect::<Vec<_>>(), list=elt.constraints.into_iter().collect::<Vec<_>>(),
view=move |c_key: usize| { view=move |c_key| {
let c_state = use_context::<AppState>(); let c_state = use_context::<AppState>();
let assembly = &c_state.assembly; let assembly = &c_state.assembly;
let cst = assembly.constraints.with(|csts| csts[c_key].clone()); let cst = assembly.constraints.with(|csts| csts[c_key].clone());