Curvature regulators #80
No reviewers
Labels
No labels
bug
design
duplicate
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: StudioInfinity/dyna3#80
Loading…
Add table
Add a link
Reference in a new issue
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?
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
traitEach 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 twoRegulator
implementations:ProductRegulator
andHalfCurvatureRegulator
.Right now, I'm imagining that
Regulator
will have one supertrait for each module a regulator needs to communicate with:assembly::ProblemPoser
trait, described below, provides for communication with the engine.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 theassembly
module. That's becauseProblemPoser
seems especially tightly integrated with the design of elements and regulators.The
OutlineItem
traitIn 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
structureThe specification of a constraint problem is encapsulated in the
engine::ConstraintProblem
structure. To send a problem to the engine, we create a newConstraintProblem
, write the constraints and the initial guess into it, and then pass it toengine::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
traitEach 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 thepose
method. In the branch to be merged, there are three kinds of problem posers:ProductRegulator
), which fixes the Lorentz product between two element representation vectors by setting an off-diagonal entry of the Gram matrix.HalfCurvatureRegulator
), which freezes the curvature entry of a sphere's representation vector.Element
, because it's the only kind of element so far), which: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 thepose
method is where the rubber will meet the road.Future directions
Element
from a structure that implementsProblemPoser
to a trait that hasProblemPoser
as a supertrait, just like this pull request changesRegulator
from a structure to a trait.OutlineItem
provides for communication with the outline view.ConstraintProblem
structure encapsulates a constraint problem passed into the engine.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.WIP: Curvature regulatorsto Curvature regulatorsI'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.)
@Vectornaut wrote in #80 (comment):
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 #80 (comment):
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.
Yes, the future
Element
trait should haveProblemPoser
as a supertrait. I've updated the "Future directions" section to mention this.@ -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 || {
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.
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
andHalfCurvatureRegulator
are different structures. The only thing that seems really straightforwardly shared is initializing the set point tocreate_signal(SpecifiedValue::from_empty_spec())
,which seems like too little code to factor out profitably.
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:
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.
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.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 routinechange_half_curvature
for now.@ -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());
It's OK with me if you just call the interior variable
problem
even though it's being returned out to an exterior variable namedproblem
-- 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.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.
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.
I've switched to the same-name convention in commit
955220c
.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.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.Nice refactor! Tests pass and app behaves on my machine. Just a couple comments. Look like we should be able to merge this quickly.
I've made the changes we discussed during Monday's meeting, so this should be ready for review again!
@ -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;
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?
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.
Done, by making
CURVATURE_COMPONENT
an associated constant of theElement
structure (commit7f21e7e
). When we introduce separate structures for different kinds of elements, this constant will go in theSphere
structure.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.
@ -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);
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 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 thepose
method of theProblemPoser
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 theProblemPoser
trait, but I don't think that's called for yet.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!
Done (commits
ee8a01b
–52d9975
). The code that creates and inserts regulators looks much better-organized now!@ -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])
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.
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.
OK, that's another round of review; will pick up again (or merge) when these points have been addressed. Thanks!
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.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!
insert_regulator
8f8e806d12Renamed (commit
620a6be
). I think the code is ready for review again now.I've made the changes we discussed during our meeting:
8dab223
).5506ec1
).Let me know if you spot any other things to look at!
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.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.)
@ -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 {
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".
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 theOption
, as you point out, has the semantics we want, and that's what we do on the main branch (commitb86f176
). 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 (commit5eeb093
). I've also rewritten the panic messages in the style recommended by theexpect
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.
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!
expect