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

Curvature regulators #80

Merged
glen merged 21 commits from Vectornaut/dyna3:curvature-regulators into main 2025-04-21 23:40:43 +00:00
Conversation 22 Commits 21 Files changed 6 +640 -331
Vectornaut commented 2025-04-09 23:00:04 +00:00
Member
Copy link

Summary

On the main branch, there's only one kind of regulator: the one that regulates the inversive distance between two spheres (or, more generally, the Lorentz product between two element representation vectors). The branch to be merged adds a new kind of regulator, which regulates the curvature of a sphere (issue #55). In the process, it introduces a general framework for organizing and sharing code between different kinds of regulators.

Organization

Regulators

The Regulator trait

Each kind of regulator must implement the assembly::Regulator trait. Its methods provide access to basic information about a regulator, and its supertraits provide all the other functionality a regulator needs. The branch to be merged has two Regulator implementations: ProductRegulator and HalfCurvatureRegulator.

Right now, I'm imagining that Regulator will have one supertrait for each module a regulator needs to communicate with:

  • The assembly::ProblemPoser trait, described below, provides for communication with the engine.
  • The outline::OutlineItem trait provides for display in the outline view.

I think a communication trait should typically be defined and implemented in the module it provides communication with. I've made an exception for ProblemPoser, which provides communication with the engine, but is housed in the assembly module. That's because ProblemPoser seems especially tightly integrated with the design of elements and regulators.

The OutlineItem trait

In the future, I'd like each view to come with a trait to be implemented by each part of the model that the view exposes. The branch to be merged adopts this approach for displaying regulators in the outline view.

Constraint problems

The ConstraintProblem structure

The specification of a constraint problem is encapsulated in the engine::ConstraintProblem structure. To send a problem to the engine, we create a new ConstraintProblem, write the constraints and the initial guess into it, and then pass it to engine::realize_gram.

I'm thinking of this structure as the engine's native problem format. If we switch to a new engine, we'll create a new ConstraintProblem structure tailored to it.

The ProblemPoser trait

Each object that needs to be represented in a constraint problem is given the assembly::ProblemPoser trait. When we want to put together a constraint problem, we call on each problem poser to specify its own part of the problem using the pose method. In the branch to be merged, there are three kinds of problem posers:

  • A Lorentz product regulator (ProductRegulator), which fixes the Lorentz product between two element representation vectors by setting an off-diagonal entry of the Gram matrix.
  • A half-curvature regulator (HalfCurvatureRegulator), which freezes the curvature entry of a sphere's representation vector.
  • A sphere (currently called Element, because it's the only kind of element so far), which:
    • Poses an initial guess for its representation vector.
    • Fixes the self-product of the representation vector at 1 by setting a diagonal entry of the Gram matrix.

I'm thinking of this trait as the interface between the assembly and the engine. If we switch to a new engine, we'll port each problem poser's pose method to the new problem format. We may also want to adjust to the problem posers' fields and other methods to bring them conceptually closer to the new engine, but the pose method is where the rubber will meet the road.

Future directions

  • When we introduce point elements (issue #27), we'll change Element from a structure that implements ProblemPoser to a trait that has ProblemPoser as a supertrait, just like this pull request changes Regulator from a structure to a trait.
  • For consistency, we should add a trait that provides for communication with the 3d display view, just as OutlineItem provides for communication with the outline view.
  • It would be nice to add a structure that encapsulates a constraint satisfier returned from the engine, just as the ConstraintProblem structure encapsulates a constraint problem passed into the engine.
## Summary On the main branch, there's only one kind of regulator: the one that regulates the inversive distance between two spheres (or, more generally, the Lorentz product between two element representation vectors). The branch to be merged adds a new kind of regulator, which regulates the curvature of a sphere (issue #55). In the process, it introduces a general framework for organizing and sharing code between different kinds of regulators. ## Organization ### Regulators #### The `Regulator` trait Each kind of regulator must implement the `assembly::Regulator` trait. Its methods provide access to basic information about a regulator, and its supertraits provide all the other functionality a regulator needs. The branch to be merged has two `Regulator` implementations: `ProductRegulator` and `HalfCurvatureRegulator`. Right now, I'm imagining that `Regulator` will have one supertrait for each module a regulator needs to communicate with: - The `assembly::ProblemPoser` trait, described below, provides for communication with the engine. - The `outline::OutlineItem` trait provides for display in the outline view. I think a communication trait should typically be defined and implemented in the module it provides communication with. I've made an exception for `ProblemPoser`, which provides communication with the engine, but is housed in the `assembly` module. That's because `ProblemPoser` seems especially tightly integrated with the design of elements and regulators. #### The `OutlineItem` trait In the future, I'd like each view to come with a trait to be implemented by each part of the model that the view exposes. The branch to be merged adopts this approach for displaying regulators in the outline view. ### Constraint problems #### The `ConstraintProblem` structure The specification of a constraint problem is encapsulated in the `engine::ConstraintProblem` structure. To send a problem to the engine, we create a new `ConstraintProblem`, write the constraints and the initial guess into it, and then pass it to `engine::realize_gram`. I'm thinking of this structure as the engine's native problem format. If we switch to a new engine, we'll create a new `ConstraintProblem` structure tailored to it. #### The `ProblemPoser` trait Each object that needs to be represented in a constraint problem is given the `assembly::ProblemPoser` trait. When we want to put together a constraint problem, we call on each problem poser to specify its own part of the problem using the `pose` method. In the branch to be merged, there are three kinds of problem posers: - A Lorentz product regulator (`ProductRegulator`), which fixes the Lorentz product between two element representation vectors by setting an off-diagonal entry of the Gram matrix. - A half-curvature regulator (`HalfCurvatureRegulator`), which freezes the curvature entry of a sphere's representation vector. - A sphere (currently called `Element`, because it's the only kind of element so far), which: - Poses an initial guess for its representation vector. - Fixes the self-product of the representation vector at $1$ by setting a diagonal entry of the Gram matrix. I'm thinking of this trait as the interface between the assembly and the engine. If we switch to a new engine, we'll port each problem poser's `pose` method to the new problem format. We may also want to adjust to the problem posers' fields and other methods to bring them conceptually closer to the new engine, but the `pose` method is where the rubber will meet the road. ## Future directions - When we introduce point elements (issue #27), we'll change `Element` from a structure that implements `ProblemPoser` to a trait that has `ProblemPoser` as a supertrait, just like this pull request changes `Regulator` from a structure to a trait. - For consistency, we should add a trait that provides for communication with the 3d display view, just as `OutlineItem` provides for communication with the outline view. - It would be nice to add a structure that encapsulates a constraint satisfier returned from the engine, just as the `ConstraintProblem` structure encapsulates a constraint problem passed into the engine.
Vectornaut added 9 commits 2025-04-09 23:00:05 +00:00 
This will make it easier for elements and regulators to write themselves
into the constraint problem.
When we realize an assembly, each element and regulator now writes its
own data into the constraint problem.
This lets us iterate over subjects. Based on commit 257ce33, with a few
updates from 4a9e777.
This will provide a common interface for Lorentz product regulators,
curvature regulators, and hopefully all the other regulators too.
Thanks, Clippy!
Before, a `ConstraintProblem` only specified the indices of the frozen
entries. During realization, the frozen entries kept whatever values
they had in the initial guess.

This commit adds the values of the frozen entries to the `frozen` field
of `ConstraintProblem`. The frozen entries of the guess are set to the
desired values at the beginning of realization.

This commit also improves the `PartialMatrix` structure, which is used
to specify the indices and values of the frozen entries.
In the process, add the `OutlineItem` trait so that each regulator can
implement its own outline item view.
Elements and regulators use this common interface to write their data
into the constraint problem.
Give every sphere a curvature regulator
All checks were successful
/ test (pull_request) Successful in 2m24s
Details
81e423fcbe 
In the process, fix a reactivity bug by removing unintended signal
tracking from `insert_regulator`.
Vectornaut commented 2025-04-09 23:04:05 +00:00
Author
Member
Copy link

I've marked this PR as a work in progress because the example switcher is broken as of commit 81e423f. You're welcome to start reviewing while I debug the example switcher, because I expect the bug to be fixable without big changes.

I've marked this PR as a work in progress because the example switcher is broken as of commit 81e423f. You're welcome to start reviewing while I debug the example switcher, because I expect the bug to be fixable without big changes.
Vectornaut added 1 commit 2025-04-10 19:16:56 +00:00
Clear the regulator list when switching examples
All checks were successful
/ test (pull_request) Successful in 2m21s
Details
e1952d7d52
Vectornaut changed title from WIP: Curvature regulators to Curvature regulators 2025-04-10 19:18:37 +00:00
Vectornaut commented 2025-04-10 19:21:25 +00:00
Author
Member
Copy link

I've fixed the example switcher regression and removed the work-in-progress flag! (The example switcher is now equally buggy on the main branch and on the branch to be merged. I'm not sure whether it's worth filing an issue for, since the example switcher will hopefully soon be replaced with something more durable.)

I've fixed the example switcher regression and removed the work-in-progress flag! (The example switcher is now equally buggy on the main branch and on the branch to be merged. I'm not sure whether it's worth filing an issue for, since the example switcher will hopefully soon be replaced with something more durable.)
glen commented 2025-04-10 22:50:30 +00:00
Owner
Copy link

@Vectornaut wrote in #80 (comment):

I'm thinking of this structure as the engine's native problem format. If we switch to a new engine, we'll create a new ConstraintProblem structure tailored to it.

Let's not worry about how that will go until we decide it's worth crossing that bridge. We might reasonably want to have our own "GenericConstraintProblem" specification, and have our elements write in that specification, and then to switch engines, "all" we have to do is make a translation from our "GenericConstraintProblem" to the native format of that new engine. But it's hard to envision what that GenericConstraintProblem might be like until we have two different native formats to compare. The hypothesis here is that there might be a smaller number of primitive "problem aspects" that a more numerous collection of elements and regulators can specify themselves in terms of, creating less translation work when switching engines if you only need to worry about how to capture those primitive problem elements. I'm really not sure. It also seems to me that Elements are ProblemPosers as well, but it's fine if that's not reflected in the software structure in this PR -- but you may want to contemplate that for #27. Otherwise, no action item here. Oh, I see you are already doing it that way. Great.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/80#issue-496: > I'm thinking of this structure as the engine's native problem format. If we switch to a new engine, we'll create a new `ConstraintProblem` structure tailored to it. Let's not worry about how that will go until we decide it's worth crossing that bridge. We might reasonably want to have our own "GenericConstraintProblem" specification, and have our elements write in that specification, and then to switch engines, "all" we have to do is make a translation from our "GenericConstraintProblem" to the native format of that new engine. But it's hard to envision what that GenericConstraintProblem might be like until we have two different native formats to compare. The hypothesis here is that there might be a smaller number of primitive "problem aspects" that a more numerous collection of elements and regulators can specify themselves in terms of, creating less translation work when switching engines if you only need to worry about how to capture those primitive problem elements. I'm really not sure. ~~It also seems to me that Elements are ProblemPosers as well, but it's fine if that's not reflected in the software structure in this PR -- but you may want to contemplate that for #27. Otherwise, no action item here.~~ Oh, I see you are already doing it that way. Great.
glen commented 2025-04-10 22:54:50 +00:00
Owner
Copy link

@Vectornaut wrote in #80 (comment):

We may also want to adjust to the problem posers' fields and other methods to bring them conceptually closer to the new engine,

Or conversely, switching engines might make us want to make the problem posers' internal data more intrinsic to the underlying mathematical structure of that poser, and hence actually farther from both the old and new engine problem specs, to make them more independent and invariant as we change engines... just a matter of seeing how it works out. Again, nothing to worry about at the moment.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/80#issue-496: > We may also want to adjust to the problem posers' fields and other methods to bring them conceptually closer to the new engine, Or conversely, switching engines might make us want to make the problem posers' internal data more intrinsic to the underlying mathematical structure of that poser, and hence actually farther from both the old and new engine problem specs, to make them more independent and invariant as we change engines... just a matter of seeing how it works out. Again, nothing to worry about at the moment.
Vectornaut commented 2025-04-10 22:54:56 +00:00
Author
Member
Copy link

It also seems to me that Elements are ProblemPosers as well, but it's fine if that's not reflected in the software structure in this PR -- but you may want to contemplate that for #27.

Yes, the future Element trait should have ProblemPoser as a supertrait. I've updated the "Future directions" section to mention this.

> It also seems to me that Elements are ProblemPosers as well, but it's fine if that's not reflected in the software structure in this PR -- but you may want to contemplate that for #27. Yes, the future `Element` trait should have `ProblemPoser` as a supertrait. I've updated the "Future directions" section to mention this.
glen reviewed 2025-04-10 23:31:53 +00:00
app-proto/src/assembly.rs Outdated
@ -264,0 +393,4 @@
// update the realization when the regulator becomes a constraint, or is
// edited while acting as a constraint
let self_for_effect = self.clone();
create_effect(move || {
glen commented 2025-04-10 23:31:53 +00:00
Owner
Copy link

This function evokes a few questions:

A) Seems like there is some duplication at least of structure/behavior here with the insert_new_product_regulator; is there anything that can be profitable factored out (some common insert_regulator common functionality that both of these can use)?

B) Why do you have to do so much work updating the guess (I think that's what's going on) when you start regulating curvature, but it seems like you don't do much of anything when you start regulating an inversive distance?

C) This very much has the look of engine code that has leaked into the assembly: nitty gritty dealing with the internal coordinates of an element. Any chance it could be implemented in the engine and just called here, or perhaps even just deferred until a pre-processing step when the realization is called? Either way, it also might make the code between insert curvature regulator and insert product regulator more similar and easier to share between the two.

This function evokes a few questions: A) Seems like there is some duplication at least of structure/behavior here with the insert_new_product_regulator; is there anything that can be profitable factored out (some common insert_regulator common functionality that both of these can use)? B) Why do you have to do so much work updating the guess (I think that's what's going on) when you start regulating curvature, but it seems like you don't do much of anything when you start regulating an inversive distance? C) This very much has the look of engine code that has leaked into the assembly: nitty gritty dealing with the internal coordinates of an element. Any chance it could be implemented in the engine and just called here, or perhaps even just deferred until a pre-processing step when the realization is called? Either way, it also might make the code between insert curvature regulator and insert product regulator more similar and easier to share between the two.
Vectornaut commented 2025-04-14 19:25:02 +00:00
Author
Member
Copy link

A) Seems like there is some duplication at least of structure/behavior here with the insert_new_product_regulator; is there anything that can be profitable factored out (some common insert_regulator common functionality that both of these can use)?

I agree that the code is organized similarly, but I haven't found anything we can usefully factor out. For example, I don't see a way to put the shared organization in a shared constructor, because ProductRegulator and HalfCurvatureRegulator are different structures. The only thing that seems really straightforwardly shared is initializing the set point to

create_signal(SpecifiedValue::from_empty_spec()),

which seems like too little code to factor out profitably.

> A) Seems like there is some duplication at least of structure/behavior here with the insert_new_product_regulator; is there anything that can be profitable factored out (some common insert_regulator common functionality that both of these can use)? I agree that the code is organized similarly, but I haven't found anything we can usefully factor out. For example, I don't see a way to put the shared organization in a shared constructor, because `ProductRegulator` and `HalfCurvatureRegulator` are different structures. The only thing that seems really straightforwardly shared is initializing the set point to ```create_signal(SpecifiedValue::from_empty_spec())```, which seems like too little code to factor out profitably.
Vectornaut commented 2025-04-14 20:07:38 +00:00
Author
Member
Copy link

B) Why do you have to do so much work updating the guess (I think that's what's going on) when you start regulating curvature, but it seems like you don't do much of anything when you start regulating an inversive distance?

We don't have to be so careful about updating the guess. My first prototype of the curvature regulator didn't do anything special when it started regulating; the curvature component of the representation vector would just get overwritten with the desired value at the beginning of the engine's realization routine. That seemed to work fine in the examples I played with.

I felt compelled to be careful about updating the guess for three related reasons:

  1. Just overwriting the curvature component of the representation vector messes up the vector's normalization. I worried that starting from a non-normalized guess could make the engine more likely to stall, and could make the assembly change especially erratically when the curvature regulator is turned on.
  2. I could imagine using a curvature regulator to make extreme curvature changes. For example, someone who's doing conformal geometry on the boundary of hyperbolic 3-space might switch between the Poincaré model and the upper half-space model by switching the curvature of the boundary sphere between 1 and 0. I worried that if we just took whatever realization the engine happened to find, extreme curvature changes might throw the sphere far outside the field of view.
  3. For a moderately curved sphere with nothing constrained except its curvature, one might expect that changing the curvature shouldn't affect the center much. I wanted to demonstrate that we could meet this expectation, in case you thought it was important.

I'd be open to also choosing the initial guess more carefully when we start regulating an inversive distance. That seems tricker to me, though, and I haven't felt a need for it.

> B) Why do you have to do so much work updating the guess (I think that's what's going on) when you start regulating curvature, but it seems like you don't do much of anything when you start regulating an inversive distance? We don't *have* to be so careful about updating the guess. My first prototype of the curvature regulator didn't do anything special when it started regulating; the curvature component of the representation vector would just get overwritten with the desired value at the beginning of the engine's realization routine. That seemed to work fine in the examples I played with. I felt compelled to be careful about updating the guess for three related reasons: 1. Just overwriting the curvature component of the representation vector messes up the vector's normalization. I worried that starting from a non-normalized guess could make the engine more likely to stall, and could make the assembly change especially erratically when the curvature regulator is turned on. 2. I could imagine using a curvature regulator to make extreme curvature changes. For example, someone who's doing conformal geometry on the boundary of hyperbolic 3-space might switch between the Poincaré model and the upper half-space model by switching the curvature of the boundary sphere between 1 and 0. I worried that if we just took whatever realization the engine happened to find, extreme curvature changes might throw the sphere far outside the field of view. 3. For a moderately curved sphere with nothing constrained except its curvature, one might expect that changing the curvature shouldn't affect the center much. I wanted to demonstrate that we could meet this expectation, in case you thought it was important. I'd be open to also choosing the initial guess more carefully when we start regulating an inversive distance. That seems tricker to me, though, and I haven't felt a need for it.
Vectornaut commented 2025-04-14 20:26:58 +00:00
Author
Member
Copy link

C) This very much has the look of engine code that has leaked into the assembly: nitty gritty dealing with the internal coordinates of an element.

Yes, I think this code is on the border between the assembly module's responsibilities and the engine's responsibilities. I decided to put it in the assembly module because its core purpose is to decide how an otherwise unconstrained sphere should behave when you change its curvature. That seems more like user interaction than like constraint solving to me.

On the other hand, the engine does provide representation-specific implementations of other user-facing, mostly-representation-agnostic tasks, like creating a sphere from a center and a radius (sphere) or a direction, an offset, and a curvature (sphere_with_offset). If we can find a mostly-representation-agnostic way to describe how we update the curvature, maybe it would feel similar in spirit to those tasks.

> C) This very much has the look of engine code that has leaked into the assembly: nitty gritty dealing with the internal coordinates of an element. Yes, I think this code is on the border between the assembly module's responsibilities and the engine's responsibilities. I decided to put it in the assembly module because its core purpose is to decide how an otherwise unconstrained sphere should behave when you change its curvature. That seems more like user interaction than like constraint solving to me. On the other hand, the engine does provide representation-specific implementations of other user-facing, mostly-representation-agnostic tasks, like creating a sphere from a center and a radius (`sphere`) or a direction, an offset, and a curvature (`sphere_with_offset`). If we can find a mostly-representation-agnostic way to describe how we update the curvature, maybe it would feel similar in spirit to those tasks.
Vectornaut commented 2025-04-16 06:59:41 +00:00
Author
Member
Copy link

On the other hand, the engine does provide representation-specific implementations of other user-facing, mostly-representation-agnostic tasks […]. If we can find a mostly-representation-agnostic way to describe how we update the curvature, maybe it would feel similar in spirit to those tasks.

I've adopted this approach in commit 4654bf0 by moving the half-curvature change routine into the engine module. I haven't come up with a representation-agnostic description of how the sphere is supposed to change, so I'm just calling the routine change_half_curvature for now.

> On the other hand, the engine does provide representation-specific implementations of other user-facing, mostly-representation-agnostic tasks […]. If we can find a mostly-representation-agnostic way to describe how we update the curvature, maybe it would feel similar in spirit to those tasks. I've adopted this approach in commit 4654bf0 by moving the half-curvature change routine into the engine module. I haven't come up with a representation-agnostic description of how the sphere is supposed to change, so I'm just calling the routine `change_half_curvature` for now.
glen marked this conversation as resolved
glen reviewed 2025-04-10 23:35:06 +00:00
app-proto/src/assembly.rs Outdated
@ -281,1 +450,3 @@
let mut gram_to_be = PartialMatrix::new();
// set up the constraint problem
let problem = self.elements.with_untracked(|elts| {
let mut problem_to_be = ConstraintProblem::new(elts.len());
glen commented 2025-04-10 23:35:06 +00:00
Owner
Copy link

It's OK with me if you just call the interior variable problem even though it's being returned out to an exterior variable named problem -- I don't think it would reduce clarity. Not necessary to change, just wanted to make sure you knew that I don't have any issue with using the same name in a case like this. And I do think that concise variable names are definitely helpful.

It's OK with me if you just call the interior variable `problem` even though it's being returned out to an exterior variable named `problem` -- I don't think it would reduce clarity. Not necessary to change, just wanted to make sure you knew that I don't have any issue with using the same name in a case like this. And I do think that concise variable names are definitely helpful.
Vectornaut commented 2025-04-14 20:31:45 +00:00
Author
Member
Copy link

Good to know. For now, I think it's worth clearly distinguishing the interior "builder" variable and the exterior "storage" variable, because they have some differences in usage: for example, the builder variable is mutable, and the storage variable isn't. However, I'd be fine with switching to the convention of using the same name for both.

Good to know. For now, I think it's worth clearly distinguishing the interior "builder" variable and the exterior "storage" variable, because they have some differences in usage: for example, the builder variable is mutable, and the storage variable isn't. However, I'd be fine with switching to the convention of using the same name for both.
glen commented 2025-04-14 23:23:32 +00:00
Owner
Copy link

Well, we are either going to have the convention of using the same variable name inside and outside, or always distinguishing the names. We're not going to do it one way in some places and the other way in other places. I like the "same name" convention better -- more concise with no loss in clarity. So if you are not against that change in convention, please just go ahead and change this instance to conform. We can discuss (A,B,C) from the other thread in person.

Well, we are either going to have the convention of using the same variable name inside and outside, or always distinguishing the names. We're not going to do it one way in some places and the other way in other places. I like the "same name" convention better -- more concise with no loss in clarity. So if you are not against that change in convention, please just go ahead and change this instance to conform. We can discuss (A,B,C) from the other thread in person.
Vectornaut commented 2025-04-16 06:54:47 +00:00
Author
Member
Copy link

I've switched to the same-name convention in commit 955220c.

I've switched to the same-name convention in commit 955220c.
glen commented 2025-04-16 21:26:00 +00:00
Owner
Copy link

Great, I see that, and this code is fine. It occurs to me that the organization that would be used for this code in some languages would be to have the problem = ConstraintProblem::new(...) outside the closure in self.elements.with_untracked(...) and then have some sort of reference to the problem be visible inside that closure. I suppose that is possible in Rust as well. If you think it's cleaner/clearer to organize it that way, feel free to refactor this, but also feel free to leave it as is if you prefer. In the latter case, please just resolve this thread.

Great, I see that, and this code is fine. It occurs to me that the organization that would be used for this code in some languages would be to have the `problem = ConstraintProblem::new(...)` outside the closure in self.elements.with_untracked(...) and then have some sort of reference to the problem be visible inside that closure. I suppose that is possible in Rust as well. If you think it's cleaner/clearer to organize it that way, feel free to refactor this, but also feel free to leave it as is if you prefer. In the latter case, please just resolve this thread.
Vectornaut commented 2025-04-17 21:47:39 +00:00
Author
Member
Copy link

I chose this organization so that problem could be immutable in the scope where it's used, and only mutable in the scope where it's created.

I chose this organization so that `problem` could be immutable in the scope where it's used, and only mutable in the scope where it's created.
glen marked this conversation as resolved
glen commented 2025-04-10 23:40:00 +00:00
Owner
Copy link

Nice refactor! Tests pass and app behaves on my machine. Just a couple comments. Look like we should be able to merge this quickly.

Nice refactor! Tests pass and app behaves on my machine. Just a couple comments. Look like we should be able to merge this quickly.
Vectornaut added 2 commits 2025-04-16 06:49:18 +00:00 
This routine is implemented in a very representation-specific way, so
the engine seems like the best place for it.
Shadow storage variable with builder variable
All checks were successful
/ test (pull_request) Successful in 2m24s
Details
955220c0bc
Vectornaut commented 2025-04-16 07:00:28 +00:00
Author
Member
Copy link

I've made the changes we discussed during Monday's meeting, so this should be ready for review again!

I've made the changes we discussed during Monday's meeting, so this should be ready for review again!
glen reviewed 2025-04-16 20:54:48 +00:00
app-proto/src/assembly.rs Outdated
@ -127,0 +209,4 @@
self.set_point.with_untracked(|set_pt| {
if let Some(val) = set_pt.value {
if let Some(col) = elts[self.subject].column_index {
const CURVATURE_COMPONENT: usize = 3;
glen commented 2025-04-16 20:54:48 +00:00
Owner
Copy link

At some point, it would be nice to have symbolic names for all of the components in some standard place and use them anywhere needed. I am not saying it has to be part of this PR. But it could be. Thoughts?

At some point, it would be nice to have symbolic names for all of the components in some standard place and use them anywhere needed. I am not saying it has to be part of this PR. But it could be. Thoughts?
glen commented 2025-04-16 21:13:41 +00:00
Owner
Copy link

Since you have to make the other 3 the same as this 3, likely you may as well do centralized symbolic names for all the components right at the moment, although anything that makes the two 3s the same thing and not magic numbers is OK for the moment if you prefer not to do a full central enumeration of components.

Since you have to make the _other_ 3 the same as this 3, likely you may as well do centralized symbolic names for all the components right at the moment, although anything that makes the two 3s the same thing and not magic numbers is OK for the moment if you prefer not to do a full central enumeration of components.
👍 1
Vectornaut commented 2025-04-17 21:23:05 +00:00
Author
Member
Copy link

Done, by making CURVATURE_COMPONENT an associated constant of the Element structure (commit 7f21e7e). When we introduce separate structures for different kinds of elements, this constant will go in the Sphere structure.

Done, by making `CURVATURE_COMPONENT` an associated constant of the `Element` structure (commit 7f21e7e). When we introduce separate structures for different kinds of elements, this constant will go in the `Sphere` structure.
glen commented 2025-04-21 19:53:13 +00:00
Owner
Copy link

There's still pretty tight coupling between Elements and the Engine, because Elements are storing their own representations in Engine coordinates. So the Elements are not really abstract geometric elements, divorced from Engine details. Or put another way, an Engine change would entail deeper changes to the innards of Elements than might be ideal. But there's no real need to pre-worry about fixing that until we actually want to try another Engine. And the revised code does eliminate unlinked occurrences of the same magic number, so resolving this conversation.

There's still pretty tight coupling between Elements and the Engine, because Elements are storing their own representations in Engine coordinates. So the Elements are not really abstract geometric elements, divorced from Engine details. Or put another way, an Engine change would entail deeper changes to the innards of Elements than might be ideal. But there's no real need to pre-worry about fixing that until we actually want to try another Engine. And the revised code does eliminate unlinked occurrences of the same magic number, so resolving this conversation.
glen marked this conversation as resolved
glen reviewed 2025-04-16 21:08:20 +00:00
app-proto/src/assembly.rs Outdated
@ -255,0 +351,4 @@
// create and insert a new product regulator
let measurement = self.elements.map(
move |elts| {
let representations = subjects.map(|subj| elts[subj].representation);
glen commented 2025-04-16 21:08:20 +00:00
Owner
Copy link

I figured out what's bothering me about this code. It's connected with my feeling that these two methods (insert_new_product_regulator and insert_new_half_curvature_regulator) should either be the same or be more highly factored than they are now. It's that there's a de-localization of concerns at the moment. It appears that the definition of the measurement of a product regulator or a half-curvature regulator currently "lives" in these insertion methods. But such code should be part of the code for that flavor of regulator, not in an "outside" method like this insertion gadget. The current code structure suggests that it would be perfectly legitimate to insert a half-curvature regulator that measures the mean of the first three coordinates. It would not be.

In other words, we don't have ideal encapsulation of regulators at the moment. Please refactor; and hopefully, after refactoring, these two functions will be identical or very thin veneer over something identical. Thanks!

I figured out what's bothering me about this code. It's connected with my feeling that these two methods (insert_new_product_regulator and insert_new_half_curvature_regulator) should either be the same or be more highly factored than they are now. It's that there's a de-localization of concerns at the moment. It appears that the definition of the measurement of a product regulator or a half-curvature regulator currently "lives" in these insertion methods. But such code should be part of the code for that flavor of regulator, not in an "outside" method like this insertion gadget. The current code structure suggests that it would be perfectly legitimate to insert a half-curvature regulator that measures the mean of the first three coordinates. It would not be. In other words, we don't have ideal encapsulation of regulators at the moment. Please refactor; and hopefully, after refactoring, these two functions will be identical or very thin veneer over something identical. Thanks!
Vectornaut commented 2025-04-16 22:59:47 +00:00
Author
Member
Copy link

But such code should be part of the code for that flavor of regulator, not in an "outside" method like this insertion gadget.

I think that's a great idea. I propose moving the insertion code into a method of the Regulator trait, analogous to how the code that writes constraint problem data is kept in the pose method of the ProblemPoser trait. If you'd like, we could try to make a supertrait for anything that can be inserted into an assembly, which would be even more analogous to the ProblemPoser trait, but I don't think that's called for yet.

> But such code should be part of the code for that flavor of regulator, not in an "outside" method like this insertion gadget. I think that's a great idea. I propose moving the insertion code into a method of the `Regulator` trait, analogous to how the code that writes constraint problem data is kept in the `pose` method of the `ProblemPoser` trait. If you'd like, we could try to make a supertrait for anything that can be inserted into an assembly, which would be even more analogous to the `ProblemPoser` trait, but I don't think that's called for yet.
glen commented 2025-04-16 23:14:34 +00:00
Owner
Copy link

yes a method of regulator trait sounds good, no not necessary to generalize into an assembly-inserting supertrait, at least not until we have something else that would also use that supertrait, i.e. no need for premature abstraction. Thanks!

yes a method of regulator trait sounds good, no not necessary to generalize into an assembly-inserting supertrait, at least not until we have something else that would also use that supertrait, i.e. no need for premature abstraction. Thanks!
Vectornaut commented 2025-04-17 21:27:31 +00:00
Author
Member
Copy link

Done (commits ee8a01b – 52d9975). The code that creates and inserts regulators looks much better-organized now!

Done (commits ee8a01b – 52d9975). The code that creates and inserts regulators looks much better-organized now!
glen marked this conversation as resolved
glen reviewed 2025-04-16 21:10:14 +00:00
app-proto/src/assembly.rs Outdated
@ -264,0 +382,4 @@
pub fn insert_new_half_curvature_regulator(&self, subject: ElementKey) {
// create and insert a new half-curvature regulator
let measurement = self.elements.map(
move |elts| elts[subject].representation.with(|rep| rep[3])
glen commented 2025-04-16 21:10:14 +00:00
Owner
Copy link

Aha, this 3 is the same 3 as that other one I recently commented about. They need to be instances of the same symbol. Otherwise, they're just magic numbers that may leave a maintainer wondering whether and why they need to be the same. Thanks for fixing.

Aha, this 3 is the _same_ 3 as that other one I recently commented about. They need to be instances of the same symbol. Otherwise, they're just magic numbers that may leave a maintainer wondering whether and why they need to be the same. Thanks for fixing.
👍 1
glen marked this conversation as resolved
glen commented 2025-04-16 21:33:40 +00:00
Owner
Copy link

RESOLVED: I also just realized that the names of the two regulators are not parallel: currently they are "HalfCurvatureRegulator" and "ProductRegulator". The first is geometry-centric, the second is representation-centric. So they should either be "HalfCurvatureRegulator"/"InversiveDistanceRegulator" or "FourthComponentRegulator"/"ProductRegulator". I think the more geometric perspective is appropriate for their role in the code: if we changed representations, we could still potentially have an InversiveDistanceRegulator but we might have to completely reimplement it, whereas a ProductRegulator might mean something completely different, changing the behavior of existing assemblies. But either way, please do make the names parallel (not wedded to the exact spellings), thanks.

RESOLVED: I also just realized that the names of the two regulators are not parallel: currently they are "HalfCurvatureRegulator" and "ProductRegulator". The first is geometry-centric, the second is representation-centric. So they should either be "HalfCurvatureRegulator"/"InversiveDistanceRegulator" or "FourthComponentRegulator"/"ProductRegulator". I think the more geometric perspective is appropriate for their role in the code: if we changed representations, we could still potentially have an InversiveDistanceRegulator but we might have to completely reimplement it, whereas a ProductRegulator might mean something completely different, changing the behavior of existing assemblies. But either way, please do make the names parallel (not wedded to the exact spellings), thanks.
glen commented 2025-04-16 21:36:38 +00:00
Owner
Copy link

OK, that's another round of review; will pick up again (or merge) when these points have been addressed. Thanks!

OK, that's another round of review; will pick up again (or merge) when these points have been addressed. Thanks!
Vectornaut commented 2025-04-16 23:04:33 +00:00
Author
Member
Copy link

I also just realized that the names of the two regulators are not parallel: currently they are "HalfCurvatureRegulator" and "ProductRegulator". The first is geometry-centric, the second is representation-centric.

This inconsistency in naming reflects a current inconsistency in design. The product regulator, as currently written, isn't limited to spheres, so calling it an inversive distance regulator would be more of a usage suggestion than a reflection of what it concretely does. On the other hand, it does label itself an "inversive distance regulator" in the outline view, so there's at least one small behavior that concretely distinguishes it from a general product regulator. If you think this is enough to merit the name InversiveDistanceRegulator, I'll make that change.

> I also just realized that the names of the two regulators are not parallel: currently they are "HalfCurvatureRegulator" and "ProductRegulator". The first is geometry-centric, the second is representation-centric. This inconsistency in naming reflects a current inconsistency in design. The product regulator, as currently written, isn't limited to spheres, so calling it an inversive distance regulator would be more of a usage suggestion than a reflection of what it concretely does. On the other hand, it does label itself an "inversive distance regulator" in the outline view, so there's at least one small behavior that concretely distinguishes it from a general product regulator. If you think this is enough to merit the name `InversiveDistanceRegulator`, I'll make that change.
glen commented 2025-04-16 23:13:14 +00:00
Owner
Copy link

This one is intended to be an inversive distance regulator. If the product means something else for two points, say, and we want that regulator too, we will call it by its geometric name, and share 95-100% of the code until we switch representations and then perhaps the code will need to diverge. By keeping track of the intention of each regulator, we know what we do/don't have to reimplement when we switch representations/engines. Thanks!

This one is intended to be an inversive distance regulator. If the product means something else for two points, say, and we want that regulator too, we will call it by its geometric name, and share 95-100% of the code until we switch representations and then perhaps the code will need to diverge. By keeping track of the intention of each regulator, we know what we do/don't have to reimplement when we switch representations/engines. Thanks!
Vectornaut added 4 commits 2025-04-17 21:20:25 +00:00 
This improves code organization at the cost of a little redundancy: the
default implementation of `activate` doesn't do anything, and its
implementation for `HalfCurvatureRegulator` redundantly accesses the
set point signal and checks whether the regulator is set.
This will make it easier to give each regulator a constructor instead of
an "insert new" method.
The assembly shouldn't have to know how to construct regulators.
Centralize the curvature component index constant
All checks were successful
/ test (pull_request) Successful in 2m25s
Details
7f21e7e999
Vectornaut added 1 commit 2025-04-17 21:40:35 +00:00
Make regulator naming more consistent
All checks were successful
/ test (pull_request) Successful in 2m27s
Details
620a6be918 
Although the inversive distance regulator mostly acts as a general
Lorentz product regulator, calling it `InversiveDistanceRegulator`
explains how we intend to use it and demonstrates our naming convention.
Vectornaut commented 2025-04-17 21:42:02 +00:00
Author
Member
Copy link

This one is intended to be an inversive distance regulator.

Renamed (commit 620a6be). I think the code is ready for review again now.

> This one is intended to be an inversive distance regulator. Renamed (commit 620a6be). I think the code is ready for review again now.
Vectornaut added 3 commits 2025-04-18 04:35:52 +00:00
Flag regulator update logging as debug
All checks were successful
/ test (pull_request) Successful in 2m19s
Details
99a9c3ec55
Vectornaut commented 2025-04-18 04:41:24 +00:00
Author
Member
Copy link

I've made the changes we discussed during our meeting:

Let me know if you spot any other things to look at!

I've made the changes we discussed during our meeting: - Use [field init shorthand](https://doc.rust-lang.org/book/ch05-01-defining-structs.html#using-the-field-init-shorthand) in the regulator constructors (8dab223). - Make regulator activation less redundant (commit 5506ec1). Let me know if you spot any other things to look at!
Vectornaut commented 2025-04-18 04:44:06 +00:00
Author
Member
Copy link

Now that I've merged pull request #81, the branch for this pull request (curvature-regulators) needs to be rebased onto main. If I'm the only one with a local copy of this branch, I'd be happy to do the rebase so I can more easily keep everything in sync. If you have a local copy, let me know when is a good time to rebase.

Now that I've merged pull request #81, the branch for this pull request (`curvature-regulators`) needs to be rebased onto main. If I'm the only one with a local copy of this branch, I'd be happy to do the rebase so I can more easily keep everything in sync. If you have a local copy, let me know when is a good time to rebase.
Vectornaut commented 2025-04-19 16:19:16 +00:00
Author
Member
Copy link

Now that I've merged pull request #81, the branch for this pull request (curvature-regulators) needs to be rebased onto main.

Or maybe not! I just noticed that the merge control panel says "This pull request can be merged automatically." When you're ready to merge, could you try doing Create squash commit with the branch as-is, without rebasing first? I think this would make it easier to preserve all the commit references in the review conversation. (If that doesn't work, this comment applies again.)

> Now that I've merged pull request #81, the branch for this pull request (curvature-regulators) needs to be rebased onto main. Or maybe not! I just noticed that the merge control panel says "This pull request can be merged automatically." When you're ready to merge, could you try doing *Create squash commit* with the branch as-is, without rebasing first? I think this would make it easier to preserve all the commit references in the review conversation. (If that doesn't work, [this comment](#issuecomment-2739) applies again.)
glen reviewed 2025-04-21 19:45:36 +00:00
app-proto/src/assembly.rs Outdated
@ -122,1 +135,3 @@
pub subjects: (ElementKey, ElementKey),
impl ProblemPoser for Element {
fn pose(&self, problem: &mut ConstraintProblem, _elts: &Slab<Element>) {
if let Some(index) = self.column_index {
glen commented 2025-04-21 19:45:36 +00:00
Owner
Copy link

The check here and the potential panic if it fails (and analogous code in two other ProblemPoser implementations) is evidence of misfactoring: in fact, the ProblemPosers only ever get to execute when every element has just been assigned a column index. So there is no need to have such a check.

I am open to any reasonable way to factor these checks out. One is to insist that all elements in an assembly always have a column index. As far as I can see, the only thing that None column indices are currently used for is a transient period between an Element being added to an Assembly and the next call to that Assembly being realized. That period could be eliminated by simply assigning each Element the next available column index and immediately calling realize. Or there could be a method (likely in engine?) that takes a realized assembly and a new completely unconstrained element and updates the realization to include the unconstrained element -- that should presumably be far less work than a full call to realize. As far as I can tell, the only consistency bit that needs to be kept up is the tangent space bases in the realization. But I suppose it should be possible to calculate the new tangent space basis matriceso directly in the case of a new unconstrained element added to a realized assembly.

Or we can insist that elements always have an index by assigning indices on creation, and dropping the invariant that an element with an index has to have tangent motions in the corresponding column of the tangent space basis matrices. That might not be so terrible -- if an element's column is out of range for the tangent space basis matrices, that must mean it is totally unconstrained, and so we can actually deform it freely, and so just switch to simpler deformation code for that case.

Another possibility is to initially use column index -1 for a new element, and not worry about any -1s in the problem posing because the code is structured such that they never occur when a problem is being posed; we only need to worry about the -1s when deforming, and handle the case there.

Another possibility (if I understand Rust and the Option type properly) is to simply use unwrap in these posers. It nominally could panic, but we know it won't because problems are only posed just after every element has been assigned an index. This route seems the least satisfying because we still have all of the baggage of Option even though the other possibilities make it clear that Option isn't filling a particularly critical need for the column indices of elements. But this route also might be the path of least resistance, so I'm OK if that's the way you want to go here.

Or there might be some other mechanism whereby the data structure passed to a problem poser is something of a slightly different type that definitely has a column for each element. (Perhaps this mild redundant-checking issue is even evidence that elements shouldn't store their column indices; conceptually, a column index is more of a property of an Element as a part of an Assembly, rather than intrinsically of the Element itself. So maybe instead the Assembly has some record of the column index for each Element, where the indices are plain ol' integers, and that record is provided to problem posers. But I am definitely not mandating a refactor of that scope as part of this PR.)

What doesn't seem good is writing out checks and manual panic messages in three places, for panics that can literally never occur because of the structure of the code. That redundant checking is what I mean by "evidence of misfactoring".

The check here and the potential panic if it fails (and analogous code in two other ProblemPoser implementations) is evidence of misfactoring: in fact, the ProblemPosers only ever get to execute when every element has just been assigned a column index. So there is no need to have such a check. I am open to any reasonable way to factor these checks out. One is to insist that all elements in an assembly always have a column index. As far as I can see, the only thing that None column indices are currently used for is a transient period between an Element being added to an Assembly and the next call to that Assembly being realized. That period could be eliminated by simply assigning each Element the next available column index and immediately calling realize. Or there could be a method (likely in engine?) that takes a realized assembly and a new completely unconstrained element and updates the realization to include the unconstrained element -- that should presumably be far less work than a full call to realize. As far as I can tell, the only consistency bit that needs to be kept up is the tangent space bases in the realization. But I suppose it should be possible to calculate the new tangent space basis matriceso directly in the case of a new unconstrained element added to a realized assembly. Or we can insist that elements always have an index by assigning indices on creation, and dropping the invariant that an element with an index has to have tangent motions in the corresponding column of the tangent space basis matrices. That might not be so terrible -- if an element's column is out of range for the tangent space basis matrices, that must mean it is totally unconstrained, and so we can actually deform it freely, and so just switch to simpler deformation code for that case. Another possibility is to initially use column index -1 for a new element, and not worry about any -1s in the problem posing because the code is structured such that they never occur when a problem is being posed; we only need to worry about the -1s when deforming, and handle the case there. Another possibility (if I understand Rust and the Option type properly) is to simply use unwrap in these posers. It nominally could panic, but we know it won't because problems are only posed just after every element has been assigned an index. This route seems the least satisfying because we still have all of the baggage of Option even though the other possibilities make it clear that Option isn't filling a particularly critical need for the column indices of elements. But this route also might be the path of least resistance, so I'm OK if that's the way you want to go here. Or there might be some other mechanism whereby the data structure passed to a problem poser is something of a slightly different type that definitely has a column for each element. (Perhaps this mild redundant-checking issue is even evidence that elements shouldn't store their column indices; conceptually, a column index is more of a property of an Element as a part of an Assembly, rather than intrinsically of the Element itself. So maybe instead the Assembly has some record of the column index for each Element, where the indices are plain ol' integers, and that record is provided to problem posers. But I am definitely not mandating a refactor of that scope as part of this PR.) What doesn't seem good is writing out checks and manual panic messages in three places, for panics that can literally never occur because of the structure of the code. That redundant checking is what I mean by "evidence of misfactoring".
Vectornaut commented 2025-04-21 21:56:27 +00:00
Author
Member
Copy link

I think there are two problems here—a narrow one that falls within the scope of this pull request, and a broader one that deserves its own pull request.

Narrow: semantics issue

The conditional panic doesn't do a great job of communicating that we expect every element to have a column index when pose is called. Unwrapping the Option, as you point out, has the semantics we want, and that's what we do on the main branch (commit b86f176). However, unwrapping doesn't provide an informative panic message, which would make it harder to debug an invariant violation.

I've switched to the expect method, which has the same semantics, and allows us to provide our own panic message (commit 5eeb093). I've also rewritten the panic messages in the style recommended by the expect documentation.

Broad: factoring issue

I agree that we should eventually rewrite the column index system to avoid these checks entirely. I think that deserves its own pull request, since this pull request didn't create and doesn't modify the column index system.

I think there are two problems here—a narrow one that falls within the scope of this pull request, and a broader one that deserves its own pull request. #### Narrow: semantics issue The conditional panic doesn't do a great job of communicating that we expect every element to have a column index when `pose` is called. Unwrapping the `Option`, as you point out, has the semantics we want, and that's what we do on the main branch (commit b86f176). However, unwrapping doesn't provide an informative panic message, which would make it harder to debug an invariant violation. I've switched to the [`expect`](https://doc.rust-lang.org/std/option/enum.Option.html#method.expect) method, which has the same semantics, and allows us to provide our own panic message (commit 5eeb093). I've also rewritten the panic messages in the [style recommended](https://doc.rust-lang.org/std/option/enum.Option.html#recommended-message-style) by the `expect` documentation. #### Broad: factoring issue I agree that we should eventually rewrite the column index system to avoid these checks entirely. I think that deserves its own pull request, since this pull request didn't create and doesn't modify the column index system.
glen marked this conversation as resolved
glen commented 2025-04-21 20:09:26 +00:00
Owner
Copy link

OK, re-reviewed; all of the prior open threads appear to have been satisfactorily resolved, at least vis-a-vis the goals of this PR. So all that remains is the new point I noticed, commented on above. Since all of those pose methods that have the redundant checks are new in this PR, it does seem like it should be addressed within this PR. Thanks!

OK, re-reviewed; all of the prior open threads appear to have been satisfactorily resolved, at least vis-a-vis the goals of this PR. So all that remains is the new point I noticed, commented on above. Since all of those pose methods that have the redundant checks are new in this PR, it does seem like it should be addressed within this PR. Thanks!
Vectornaut added 1 commit 2025-04-21 21:47:41 +00:00
Change conditional panic to expect
All checks were successful
/ test (pull_request) Successful in 2m21s
Details
5eeb0935ca 
Use `expect` to communicate that every element should have a column
index when `pose` is called. Rewrite the panic messages in the style
recommended by the `expect` documentation.
glen merged commit 360ce12d8b into main 2025-04-21 23:40:43 +00:00
glen referenced this pull request from a commit 2025-04-21 23:40:43 +00:00
feat: Curvature regulators (#80)
Sign in to join this conversation.
Reviewers
No reviewers
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#80
No description provided.
Delete branch "Vectornaut/dyna3:curvature-regulators"

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?