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

Dispatch normalization routines correctly #87

Merged
glen merged 4 commits from Vectornaut/dyna3:dispatch-normalization into main 2025-06-04 21:01:14 +00:00
Conversation 9 Commits 4 Files changed 3 +112 -52
Vectornaut commented 2025-05-21 01:23:30 +00:00
Member
Copy link

This pull request addresses issue #86 by correctly dispatching the routine used to normalize spheres during nudging, and it adds a test that would've detected the issue. It also makes some other changes in support of these.

This pull request should commute with #85.

Primary changes

Dispatching the normalization routine correctly

On the main branch, the routine that's supposed to normalize spheres during nudging never runs, because the test in the conditional

if elt.type_id() == TypeId::of::<Sphere>() { /* normalize */ }

always evaluates to false. On the branch to be merged, we dispatch the normalization routine in a way that actually works, and is also more idiomatic.

Testing for curvature drift

The normalization routine helps preserve the curvature of a sphere while nudging its position. The newly added assembly::curvature_drift_test confirms that the curvature of a sphere doesn't drift too much during a series of position nudges, equivalent to holding shift+S for six or seven seconds in the user interface. The test fails, as expected, if you disable sphere normalization by making Sphere::normalize_mut_rep do nothing.

Supporting changes

Dispatching

Right now, only spheres need to be normalized during nudging, because moving along a tangent vector doesn't affect the normalization of points. To simplify the semantics of the new Element::normalize_mut_rep trait method, however, I've made it do normalization in both its implementation for Sphere and its implementation for Point. This shouldn't have much of a performance cost.

Testing

Since the tests aren't built for WebAssembly, we have to replace console::log with console_log! in all of the functions used by assembly::curvature_drift_test. We'll eventually want to do this replacement everywhere.

This pull request addresses issue #86 by correctly dispatching the routine used to normalize spheres during nudging, and it adds a test that would've detected the issue. It also makes some other changes in support of these. This pull request should commute with #85. ### Primary changes #### Dispatching the normalization routine correctly On the main branch, the routine that's supposed to normalize spheres during nudging never runs, because the test in the [conditional](https://code.studioinfinity.org/StudioInfinity/dyna3/src/commit/a2478febc128b218d833bc3e904482f2055865cb/app-proto/src/assembly.rs#L715) ```rust if elt.type_id() == TypeId::of::<Sphere>() { /* normalize */ } ``` always evaluates to `false`. On the branch to be merged, we dispatch the normalization routine in a way that actually works, and is also more idiomatic. #### Testing for curvature drift The normalization routine helps preserve the curvature of a sphere while nudging its position. The newly added `assembly::curvature_drift_test` confirms that the curvature of a sphere doesn't drift too much during a series of position nudges, equivalent to holding **shift+S** for six or seven seconds in the user interface. The test fails, as expected, if you disable sphere normalization by making `Sphere::normalize_mut_rep` do nothing. ### Supporting changes #### Dispatching Right now, only spheres need to be normalized during nudging, because moving along a tangent vector doesn't affect the normalization of points. To simplify the semantics of the new `Element::normalize_mut_rep` trait method, however, I've made it do normalization in both its implementation for `Sphere` and its implementation for `Point`. This shouldn't have much of a performance cost. #### Testing Since the tests aren't built for WebAssembly, we have to replace `console::log` with `console_log!` in all of the functions used by `assembly::curvature_drift_test`. We'll eventually want to do this replacement everywhere.
Vectornaut added 2 commits 2025-05-21 01:23:31 +00:00 
This corrects the dispatching of the normalization routine for spheres,
and it adds a (perhaps redundant) normalization routine for points.
Test curvature drift during position nudging
All checks were successful
/ test (pull_request) Successful in 2m25s
Details
f1da5155a3 
The test fails, as expected, if you disable sphere normalization by
making `Sphere::normalize_mut_rep` do nothing. Since the tests aren't
built for WebAssembly, we have to replace `console::log` with
`console_log!` in all of the functions they use. We'll eventually want
to do this replacement everywhere.
glen requested changes 2025-05-30 00:28:27 +00:00
glen left a comment
Owner
Copy link

Just a couple of superficial items, otherwise looks good

Just a couple of superficial items, otherwise looks good
app-proto/Cargo.toml Outdated
@ -47,2 +47,4 @@
wasm-bindgen-test = "0.3.34"
[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ["cfg(sycamore_force_ssr)"] }
glen commented 2025-05-30 00:22:16 +00:00
Owner
Copy link

Do Cargo.toml files allow comments? If so, please comment the meaning/purpose of this new block, it's a bit inscrutable. Something like "previously we used server-side rendering, but we switched to BLAH because BLEE, so warn if SSR is turned on". Of course I may totally have misunderstood what's going on here, that's just meant as an example comment.

Do Cargo.toml files allow comments? If so, please comment the meaning/purpose of this new block, it's a bit inscrutable. Something like "previously we used server-side rendering, but we switched to BLAH because BLEE, so warn if SSR is turned on". Of course I may totally have misunderstood what's going on here, that's just meant as an example comment.
Vectornaut commented 2025-05-30 00:53:57 +00:00
Author
Member
Copy link

Done in commit e19792d (previously 8732cb7)!

Done in commit e19792d (previously 8732cb7)!
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -106,6 +107,9 @@ pub trait Element: Serial + ProblemPoser + DisplayItem {
// element is responsible for keeping this set up to date
fn regulators(&self) -> Signal<BTreeSet<Rc<dyn Regulator>>>;
// normalize a representation vector for this kind of element
glen commented 2025-05-30 00:26:00 +00:00
Owner
Copy link

Do you like the word "normalize" or "canonicalize"? I feel like the latter more explicitly means "choose the favored representative of an equivalence class" (which I think is mainly what's going on here) and the former can have broader meanings in which the entity is actually changed, like when you find a unit vector in the same direction -- I think that's sometimes called normalization. Only make a change if you feel like "canonicalize" would be a better/clearer choice.

Do you like the word "normalize" or "canonicalize"? I feel like the latter more explicitly means "choose the favored representative of an equivalence class" (which I think is mainly what's going on here) and the former can have broader meanings in which the entity is actually changed, like when you find a unit vector in the same direction -- I think that's sometimes called normalization. Only make a change if you feel like "canonicalize" would be a better/clearer choice.
Vectornaut commented 2025-05-30 00:46:35 +00:00
Author
Member
Copy link

In my mind, an element is represented by a line through the origin in \mathbb{R}^{4,1}, so choosing a favored representative is equivalent to choosing a particular scaling. That's why I used the word "normalization." However, the implementation of normalize_rep_mut for spheres actually changes the equivalence class. That's because the only thing we use normalize_rep_mut for right now is to keep elements normalized during nudging, and normalizing in a way that preserves the equivalence class makes nudging less stable. If both "normalize" and "canonicalize" imply staying within an equivalence class for you, then maybe we need another term entirely.

In my mind, an element is represented by a line through the origin in $\mathbb{R}^{4,1}$, so choosing a favored representative is equivalent to choosing a particular scaling. That's why I used the word "normalization." However, the implementation of `normalize_rep_mut` for spheres actually changes the equivalence class. That's because the only thing we use `normalize_rep_mut` for right now is to keep elements normalized during nudging, and normalizing in a way that preserves the equivalence class makes nudging less stable. If both "normalize" and "canonicalize" imply staying within an equivalence class for you, then maybe we need another term entirely.
glen commented 2025-05-30 20:47:03 +00:00
Owner
Copy link

Ah, hmm, I hadn't appreciated that the result of normalize_rep_mut might be in a different equivalence class. "Normalize" does not by itself connote "stay in the same equivalence class, but "normalize representation" does seem to imply strongly that we are just choosing a different representation of the same entity. So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity. If you don't like that, perhaps "stabilize" or "standardize" or "choose_canonical" or "nearby_canonical" or something?

Ah, hmm, I hadn't appreciated that the result of normalize_rep_mut might be in a different equivalence class. "Normalize" does not by itself connote "stay in the same equivalence class, but "normalize representation" does seem to imply strongly that we are just choosing a different representation of the same entity. So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity. If you don't like that, perhaps "stabilize" or "standardize" or "choose_canonical" or "nearby_canonical" or something?
Vectornaut commented 2025-05-30 21:19:52 +00:00
Author
Member
Copy link

How about something like proj_to_normalized, to communicate that we're projecting the given representation vector to the normalized configuration variety for the given element type?

So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity.

The rep isn't working as intended in the name normalize_rep_mut anyway. I added it to emphasize that this method acts on the representation vector provided as an argument, not the vector representing the element the method is called from. This makes it different from the normalize_mut methods for nalgebra matrices.

(Using an associated function instead of a method would be the best way to clarify this, but I don't think it's possible to call associated functions from trait objects. This complaint about the limitation and this unsuccessful attempt to lift the limitation seem to demonstrate that it exists, or at least used to exist. If you want, though, I can try again to use an associated function and look at exactly what compiler error it produced.)

How about something like `proj_to_normalized`, to communicate that we're projecting the given representation vector to the normalized configuration variety for the given element type? > So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity. The `rep` isn't working as intended in the name `normalize_rep_mut` anyway. I added it to emphasize that this method acts on the representation vector provided as an argument, not the vector representing the element the method is called from. This makes it different from the [`normalize_mut`](https://docs.rs/nalgebra/latest/nalgebra/base/type.DMatrix.html#method.normalize_mut) methods for nalgebra matrices. (Using an associated function instead of a method would be the best way to clarify this, but I don't think it's possible to call associated functions from trait objects. [This complaint](https://users.rust-lang.org/t/trait-object-safety-still-confuses-me/39699/4) about the limitation and [this unsuccessful attempt](https://github.com/rust-lang/rfcs/pull/2886) to lift the limitation seem to demonstrate that it exists, or at least used to exist. If you want, though, I can try again to use an associated function and look at exactly what compiler error it produced.)
glen commented 2025-06-01 02:50:17 +00:00
Owner
Copy link

Sure, any of project_to_canonical or project_to_normal or project_to_normalized or canonical_projection or normal_projection would be fine, whatever seems simplest/clearest to you. I'd suggest that with a name that's already this long, there's no point in abbreviating with proj -- just use whatever full word proj would be an abbreviation for. Using a method is fine, don't worry about the associated function thing.

Sure, any of `project_to_canonical` or `project_to_normal` or `project_to_normalized` or `canonical_projection` or `normal_projection` would be fine, whatever seems simplest/clearest to you. I'd suggest that with a name that's already this long, there's no point in abbreviating with `proj` -- just use whatever full word `proj` would be an abbreviation for. Using a method is fine, don't worry about the associated function thing.
Vectornaut commented 2025-06-01 17:52:21 +00:00
Author
Member
Copy link

I just pushed a commit with better names (54b34e0582). My look-over before pushing was a bit cursory; I'll look at it harder later today.

I just pushed a commit with better names (`54b34e0582`). My look-over before pushing was a bit cursory; I'll look at it harder later today.
Vectornaut commented 2025-06-01 23:11:04 +00:00
Author
Member
Copy link

I've looked at the renaming commit again and I still think it's fine, so let me know what you think!

I've looked at the renaming commit again and I still think it's fine, so let me know what you think!
glen marked this conversation as resolved
Vectornaut added 1 commit 2025-05-30 00:53:15 +00:00
Explain the new check-cfg lint
All checks were successful
/ test (pull_request) Successful in 2m26s
Details
8732cb77c2
Vectornaut added 1 commit 2025-06-01 17:50:14 +00:00
Improve the naming of the normalization methods
All checks were successful
/ test (pull_request) Successful in 2m26s
Details
54b34e0582
glen commented 2025-06-02 16:01:47 +00:00
Owner
Copy link

The branch is out-of-date. I would just click "update branch by rebase" but I am not sure if you want me rebasing your branches, so please bring it up-to-date as you see fit (and let me know if you are OK with my hitting the "update branch by rebase" button in the future).

The branch is out-of-date. I would just click "update branch by rebase" but I am not sure if you want me rebasing your branches, so please bring it up-to-date as you see fit (and let me know if you are OK with my hitting the "update branch by rebase" button in the future).
glen commented 2025-06-02 16:04:13 +00:00
Owner
Copy link

I am happy with the names but it seems cumbersome that there are methods project_XXX_to_normalized and then two boilerplate implementations of project_to_normalized that just forward to the project_XXX versions. Could this method be refactored so that the code for project_to_normalized for each element type is just in one place where it makes the most sense to be located, and avoid this verbose forwarding pattern?

I am happy with the names but it seems cumbersome that there are methods project_XXX_to_normalized and then two boilerplate implementations of project_to_normalized that just forward to the project_XXX versions. Could this method be refactored so that the code for project_to_normalized for each element type is just in one place where it makes the most sense to be located, and avoid this verbose forwarding pattern?
Vectornaut force-pushed dispatch-normalization from 54b34e0582 to f332f755e0 2025-06-02 21:49:52 +00:00 Compare
Vectornaut commented 2025-06-02 21:56:50 +00:00
Author
Member
Copy link

The branch is out-of-date. I would just click "update branch by rebase" but I am not sure if you want me rebasing your branches, so please bring it up-to-date as you see fit (and let me know if you are OK with my hitting the "update branch by rebase" button in the future).

I think asking me before hitting the "update branch by rebase" button is a good policy. In this case, I appreciated the chance to look over and test a local rebase before force-pushing the updated branch to the server. I've now force-pushed, as shown above.

> The branch is out-of-date. I would just click "update branch by rebase" but I am not sure if you want me rebasing your branches, so please bring it up-to-date as you see fit (and let me know if you are OK with my hitting the "update branch by rebase" button in the future). I think asking me before hitting the "update branch by rebase" button is a good policy. In this case, I appreciated the chance to look over and test a local rebase before force-pushing the updated branch to the server. I've now force-pushed, as shown above.
Vectornaut commented 2025-06-02 22:04:28 +00:00
Author
Member
Copy link

I am happy with the names but it seems cumbersome that there are methods project_XXX_to_normalized and then two boilerplate implementations of project_to_normalized that just forward to the project_XXX versions. Could this method be refactored so that the code for project_to_normalized for each element type is just in one place where it makes the most sense to be located, and avoid this verbose forwarding pattern?

Yes, I think this can profitably be cleaned up now. For now, I'll try to do it with small-scale changes. We may want to do a deeper refactoring later, especially if we keep using this pattern.

> I am happy with the names but it seems cumbersome that there are methods project_XXX_to_normalized and then two boilerplate implementations of project_to_normalized that just forward to the project_XXX versions. Could this method be refactored so that the code for project_to_normalized for each element type is just in one place where it makes the most sense to be located, and avoid this verbose forwarding pattern? Yes, I think this can profitably be cleaned up now. For now, I'll try to do it with small-scale changes. We may want to do a deeper refactoring later, especially if we keep using this pattern.
Vectornaut commented 2025-06-02 23:24:36 +00:00
Author
Member
Copy link

My first attempt to avoid forwarding the project_to_normalized implementations was:

  • Make project_to_normalized a method of a new engine::EngineElement trait.
  • Make engine::EngineElement a supertrait of assembly::Element.
  • Implement engine::EngineElement for each element within the element module.

We've already used this pattern with traits like outline::OutlineItem and display::DisplayItem, and I like the way it works in general. However, there's an obstacle to using it in this case: the standalone dyna3 library target we set up in pull request #24. The pattern we've been using would make the element module depend on the assembly module, which is left out of the standalone library target because it depends in turn on many parts of the interface.

Possible workarounds

  1. Move the implementations of engine::EngineElement into the assembly module. However, this would leak engine-specific code out of the engine module, which we've been trying to avoid.
  2. Put the EngineElement trait and its implementations in a new intermediate module, called something like engine-bindings, which is allowed to contain engine-specific code but is left out of the standalone library target.
My first attempt to avoid forwarding the `project_to_normalized` implementations was: - Make `project_to_normalized` a method of a new `engine::EngineElement` trait. - Make `engine::EngineElement` a supertrait of `assembly::Element`. - Implement `engine::EngineElement` for each element within the `element` module. We've already used this pattern with traits like `outline::OutlineItem` and `display::DisplayItem`, and I like the way it works in general. However, there's an obstacle to using it in this case: the standalone dyna3 library target we set up in pull request #24. The pattern we've been using would make the `element` module depend on the `assembly` module, which is left out of the standalone library target because it depends in turn on many parts of the interface. #### Possible workarounds 1. Move the implementations of `engine::EngineElement` into the `assembly` module. However, this would leak engine-specific code out of the `engine` module, which we've been trying to avoid. 2. Put the `EngineElement` trait and its implementations in a new intermediate module, called something like `engine-bindings`, which is allowed to contain engine-specific code but is left out of the standalone library target.
Vectornaut commented 2025-06-03 00:27:15 +00:00
Author
Member
Copy link

During our meeting, we decided that cleaning up the forwarded project_to_normalized implementations will best be done in a dedicated pull request that encapsulates element representation data types within the engine module, so the assembly module no longer has to know that each element is represented by a DVector<f64>. I'll file an issue for that later today.

During our meeting, we decided that cleaning up the forwarded `project_to_normalized` implementations will best be done in a dedicated pull request that encapsulates element representation data types within the `engine` module, so the `assembly` module no longer has to know that each element is represented by a `DVector<f64>`. I'll file an issue for that later today.
glen commented 2025-06-04 20:59:42 +00:00
Owner
Copy link

Tests pass, nudging around by hand looks good, and we are deferring the refactoring (don't forget to file that issue), so merging.

Tests pass, nudging around by hand looks good, and we are deferring the refactoring (don't forget to file that issue), so merging.
👍 1
glen merged commit e447e7ea96 into main 2025-06-04 21:01:14 +00:00
Vectornaut commented 2025-06-04 22:48:49 +00:00
Author
Member
Copy link

don't forget to file that issue

Whoops, thanks for the reminder! This is now issue #90.

> don't forget to file that issue Whoops, thanks for the reminder! This is now issue #90.
Sign in to join this conversation.
Reviewers
No reviewers
glen
Labels
Clear labels   
bug

Something is not working

  
design

Issues that entail architecture/design decisions

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
maintenance

refactors andother behavior-orthogonal tasks

  
prospective

this becomes an issue once a particular feature is implemented or pull request is merged

  
question

More information is needed

  
regression

A bug that's resurfaced or a feature that's accidentally been removed

  
stub

We need to fill in the details of this issue

  
todo

Planned step in implementation

  
ui

issue concerning the user interface

  
wontfix

This won't be fixed

No labels
bug
design
duplicate
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: StudioInfinity/dyna3#87
No description provided.
Delete branch "Vectornaut/dyna3:dispatch-normalization"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?