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

feat: Point coordinate regulators #118

Merged
Vectornaut merged 9 commits from glen/dyna3:pointCoordRegulators into main 2025-10-13 22:52:03 +00:00
Conversation 27 Commits 9 Files changed 6 +180 -49
glen commented 2025-09-20 08:56:24 +00:00
Owner
Copy link

Implements regulators for the Euclidean coordinates of Point entities, automatically creating all three of them for each added point entity. When such a regulator is set, it freezes the corresponding representation coordinate to the set point. In addition, if all three coordinates of a given Point are set, the coradius coordinate (which holds the norm of the point) is frozen as well.

Note that a PointCoordinateRegulator must be created with a Point as the subject. This commit modifies HalfCurvatureRegulator analogously, so that it can only be created with a Sphere.

A couple of prospective issues that should be filed in association with this commit:

  • (Covered by issue #122) The new coordinate regulators create redundant display information with the raw representation coordinates of a point that are already shown in the outline view.
  • (Underlying issue is #123) The optimization status of these regulators together with HalfCurvature regulators (i.e., the ones implemented by freezing coordinates) is different from InversiveDistance regulators when an Assembly is unrealizable: the frozen-coordinate constraints will be "hard" in that they will be forced to precisely equal their set point, whereas the distance regulators are "soft" in that they can be relaxed from their set points in an effort to minimize the loss function of the configuration as compared to the values of the constraints. Perhaps at some point we should/will have a mechanism to specify the softness/hardness of constraints, but in the meantime, there should not be two different categories of constraints. Suppose we decide that by default that all constraints are soft. Then the optimizer should be able to search changing, for example, the radius of a curvature-constrained sphere, so as to minimize the loss function (for a loss that would therefore presumably have a term akin to the square of the difference between the specified and actual half-curvature of the sphere). For example, suppose you specify that the half-curvature of a sphere is 1 (so it has radius \tfrac{1}{2}) but that its distance to a point is -1. These constraints cannot be satisfied, so the optimization fails, presumably with the point at the sphere center, and the sphere with radius \tfrac{1}{2}. So all of the loss is concentrated in the difference between the actual point-sphere distance being -\tfrac{1}{2}, not -1. It would be more appropriate (in the all-soft constraint regime) to end up at something like a sphere of half-curvature \tfrac{1}{\sqrt{2}} with the point at the center, so that the loss is split between both the half-curvature and the distance to the sphere being off by 1 - \tfrac{1}{\sqrt{2}}. (At a guess, that would minimize the sum of the squares of the two differences.)
Implements regulators for the Euclidean coordinates of `Point` entities, automatically creating all three of them for each added point entity. When such a regulator is set, it freezes the corresponding representation coordinate to the set point. In addition, if all three coordinates of a given `Point` are set, the coradius coordinate (which holds the norm of the point) is frozen as well. Note that a `PointCoordinateRegulator` must be created with a `Point` as the subject. This commit modifies `HalfCurvatureRegulator` analogously, so that it can only be created with a `Sphere`. A couple of prospective issues that should be filed in association with this commit: - [x] _(Covered by issue #122)_ The new coordinate regulators create redundant display information with the raw representation coordinates of a point that are already shown in the outline view. - [x] _(Underlying issue is #123)_ The optimization status of these regulators together with HalfCurvature regulators (i.e., the ones implemented by freezing coordinates) is different from InversiveDistance regulators when an Assembly is unrealizable: the frozen-coordinate constraints will be "hard" in that they will be forced to precisely equal their set point, whereas the distance regulators are "soft" in that they can be relaxed from their set points in an effort to minimize the loss function of the configuration as compared to the values of the constraints. Perhaps at some point we should/will have a mechanism to specify the softness/hardness of constraints, but in the meantime, there should not be two different categories of constraints. Suppose we decide that by default that all constraints are soft. Then the optimizer should be able to search changing, for example, the radius of a curvature-constrained sphere, so as to minimize the loss function (for a loss that would therefore presumably have a term akin to the square of the difference between the specified and actual half-curvature of the sphere). For example, suppose you specify that the half-curvature of a sphere is $1$ (so it has radius $\tfrac{1}{2}$) but that its distance to a point is $-1$. These constraints cannot be satisfied, so the optimization fails, presumably with the point at the sphere center, and the sphere with radius $\tfrac{1}{2}$. So all of the loss is concentrated in the difference between the actual point-sphere distance being $-\tfrac{1}{2}$, not $-1$. It would be more appropriate (in the all-soft constraint regime) to end up at something like a sphere of half-curvature $\tfrac{1}{\sqrt{2}}$ with the point at the center, so that the loss is split between both the half-curvature and the distance to the sphere being off by $1 - \tfrac{1}{\sqrt{2}}$. (At a guess, that would minimize the sum of the squares of the two differences.)
glen added 1 commit 2025-09-20 08:56:24 +00:00
feat: Point coordinate regulators
All checks were successful
/ test (pull_request) Successful in 3m35s
Details
ecbbe2068c 
Implements regulators for the Euclidean coordinates of Point entities,
  automatically creating all three of them for each added point entity. When
  such a regulator is set, it freezes the corresponding representation
  coordinate to the set point. In addition, if all three coordinates of a
  given Point are set, the coradius coordinate (which holds the norm of the
  point) is frozen as well.

  Note that a PointCoordinateRegulator must be created with a Point as the
  subject. This commit modifies HalfCurvatureRegulator analogously, so that
  it can only be created with a Sphere.

  A couple of prospective issues that should be filed in association with
  this commit:
  * The new coordinate regulators create redundant display information with
    the raw representation coordinates of a point that are already shown in
    the outline view.
  * The optimization status of these regulators together with HalfCurvature
    regulators (i.e., the ones implemented by freezing coordinates) is different
    from InversiveDistance regulators when an Assembly is unrealizable: the
    frozen-coordinate constraints will be "hard" in that they will be forced
    to precisely equal their set point, whereas the distance regulators are
    "soft" in that they can be relaxed from their set points in an effort to
    minimize the loss function of the configuration as compared to the values
    of the constraints. Perhaps at some point we should/will have a mechanism
    to specify the softness/hardness of constraints, but in the meantime,
    there should not be two different categories of constraints. Suppose we
    decide that by default that all constraints are soft. Then the optimizer
    should be able to search changing, for example, the radius of a
    curvature-constrained sphere, so as to minimize the loss function (for a
    loss that would therefore presumably have a term akin to the square of the
    difference between the specified and actual half-curvature of the sphere).
    For example, suppose you specify that the half-curvature of a sphere is 1
    (so it has radius 1/2) but that its distance to a point is -1. These
    constraints cannot be satisfied, so the optimization fails, presumably
    with the point at the sphere center, and the sphere with radius 1/2.
    So all of the loss is concentrated in the difference between the actual
    point-sphere distance being -1/2, not -1. It would be more appropriate
    (in the all-soft constraint regime) to end up at something like a sphere of
    half-curvature 1/√2 with the point at the center, so that the loss is split
    between both the half-curvature and the distance to the sphere being off by
    1 - 1/√2. (At a guess, that would minimize the sum of the squares of the
    two differences.)
glen commented 2025-09-20 08:57:49 +00:00
Author
Owner
Copy link

@Vectornaut once we are past finalizing the configuration of the Impossolid for the build, please review this PR, thanks.

@Vectornaut once we are past finalizing the configuration of the Impossolid for the build, please review this PR, thanks.
Vectornaut commented 2025-10-03 20:50:18 +00:00
Member
Copy link

I've tested the point coordinate regulators on my machine, and they seem to work as expected!

I'm happy overall with the design of the new regulator. It seems to slot into the existing framework smoothly, which is encouraging. A notable exception is the system that freezes a point's norm component if all three coordinates are frozen. I've opened a conversation about that below.

I'd make a few formatting changes for consistency with the conventions I've been using elsewhere. I'll start conversations about these in my next batch of comments.

I'll address the prospective issues you suggested in a later batch of comments.

I've tested the point coordinate regulators on my machine, and they seem to work as expected! I'm happy overall with the design of the new regulator. It seems to slot into the existing framework smoothly, which is encouraging. A notable exception is the system that freezes a point's norm component if all three coordinates are frozen. I've opened a [conversation](#issuecomment-3259) about that below. I'd make a few formatting changes for consistency with the conventions I've been using elsewhere. I'll start conversations about these in my next batch of comments. I'll address the prospective issues you suggested in a later batch of comments.
Vectornaut requested changes 2025-10-03 21:22:38 +00:00
Dismissed
app-proto/src/assembly.rs Outdated
@ -501,0 +514,4 @@
pub enum Axis {X = 0, Y = 1, Z = 2}
impl Axis {
pub const N_AXIS: usize = (Axis::Z as usize) + 1;
Vectornaut commented 2025-10-03 20:51:33 +00:00
Member
Copy link

The associated constant Axis::N_AXIS, which is defined by hand here, has the same value as the associated constant Axis::CARDINALITY, which is defined automatically when we derive the Sequence trait. I'd recommend removing N_AXIS and using CARDINALITY in its place. I'm confident that CARDINALITY is currently 3, because I get a compilation error if I replace it with a value different from 3 in the definition of Axis::NAME.

The associated constant `Axis::N_AXIS`, which is defined by hand here, has the same value as the associated constant [`Axis::CARDINALITY`](https://docs.rs/enum-iterator/latest/enum_iterator/trait.Sequence.html#associatedconstant.CARDINALITY), which is defined automatically when we derive the `Sequence` trait. I'd recommend removing `N_AXIS` and using `CARDINALITY` in its place. I'm confident that `CARDINALITY` is currently 3, because I get a compilation error if I replace it with a value different from 3 in the definition of `Axis::NAME`.
glen commented 2025-10-04 23:50:41 +00:00
Author
Owner
Copy link

Done in latest commit

Done in latest commit
👍 1
Vectornaut marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -501,0 +554,4 @@
"Subject must be indexed before point-coordinate regulator poses.");
problem.frozen.push(self.axis as usize, col, val);
// Check if all three coordinates have been frozen, and if so,
// freeze the coradius as well
Vectornaut commented 2025-10-03 21:09:42 +00:00
Member
Copy link

Design

This kind of interaction between problem posers is something we didn't anticipate when designing the ProblemPoser trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a /* KLUDGE */ flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the ProblemPoser trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.

Testing

To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call

// log the Gram matrix
console_log!("Gram matrix:\n{}", problem.gram);

with

// log the Gram matrix and frozen entries
console_log!("Gram matrix:\n{}", problem.gram);
console_log!("Frozen entries:\n{}", problem.frozen);

I tried this out locally, and it confirms that the interaction system is working in at least some cases.

Commenting

I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the /* KLUDGE */ flag suggested above and uses the capitalization convention that most other comments follow.

// if the three representation components that specify the
// point's Euclidean coordinates have been frozen, then freeze
// the norm component too
/* KLUDGE */
// we didn't anticipate this kind of interaction when we
// designed the problem poser system, so the implementation
// feels awkward
### Design This kind of interaction between problem posers is something we didn't anticipate when designing the `ProblemPoser` trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a `/* KLUDGE */` flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request. ### Testing To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call ```rust // log the Gram matrix console_log!("Gram matrix:\n{}", problem.gram); ``` with ```rust // log the Gram matrix and frozen entries console_log!("Gram matrix:\n{}", problem.gram); console_log!("Frozen entries:\n{}", problem.frozen); ``` I tried this out locally, and it confirms that the interaction system is working in at least some cases. ### Commenting I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the `/* KLUDGE */` flag suggested above and uses the capitalization convention that most other comments follow. ```rust // if the three representation components that specify the // point's Euclidean coordinates have been frozen, then freeze // the norm component too /* KLUDGE */ // we didn't anticipate this kind of interaction when we // designed the problem poser system, so the implementation // feels awkward ```
glen commented 2025-10-04 23:56:19 +00:00
Author
Owner
Copy link

@Vectornaut wrote in #118 (comment):

the best way to resolve the tension would be to redesign the ProblemPoser trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.

What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3259: > the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request. What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.
glen commented 2025-10-06 22:50:47 +00:00
Author
Owner
Copy link

OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.

OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.
Vectornaut commented 2025-10-07 19:37:12 +00:00
Member
Copy link

I agree that the comment in issue #90 is enough to remind us about this.

What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

We discussed this a little during our check-in meeting, and I jotted down a comment about our conclusions on issue #90.

I agree that the comment in issue #90 is enough to remind us about this. > What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks. We discussed this a little during our check-in meeting, and I jotted down a [comment](https://code.studioinfinity.org/StudioInfinity/dyna3/issues/90#issuecomment-3288) about our conclusions on issue #90.
Vectornaut marked this conversation as resolved
app-proto/src/components/outline.rs Outdated
@ -122,0 +126,4 @@
view! {
li(class = "regulator") {
div(class = "regulator-label") { (Axis::NAME[self.axis as usize]) }
div(class = "regulator-type") { "Coordinate" }
Vectornaut commented 2025-10-03 21:21:03 +00:00
Member
Copy link

In the outline items for previously implemented regulators, we use the regulator-label div to say which other element, if any, is subject to the regulator, and we use the regulator-type label to say what's being regulated. To bring the outline item for the point coordinate regulator in line with this convention, I'd rewrite it like this.

let reg_type = format!("{} coordinate", Axis::NAME[self.axis as usize]);
view! {
    li(class = "regulator") {
        div(class = "regulator-label") // for spacing
        div(class = "regulator-type") { (reg_type) }
        RegulatorInput(regulator = self)
        div(class = "status")
    }
}

I'm following the code organization we used for the half-curvature regulator, which is the only previously implemented single-subject regulator.

In the outline items for previously implemented regulators, we use the `regulator-label` div to say which other element, if any, is subject to the regulator, and we use the `regulator-type` label to say what's being regulated. To bring the outline item for the point coordinate regulator in line with this convention, I'd rewrite it like this. ```rust let reg_type = format!("{} coordinate", Axis::NAME[self.axis as usize]); view! { li(class = "regulator") { div(class = "regulator-label") // for spacing div(class = "regulator-type") { (reg_type) } RegulatorInput(regulator = self) div(class = "status") } } ``` I'm following the code organization we used for the half-curvature regulator, which is the only previously implemented single-subject regulator.
glen commented 2025-10-04 23:50:54 +00:00
Author
Owner
Copy link

done in latest commit

done in latest commit
Vectornaut commented 2025-10-07 19:40:43 +00:00
Member
Copy link

The code and the resulting interface both look like what I expect, so I'd say this is resolved.

The code and the resulting interface both look like what I expect, so I'd say this is resolved.
Vectornaut marked this conversation as resolved
app-proto/src/engine.rs Outdated
@ -47,3 +47,3 @@
// normalize a point's representation vector by scaling
pub fn project_point_to_normalized(rep: &mut DVector<f64>) {
rep.scale_mut(0.5 / rep[3]);
rep.scale_mut(0.5 / rep[3]); //FIXME: This 3 should be Point::WEIGHT_COMPONENT
Vectornaut commented 2025-10-03 21:21:29 +00:00
Member
Copy link

I've been flagging desired edits like this with /* TO DO */ // This 3... rather than //FIXME: This 3.... I'd like to stick to that format, because I think keeping flags consistent makes it easier to search for flagged code. I just started a wiki page to document the code flags I've been using.

I've been flagging desired edits like this with `/* TO DO */ // This 3...` rather than `//FIXME: This 3...`. I'd like to stick to that format, because I think keeping flags consistent makes it easier to search for flagged code. I just started a wiki page to document the [code flags](wiki/Code-flags) I've been using.
glen commented 2025-10-04 23:53:54 +00:00
Author
Owner
Copy link

I figured rather than any flag I would just go ahead and do it. But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState. How do we get rid of these? If there is an easy direct way please add a commit doing so. Or do we need to move Point into a different module commonly used by main and lib, and take AppState out of lib again? If so, let me know and I can go ahead and do that. And can we turn off the warning that hates on a module named appState.rs? To my mind, the clear place to define the AppState struct is in an appState module... Thanks.

I figured rather than any flag I would just go ahead and do it. But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState. How do we get rid of these? If there is an easy direct way please add a commit doing so. Or do we need to move Point into a different module commonly used by main and lib, and take AppState out of lib again? If so, let me know and I can go ahead and do that. And can we turn off the warning that hates on a module named appState.rs? To my mind, the clear place to define the AppState struct is in an appState module... Thanks.
Vectornaut commented 2025-10-06 20:42:25 +00:00
Member
Copy link

I'd prefer to just add a /* TO DO */ flag, because I think the best way to fix the issue would involve a broader discussion about the organization of the engine and assembly submodules, which is outside the scope of this pull request. I've included a little intro to that discussion below.

But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState.

These warnings in the library build are in line with the way I've been thinking of the engine submodule: as something that lives at a lower level of abstraction than the assembly submodule, and could eventually be spun off into a stand-alone library that the assembly submodule interacts with through an API.

I see this design as a path toward making the engine modular, like we ultimately want it to be. The modularity is very imperfect right now, but the imperfections are all in the assembly submodule, which refers to implementation details of the engine that it shouldn't really need or have access to. The WEIGHT_COMPONENT and NORM_COMPONENTconstants associated with Point are examples of those imperfections.

One approach to fixing them might be to add a new submodule that acts as a translation layer between the assembly and engine submodules by implementing ProblemPoser for all the elements and regulators. This translation layer might also be a natural home for the project_point_to_normalized method, which wants to refer to WEIGHT_COMPONENT.

I'd prefer to just add a `/* TO DO */` flag, because I think the best way to fix the issue would involve a broader discussion about the organization of the engine and assembly submodules, which is outside the scope of this pull request. I've included a little intro to that discussion below. > But using Point::WEIGHT_COMPONENT led to a world of unused warnings in the lib, since it never makes an AppState. These warnings in the library build are in line with the way I've been thinking of the engine submodule: as something that lives at a lower level of abstraction than the assembly submodule, and could eventually be spun off into a stand-alone library that the assembly submodule interacts with through an API. I see this design as a path toward making the engine modular, like we ultimately want it to be. The modularity is very imperfect right now, but the imperfections are all in the assembly submodule, which refers to implementation details of the engine that it shouldn't really need or have access to. The `WEIGHT_COMPONENT` and `NORM_COMPONENT`constants associated with `Point` are examples of those imperfections. One approach to fixing them might be to add a new submodule that acts as a translation layer between the assembly and engine submodules by implementing `ProblemPoser` for all the elements and regulators. This translation layer might also be a natural home for the `project_point_to_normalized` method, which wants to refer to `WEIGHT_COMPONENT`.
Vectornaut commented 2025-10-07 19:42:45 +00:00
Member
Copy link

This is now covered by issue #120. Thanks for posting that!

This is now covered by issue #120. Thanks for posting that!
Vectornaut marked this conversation as resolved
glen added 1 commit 2025-10-04 23:50:17 +00:00
refactor: as requested in review
Some checks failed
/ test (pull_request) Failing after 1m38s
Details
da9312ad71
glen commented 2025-10-04 23:57:00 +00:00
Author
Owner
Copy link

OK, I think I've responded to everything so far, but of course a few of the items are far from resolved.

OK, I think I've responded to everything so far, but of course a few of the items are far from resolved.
Vectornaut commented 2025-10-06 20:05:56 +00:00
Member
Copy link

@glen wrote in #118 (comment):

OK, I think I've responded to everything so far, but of course a few of the items are far from resolved.

Looking at your updates (commit da9312a) now! It looks like a new source file called appState.rs might've been left out of this commit. If so, pushing that file could help me review. Alternatively, you could revert the parts of da9312a that move AppState to the new source file. That might be more efficient, since I'm not sure I'd want to include the appState.rs refactoring in this pull request anyway.

@glen wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3268: > OK, I think I've responded to everything so far, but of course a few of the items are far from resolved. Looking at your updates (commit da9312a) now! It looks like a new source file called `appState.rs` might've been left out of this commit. If so, pushing that file could help me review. Alternatively, you could revert the parts of da9312a that move `AppState` to the new source file. That might be more efficient, since I'm not sure I'd want to include the `appState.rs` refactoring in this pull request anyway.
glen force-pushed pointCoordRegulators from da9312ad71 to ecbbe2068c 2025-10-06 22:22:15 +00:00 Compare
glen added 1 commit 2025-10-06 22:31:24 +00:00
chore: typographical improvements per review
All checks were successful
/ test (pull_request) Successful in 3m45s
Details
46ffd6c285
Vectornaut commented 2025-10-07 20:10:11 +00:00
Member
Copy link

Here are updates on the prospective issues mentioned in the pull request description.

The new coordinate regulators create redundant display information with
the raw representation coordinates of a point that are already shown in
the outline view.

This is now covered by issue #122, which clarifies that the representation vector in each element's outline view is there for debugging, and should be absent in production.

The optimization status of these regulators together with HalfCurvature
regulators (i.e., the ones implemented by freezing coordinates) is different
from InversiveDistance regulators when an Assembly is unrealizable […]

During our check-in meeting, we decided the underlying issue is an enhancement issue, which might be called "Graceful realization failure." @glen, let me know if you still want to file that one. I can take it over if you haven't started working on it yet.

Here are updates on the prospective issues mentioned in the pull request description. > The new coordinate regulators create redundant display information with the raw representation coordinates of a point that are already shown in the outline view. This is now covered by issue #122, which clarifies that the representation vector in each element's outline view is there for debugging, and should be absent in production. > The optimization status of these regulators together with HalfCurvature regulators (i.e., the ones implemented by freezing coordinates) is different from InversiveDistance regulators when an Assembly is unrealizable […] During our check-in meeting, we decided the underlying issue is an enhancement issue, which might be called "Graceful realization failure." @glen, let me know if you still want to file that one. I can take it over if you haven't started working on it yet.
glen commented 2025-10-07 20:40:28 +00:00
Author
Owner
Copy link

@Vectornaut wrote in #118 (comment):

@glen, let me know if you still want to file that one.

Nope, just filed #123. Thanks for the reminder.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3295: > @glen, let me know if you still want to file that one. Nope, just filed #123. Thanks for the reminder.
👍 1
Vectornaut requested changes 2025-10-07 23:45:10 +00:00
Vectornaut left a comment
Member
Copy link

Review complete, except for…

I've reviewed the updated code, and I'd be happy to merge it once @glen reviews the revisions that I've prepared! I haven't pushed those revisions yet, because there's one last thing I'd like to get feedback on first.

… a minor bug that I think is out of scope

Overview

I just noticed that this pull request reveals a minor bug in the engine: the realization routine panics if you give it a problem where every entry of the configuration is frozen.

Response

Fixing that bug seems outside the scope of this pull request, so I'd prefer to file an issue for it and deal with it in a separate pull request. If you agree, @glen, let me know so that I can push my proposed revisions and hand review over to you.

Reproduction

Here's one way to reproduce the bug.

  1. Load the empty test assembly.
  2. Add a single point.
  3. Set the regulators for all three of the point's spatial coordinates.
    • This produces a constraint problem where all of the point's inversive coordinates are frozen.

During engine::realize_gram, an nalgebra call panics with the message "At least one column must be given," which usually means we've tried to construct a matrix from an empty vector of columns.

### Review complete, except for… I've reviewed the updated code, and I'd be happy to merge it once @glen reviews the revisions that I've prepared! I haven't pushed those revisions yet, because there's one last thing I'd like to get feedback on first. ### … a minor bug that I think is out of scope #### Overview I just noticed that this pull request reveals a minor bug in the engine: the realization routine panics if you give it a problem where every entry of the configuration is frozen. #### Response Fixing that bug seems outside the scope of this pull request, so I'd prefer to file an issue for it and deal with it in a separate pull request. If you agree, @glen, let me know so that I can push my proposed revisions and hand review over to you. #### Reproduction Here's one way to reproduce the bug. 1. Load the empty test assembly. 2. Add a single point. 3. Set the regulators for all three of the point's spatial coordinates. - _This produces a constraint problem where all of the point's inversive coordinates are frozen._ During `engine::realize_gram`, an nalgebra call panics with the message "At least one column must be given," which usually means we've tried to construct a matrix from an empty vector of columns.
glen commented 2025-10-08 00:04:44 +00:00
Author
Owner
Copy link

since the PR didn't create the bug, just gave us a way for the first time to trigger it, I have no problem with your filing an issue and merging this without fixing the bug. Thanks.

since the PR didn't create the bug, just gave us a way for the first time to trigger it, I have no problem with your filing an issue and merging this without fixing the bug. Thanks.
Vectornaut added 2 commits 2025-10-09 05:14:29 +00:00 
This makes it simpler, from the programmer's perspective, to get the
name of an axis as a string slice and to format an axis name into a
string. To me, the matching method `Axis::name` seems more direct than
the explicit lookup table that it replaces, and I'm hoping that it will
be about as easy for the compiler to inline, or even easier.

Implementing `Display` enables us to hand an `Axis` to a string
formatter without any explicit conversion. It adds extra code in the
short run, but I'd expect it to simplify our code in the long run by
fitting into the conventions set by the Rust standard library.
Spruce up formatting and error messages
All checks were successful
/ test (pull_request) Successful in 3m43s
Details
adc60ac5c1 
Make the new code's formatting and error messages more consistent with
the previous code. I don't necessarily have a strong preference for the
previous conventions, but I do like stuff to be consistent.
Vectornaut commented 2025-10-09 05:28:06 +00:00
Member
Copy link

since the PR didn't create the bug, just gave us a way for the first time to trigger it, I have no problem with your filing an issue and merging this without fixing the bug.

Great. I just pushed my changes, and you're welcome to merge once you review them! Here's what I changed.

Streamline axis naming

This makes it simpler, from the programmer's perspective, to get the name of an axis as a string slice and to format an axis name into a
string. To me, the matching method Axis::name seems more direct than the explicit lookup table that it replaces, and I'm hoping that it will be equally lightweight from the compiler's perspective. I asked about this task in the Rust user forum. The responses provided some reassurance that my approach is reasonable and likely to generate well-optimized WebAssembly.

Implementing Display enables us to hand an Axis to a string formatter without any explicit conversion. It adds extra code in the short run, but I'd expect it to simplify our code in the long run by fitting into the conventions set by the Rust standard library.

Spruce up formatting and error messages

I've made the new code's formatting and error messages more consistent with the previous code. I don't necessarily have a strong preference for the previous conventions, but I do like stuff to be consistent. Feel free to jot down any formatting complaints so we can do another formatting cleanup pull request at some point.

> since the PR didn't create the bug, just gave us a way for the first time to trigger it, I have no problem with your filing an issue and merging this without fixing the bug. Great. I just pushed my changes, and you're welcome to merge once you review them! Here's what I changed. ### Streamline axis naming This makes it simpler, from the programmer's perspective, to get the name of an axis as a string slice and to format an axis name into a string. To me, the matching method `Axis::name` seems more direct than the explicit lookup table that it replaces, and I'm hoping that it will be equally lightweight from the compiler's perspective. I [asked about this task](https://users.rust-lang.org/t/unit-only-enum-to-string-slice-in-rust-1-86/134498) in the Rust user forum. The responses provided some reassurance that my approach is reasonable and likely to generate well-optimized WebAssembly. Implementing `Display` enables us to hand an `Axis` to a string formatter without any explicit conversion. It adds extra code in the short run, but I'd expect it to simplify our code in the long run by fitting into the conventions set by the Rust standard library. ### Spruce up formatting and error messages I've made the new code's formatting and error messages more consistent with the previous code. I don't necessarily have a strong preference for the previous conventions, but I do like stuff to be consistent. Feel free to jot down any formatting complaints so we can do another formatting cleanup pull request at some point.
glen reviewed 2025-10-09 15:04:31 +00:00
app-proto/src/assembly.rs Outdated
@ -126,3 +128,3 @@
impl Debug for dyn Element {
fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), fmt::Error> {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
glen commented 2025-10-09 15:04:31 +00:00
Author
Owner
Copy link

This change is irrelevant to this PR so we really oughtnt do it but no big deal I guess.

This change is irrelevant to this PR so we really oughtnt do it but no big deal I guess.
Vectornaut commented 2025-10-10 06:28:05 +00:00
Member
Copy link

I'd be fine with leaving it out of this pull request. I made it because this pull request implements Display for Axis using the terser fmt::Result convention, and I wanted the previous implementation of Debug for dyn Element to look consistent.

I'd be fine with leaving it out of this pull request. I made it because this pull request implements `Display` for `Axis` using the terser `fmt::Result` convention, and I wanted the previous implementation of `Debug` for `dyn Element` to look consistent.
glen commented 2025-10-10 17:30:27 +00:00
Author
Owner
Copy link

like I said, no big deal either way, resolving.

like I said, no big deal either way, resolving.
glen marked this conversation as resolved
glen reviewed 2025-10-09 15:06:43 +00:00
app-proto/src/assembly.rs Outdated
@ -500,1 +513,4 @@
#[derive(Clone, Copy, Sequence)]
pub enum Axis { X = 0, Y = 1, Z = 2 }
glen commented 2025-10-09 15:06:43 +00:00
Author
Owner
Copy link

I generally prefer to punctuate code as one would text, when possible. Can we remove the two extraneous spaces you just added here? They don't seem to improve readability (as we are all used to no space between brackets and content).

I generally prefer to punctuate code as one would text, when possible. Can we remove the two extraneous spaces you just added here? They don't seem to improve readability (as we are all used to no space between brackets and content).
Vectornaut commented 2025-10-10 06:01:39 +00:00
Member
Copy link

This spacing convention is widely used in Rust code, and it's recommended in the Rust style guide:

For a single-line block, put spaces after the opening brace and before the closing brace.

I like it a lot, and I've used it in hundreds of places in the dyna3 source code. If you're adamant about changing it, I'd prefer to do it throughout the source code in a formatting-only pull request.

This spacing convention is widely used in Rust code, and it's [recommended](https://doc.rust-lang.org/beta/style-guide/expressions.html#blocks) in the Rust style guide: > For a single-line block, put spaces after the opening brace and before the closing brace. I like it a lot, and I've used it in hundreds of places in the dyna3 source code. If you're adamant about changing it, I'd prefer to do it throughout the source code in a formatting-only pull request.
glen commented 2025-10-10 17:29:48 +00:00
Author
Owner
Copy link

We don't have to fix it now. I don't care at all about the Rust style guide, it's an assembly of weird rigidities from my point of view (what business does a language have semi-enforcing snake_case identifiers? I thought typography reiterating semantic category went into the trashbin with Perl a decade or more ago...). I very much like the convention of punctuating code as close as possible to regular text. I get it that you are used to these spaces that look very extra to me. We'll figure out how to resolve this at some point. No need to spin our wheels on it at the moment.

We don't have to fix it now. I don't care at all about the Rust style guide, it's an assembly of weird rigidities from my point of view (what business does a _language_ have semi-enforcing snake_case identifiers? I thought typography reiterating semantic category went into the trashbin with Perl a decade or more ago...). I very much like the convention of punctuating code as close as possible to regular text. I get it that you are used to these spaces that look very extra to me. We'll figure out how to resolve this at some point. No need to spin our wheels on it at the moment.
👍 1
glen marked this conversation as resolved
glen commented 2025-10-09 15:26:38 +00:00
Author
Owner
Copy link

I don't think it will be any surprise to you that my preference would be just to revert your "spruce up" commit. In my eyes, essentially all of its changes increase verbosity and dilute information (note it nearly doubles the lines of code), hence reduce readability. Unnecessary new temporaries are introduced, increasing cognitive load. In your PRs, I have definitely reviewed things like structure names, and I may have made a snide comment or two about very long variable names (sorry if so) but I don't think I've ever requested a change to specific variable name (apologies if I am misremembering about that).

Any chance we could just unwind that commit, and then if there are a handful of individual formatting things that you think are particularly problematic, you enter them as review comments for me to address, rather than reformatting the code I wrote wholesale? Or how would you like to handle this sort of difference of opinion? (I am sorry that I haven't had time to devote to Husht. I very much hope to get there...)

I don't think it will be any surprise to you that my preference would be just to revert your "spruce up" commit. In my eyes, essentially all of its changes increase verbosity and dilute information (note it nearly doubles the lines of code), hence reduce readability. Unnecessary new temporaries are introduced, increasing cognitive load. In your PRs, I have definitely reviewed things like structure names, and I may have made a snide comment or two about very long variable names (sorry if so) but I don't _think_ I've ever requested a change to specific variable name (apologies if I am misremembering about that). Any chance we could just unwind that commit, and then if there are a handful of individual formatting things that you think are particularly problematic, you enter them as review comments for me to address, rather than reformatting the code I wrote wholesale? Or how would you like to handle this sort of difference of opinion? (I am sorry that I haven't had time to devote to Husht. I very much hope to get there...)
glen commented 2025-10-09 15:30:22 +00:00
Author
Owner
Copy link

@Vectornaut wrote in #118 (comment):

This makes it simpler, from the programmer's perspective, to get the name of an axis as a string slice and to format an axis name into a
string. To me, the matching method Axis::name seems more direct than the explicit lookup table that it replaces,

The using end of the Axis naming changes is quite nice, thank you very much! The implementation detail is not too critical either way, as it is now nicely encapsulated; the "match" code is much more verbose than a three-element array, so I would never think of doing it that way, but I have no objection.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3312: > This makes it simpler, from the programmer's perspective, to get the name of an axis as a string slice and to format an axis name into a > string. To me, the matching method `Axis::name` seems more direct than the explicit lookup table that it replaces, The using end of the Axis naming changes is quite nice, thank you very much! The implementation detail is not too critical either way, as it is now nicely encapsulated; the "match" code is much more verbose than a three-element array, so I would never think of doing it that way, but I have no objection.
👍 1
Vectornaut commented 2025-10-10 05:44:48 +00:00
Member
Copy link

Any chance we could just unwind that commit, and then if there are a handful of individual formatting things that you think are particularly problematic, you enter them as review comments for me to address, rather than reformatting the code I wrote wholesale?

That's fine. I'll open review discussions for the error message and formatting changes that I'd most strongly like to apply from that commit. This will leave some formatting inconsistencies in the source code. Should I record them somewhere so we can resolve them later?

> Any chance we could just unwind that commit, and then if there are a handful of individual formatting things that you think are particularly problematic, you enter them as review comments for me to address, rather than reformatting the code I wrote wholesale? That's fine. I'll open review discussions for the error message and formatting changes that I'd most strongly like to apply from that commit. This will leave some formatting inconsistencies in the source code. Should I record them somewhere so we can resolve them later?
Vectornaut added 1 commit 2025-10-10 05:45:05 +00:00
Revert "Spruce up formatting and error messages"
All checks were successful
/ test (pull_request) Successful in 3m40s
Details
c081f1a809 
This reverts commit adc60ac5c1. We decided
that it would be better for me to request formatting changes one by one.
Vectornaut requested changes 2025-10-10 06:21:26 +00:00
Vectornaut left a comment
Member
Copy link

Here are the error message and formatting changes that I'd most strongly like to apply from my reverted "spruce up" commit.

Here are the error message and formatting changes that I'd most strongly like to apply from my reverted "spruce up" commit.
app-proto/src/assembly.rs Outdated
@ -302,6 +305,15 @@ impl Element for Point {
point(0.0, 0.0, 0.0),
)
}
Vectornaut commented 2025-10-10 05:53:03 +00:00
Member
Copy link

Our current convention is that all the lines in a block, including blank lines, should have the block indentation. Under that convention, this blank line would need to be indented by four spaces, just like the lines above it, below it, and elsewhere in the impl block.

Our current convention is that all the lines in a block, including blank lines, should have the block indentation. Under that convention, this blank line would need to be indented by four spaces, just like the lines above it, below it, and elsewhere in the `impl` block.
glen commented 2025-10-10 15:29:34 +00:00
Author
Owner
Copy link

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). I will go ahead and remove all trailing whitespace for consistency in an immediately following PR.

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). I will go ahead and remove all trailing whitespace for consistency in an immediately following PR.
Vectornaut commented 2025-10-10 22:49:25 +00:00
Member
Copy link

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line).

This is the convention the Rust style guide recommends, so I'm fine with it, even though it's not my favorite.

I have a slight preference for keeping this pull request consistent with the existing style, and removing all the trailing whitespace at once in the upcoming pull request.

> A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). This is the convention the [Rust style guide](https://doc.rust-lang.org/beta/style-guide/index.html#trailing-whitespace) recommends, so I'm fine with it, even though it's not my favorite. I have a slight preference for keeping this pull request consistent with the existing style, and removing all the trailing whitespace at once in the upcoming pull request.
app-proto/src/assembly.rs Outdated
@ -501,0 +558,4 @@
self.set_point.with_untracked(|set_pt| {
if let Some(val) = set_pt.value {
let col = self.subject.column_index().expect(
"Subject must be indexed before point-coordinate regulator poses.");
Vectornaut commented 2025-10-10 06:20:18 +00:00
Member
Copy link

For consistency with InversiveDistanceRegulator, I'd change the panic message to:

Subject should be indexed before point coordinate regulator writes problem data

For consistency with `InversiveDistanceRegulator`, I'd change the panic message to: > Subject should be indexed before point coordinate regulator writes problem data
glen commented 2025-10-10 15:30:51 +00:00
Author
Owner
Copy link

As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".

As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".
👍 1
glen commented 2025-10-10 17:35:03 +00:00
Author
Owner
Copy link

Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).

Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).
Vectornaut commented 2025-10-10 22:40:11 +00:00
Member
Copy link

The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes.

  • Add more detail to the helper name: maybe something like index_required_message, missing_index_panic_message, or index_before_posing_message?
  • Rename the thing argument to something like role or kind_of_thing. Or, more broadly, name the arguments something like object_kind, object_name, actor?

As far as I can see from the code, "must" is correct.

Agreed.

The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes. - Add more detail to the helper name: maybe something like `index_required_message`, `missing_index_panic_message`, or `index_before_posing_message`? - Rename the `thing` argument to something like `role` or `kind_of_thing`. Or, more broadly, name the arguments something like `object_kind`, `object_name`, `actor`? > As far as I can see from the code, "must" is correct. Agreed.
Vectornaut commented 2025-10-10 22:43:51 +00:00
Member
Copy link

To me, the placement of the new helper is kind of weird to me. It's right in the middle of the Sphere definition and implementation section, which makes it seem associated with Sphere, when in fact it's relevant to many problem-posers. It might fit better above the definition of the ProblemPoser trait, although I'd be open to other ideas about where to put it.

To me, the placement of the new helper is kind of weird to me. It's right in the middle of the `Sphere` definition and implementation section, which makes it seem associated with `Sphere`, when in fact it's relevant to many problem-posers. It might fit better above the definition of the `ProblemPoser` trait, although I'd be open to other ideas about where to put it.
glen commented 2025-10-10 23:15:46 +00:00
Author
Owner
Copy link

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.
Vectornaut commented 2025-10-10 23:35:30 +00:00
Member
Copy link

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it.

Great!

Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject.

The main thing I'd like to clarify, if possible, is that thing and name go together, and that thing is a type or role, while name is an identifier.

> Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Great! > Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. The main thing I'd like to clarify, if possible, is that `thing` and `name` go together, and that `thing` is a type or role, while `name` is an identifier.
app-proto/src/assembly.rs Outdated
@ -501,0 +561,4 @@
"Subject must be indexed before point-coordinate regulator poses.");
problem.frozen.push(self.axis as usize, col, val);
// Check if all three spatial coordinates have been frozen, and if so,
// freeze the norm component as well
Vectornaut commented 2025-10-10 06:16:29 +00:00
Member
Copy link

I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed.

I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment.

I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like.

// if all three of the subject's spatial coordinates have been
// frozen, then freeze its norm component too

(I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)

I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed. I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment. I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like. ``` // if all three of the subject's spatial coordinates have been // frozen, then freeze its norm component too ``` (I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)
glen commented 2025-10-10 15:32:30 +00:00
Author
Owner
Copy link

I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.

I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.
glen commented 2025-10-10 16:02:49 +00:00
Author
Owner
Copy link

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.
Vectornaut commented 2025-10-10 22:07:16 +00:00
Member
Copy link

Thanks for reflowing and rewording the comment!

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style.

The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the line width and comment width rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.

Thanks for reflowing and rewording the comment! > Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them. I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style. The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the [line width](https://doc.rust-lang.org/beta/style-guide/index.html#indentation-and-line-width) and [comment width](https://doc.rust-lang.org/beta/style-guide/index.html#comments) rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.
glen commented 2025-10-10 22:57:11 +00:00
Author
Owner
Copy link

I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?
Vectornaut commented 2025-10-10 23:05:52 +00:00
Member
Copy link

I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.

> I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok? Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.
glen commented 2025-10-11 01:22:49 +00:00
Author
Owner
Copy link

The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks.

As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment:

  • switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot.
  • switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g.
fn myfn(arg: Type) {
   if condition {
      take_action(arg, other, long,
         arguments, in, list)}}

fn my otherfn() -> ReturnType {
   ...
The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks. As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment: * switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot. * switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g. ``` fn myfn(arg: Type) { if condition { take_action(arg, other, long, arguments, in, list)}} fn my otherfn() -> ReturnType { ... ```
Vectornaut commented 2025-10-11 02:12:05 +00:00
Member
Copy link

So I'd like to just stick with 80, thanks.

Sounds good!

In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment

Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.

> So I'd like to just stick with 80, thanks. Sounds good! > In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.
Vectornaut marked this conversation as resolved
Vectornaut requested changes 2025-10-10 06:41:34 +00:00
app-proto/src/assembly.rs Outdated
@ -501,0 +519,4 @@
}
}
impl fmt::Display for Axis {
Vectornaut commented 2025-10-10 06:41:24 +00:00
Member
Copy link

Whoops, here's a formatting mistake from the revision I made. I should've just written

impl Display for Axis

and changed the relevant use expression as follows:

- fmt::{Debug, Formatter}
+ fmt::{Debug, Display, Formatter}

Would you mind including that change in your next round of revisions?

Whoops, here's a formatting mistake from the revision I made. I should've just written ```rust impl Display for Axis ``` and changed the relevant `use` expression as follows: ```diff - fmt::{Debug, Formatter} + fmt::{Debug, Display, Formatter} ``` Would you mind including that change in your next round of revisions?
glen commented 2025-10-10 15:24:31 +00:00
Author
Owner
Copy link

Yes of course, and this makes sense.

Yes of course, and this makes sense.
Vectornaut marked this conversation as resolved
glen added 1 commit 2025-10-10 16:40:14 +00:00
chore: uniformize error messages, 80-char lines, import fmt::Display
All checks were successful
/ test (pull_request) Successful in 3m48s
Details
c0e6ebf3d6
glen commented 2025-10-10 17:32:23 +00:00
Author
Owner
Copy link

Ok, the latest commit makes all changes per my comments above, and I have filed #126 which removes trailing whitespace, and incidentally adds final newlines so that we stop getting the "/no final newline" messages in git diff. Ball's back in your court.

Ok, the latest commit makes all changes per my comments above, and I have filed #126 which removes trailing whitespace, and incidentally adds final newlines so that we stop getting the "/no final newline" messages in `git diff`. Ball's back in your court.
glen commented 2025-10-10 17:38:06 +00:00
Author
Owner
Copy link

Oh and while I was working on this I noticed that trunk was out of date on my machine. It took me a while to figure out how to deal with it, so I made a couple small changes in the README with clues about that. I do realize this is breaking my own rule of an irrelevant change, but I feel like this change is sort of too small to bother with its own PR and moreover this PR has already become a bit of a mishmosh of the functionality change and some formatting changes so I wasn't adding much harm. If you'd like I can revert the README change and make a separate PR for it.

Oh and while I was working on this I noticed that trunk was out of date on my machine. It took me a while to figure out how to deal with it, so I made a couple small changes in the README with clues about that. I do realize this is breaking my own rule of an irrelevant change, but I feel like this change is sort of too small to bother with its own PR and moreover this PR has already become a bit of a mishmosh of the functionality change and some formatting changes so I wasn't adding much harm. If you'd like I can revert the README change and make a separate PR for it.
Vectornaut commented 2025-10-10 22:15:44 +00:00
Member
Copy link

@glen wrote in #118 (comment):

It took me a while to figure out how to deal with it, so I made a couple small changes in the README with clues about that.

This is a pretty small change, and it's done in a file that's otherwise untouched, so I'm okay with throwing it in here.

[…] moreover this PR has already become a bit of a mishmosh of the functionality change and some formatting changes […]

Like I say above, I'd prefer to keep this pull request mostly focused on behavior changes. I did make one formatting change for consistency with new code, and would be willing to accept a handful of similar small changes, but I'd rather save large-scale reformatting for its own pull request.

@glen wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3343: > It took me a while to figure out how to deal with it, so I made a couple small changes in the README with clues about that. This is a pretty small change, and it's done in a file that's otherwise untouched, so I'm okay with throwing it in here. > […] moreover this PR has already become a bit of a mishmosh of the functionality change and some formatting changes […] Like I say [above](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3344), I'd prefer to keep this pull request mostly focused on behavior changes. I did make [one formatting change](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3326) for consistency with new code, and would be willing to accept a handful of similar small changes, but I'd rather save large-scale reformatting for its own pull request.
Vectornaut requested changes 2025-10-10 22:24:31 +00:00
README.md Outdated
@ -33,1 +33,4 @@
5. Call `cargo install trunk` to install the [Trunk](https://trunkrs.dev/) web-build tool
- In the future, `trunk` can be updated with the same command. You may
need the `--locked` flag if your ambient version of `rustc` does not
match that required by `trunk`.
Vectornaut commented 2025-10-10 22:21:54 +00:00
Member
Copy link

The information here looks accurate to me, although I've never installed a global tool with the --locked flag, and I don't know whether it's recommended.

The existing style for this file does not use manual line-wrapping. I'd prefer to keep the new text consistent with that style by putting it all on one line.

The information here looks accurate to me, although I've never installed a global tool with the `--locked` flag, and I don't know whether it's recommended. The existing style for this file does not use manual line-wrapping. I'd prefer to keep the new text consistent with that style by putting it all on one line.
glen commented 2025-10-10 22:48:38 +00:00
Author
Owner
Copy link

Ok, I will go ahead and remove the newlines in the additions to this file, but i will say that trying to impose formatting rules like this on markdown files feels rather contradictory to the spirit/genesis of markdown: it's an explicit reaction to various markup languages to be more free-form and lightweight.

Ok, I will go ahead and remove the newlines in the additions to this file, but i will say that trying to impose formatting rules like this on markdown files feels rather contradictory to the spirit/genesis of markdown: it's an explicit reaction to various markup languages to be more free-form and lightweight.
glen commented 2025-10-10 22:49:47 +00:00
Author
Owner
Copy link

--locked is in fact now part of the installation command that trunk itself recommends, so i feel pretty safe recommending it here.

`--locked` is in fact now part of the installation command that trunk itself recommends, so i feel pretty safe recommending it here.
👍 1
Vectornaut marked this conversation as resolved
README.md Outdated
@ -35,2 +38,4 @@
- This lets you call Trunk, and other tools installed by Cargo, without specifying their paths
- On POSIX systems, the search path is stored in the `PATH` environment variable
- Alternatively, if you don't want to adjust your `PATH`, you can install
`trunk` in another directory `DIR` via `cargo install --root DIR trunk`
Vectornaut commented 2025-10-10 22:24:23 +00:00
Member
Copy link

I'll trust your experience with installing Cargo tools at custom locations, since I've never tried it!

The request above about manual line-wrapping also applies here.

I'll trust your experience with installing Cargo tools at custom locations, since I've never tried it! The [request](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3346) above about manual line-wrapping also applies here.
glen added 1 commit 2025-10-11 01:06:34 +00:00
chore: revert 80-character limit, reorder and rename index message helper
All checks were successful
/ test (pull_request) Successful in 3m38s
Details
50c51ca08f
glen commented 2025-10-11 01:27:04 +00:00
Author
Owner
Copy link

OK, I think I have reverted what needs reverting and made the agreed-upon changes. I have updated the trailing-whitespace PR. So those are back in your court. I will get started on an 80-character PR on top of the trailing-whitespace PR.

OK, I think I have reverted what needs reverting and made the agreed-upon changes. I have updated the trailing-whitespace PR. So those are back in your court. I will get started on an 80-character PR on top of the trailing-whitespace PR.
Vectornaut requested changes 2025-10-11 02:16:46 +00:00
Vectornaut left a comment
Member
Copy link

Okay, this looks mostly finished to me! I just have one question and one small request involving the reformatting in commit 50c51ca.

Okay, this looks mostly finished to me! I just have one question and one small request involving the reformatting in commit 50c51ca.
app-proto/src/assembly.rs Outdated
@ -602,3 +691,1 @@
let can_insert = self.elements_by_id.with_untracked(
|elts_by_id| !elts_by_id.contains_key(elt.id())
);
let can_insert = self.elements_by_id.with_untracked(|elts_by_id| !elts_by_id.contains_key(elt.id()));
Vectornaut commented 2025-10-11 01:44:20 +00:00
Member
Copy link

Flagging this as a potential mistake because it adds a new change to the pull request, rather than reverting one of the changes in commit c0e6ebf, and I don't see an obvious reason for this pull request to touch it. Could you confirm that this change was deliberate if it was, and revert it if it wasn't?

Flagging this as a potential mistake because it adds a new change to the pull request, rather than reverting one of the changes in commit c0e6ebf, and I don't see an obvious reason for this pull request to touch it. Could you confirm that this change was deliberate if it was, and revert it if it wasn't?
glen commented 2025-10-11 05:53:28 +00:00
Author
Owner
Copy link

Oh, when I was undoing, there was some exactly analogous line that was the other way, so it was easy just to falsely revert this. I will fix the mistaken non-reversion.

Oh, when I was undoing, there was some exactly analogous line that was the other way, so it was easy just to falsely revert this. I will fix the mistaken non-reversion.
app-proto/src/assembly.rs Outdated
@ -868,2 +957,3 @@
#[test]
#[should_panic(expected = "Subjects should be indexed before inversive distance regulator writes problem data")]
#[should_panic(expected = "Subject \"sphere1\" must be indexed before \
inversive distance regulator writes problem data")]
Vectornaut commented 2025-10-11 01:51:37 +00:00
Member
Copy link

I'd indent this line by two tabs to match the convention you've used for other multi-line expressions. This won't affect the string, because the string continuation escape absorbs all subsequent whitespace, resuming the string at the next non-whitespace character.

I'd indent this line by two tabs to match the convention you've used for other multi-line expressions. This won't affect the string, because the [string continuation escape](https://doc.rust-lang.org/reference/expressions/literal-expr.html#string-continuation-escapes) absorbs all subsequent whitespace, resuming the string at the next non-whitespace character.
glen commented 2025-10-11 05:51:36 +00:00
Author
Owner
Copy link

Ah, didn't know that \ at end of line (a convention I hate, but apparently the only one available in this context) sucked up leading whitespace on the next line as well. Will do.

Ah, didn't know that \ at end of line (a convention I hate, but apparently the only one available in this context) sucked up leading whitespace on the next line as well. Will do.
Vectornaut approved these changes 2025-10-11 02:24:31 +00:00
Vectornaut left a comment
Member
Copy link

Noticed one last minor formatting inconsistency.

Noticed one last minor formatting inconsistency.
README.md Outdated
@ -31,9 +31,11 @@ The latest prototype is in the folder `app-proto`. It includes both a user inter
3. Call `rustup target add wasm32-unknown-unknown` to add the [most generic 32-bit WebAssembly target](https://doc.rust-lang.org/nightly/rustc/platform-support/wasm32-unknown-unknown.html)
4. Call `cargo install wasm-pack` to install the [WebAssembly toolchain](https://rustwasm.github.io/docs/wasm-pack/)
5. Call `cargo install trunk` to install the [Trunk](https://trunkrs.dev/) web-build tool
- In the future, `trunk` can be updated with the same command. You may need the `--locked` flag if your ambient version of `rustc` does not match that required by `trunk`.
Vectornaut commented 2025-10-11 02:24:12 +00:00
Member
Copy link

Very minor: the rest of the list is formatted as one sentence per list item, with no final period. It would be nice to keep that consistent.

Very minor: the rest of the list is formatted as one sentence per list item, with no final period. It would be nice to keep that consistent.
glen commented 2025-10-11 05:49:59 +00:00
Author
Owner
Copy link

OK, I have properly punctuated the whole file, so that complete sentences have periods, and fragments do not.

OK, I have properly punctuated the whole file, so that complete sentences have periods, and fragments do not.
Vectornaut commented 2025-10-11 02:28:09 +00:00
Member
Copy link

Whoops, this was meant to be an additional request for changes, not an approval.

Whoops, [this](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3369) was meant to be an additional request for changes, not an approval.
glen added 1 commit 2025-10-11 05:49:14 +00:00
chore: Properly punctuate README
All checks were successful
/ test (pull_request) Successful in 3m38s
Details
627cea455c
glen added 1 commit 2025-10-11 05:58:25 +00:00
chore: Hopefully final formatting items from review
All checks were successful
/ test (pull_request) Successful in 3m36s
Details
6dbbe2ce2d
glen commented 2025-10-11 06:01:53 +00:00
Author
Owner
Copy link

OK, made those last couple of formatting changes you requested, and rebased the no-trailing-whitespace and wrap-80-chars PRs, so over to you :)

OK, made those last couple of formatting changes you requested, and rebased the no-trailing-whitespace and wrap-80-chars PRs, so over to you :)
Vectornaut approved these changes 2025-10-13 22:47:58 +00:00
Vectornaut left a comment
Member
Copy link

This looks ready to go! Excited to have these new regulators available. I've added a few last comments for future reference.

This looks ready to go! Excited to have these new regulators available. I've added a few last comments for future reference.
README.md
@ -13,3 +13,3 @@
### Implementation goals
* Comfortable, intuitive UI
* Provide a comfortable, intuitive UI
Vectornaut commented 2025-10-13 22:34:01 +00:00
Member
Copy link

I like the way you've added explicit verbs to these bullet points!

I like the way you've added explicit verbs to these bullet points!
glen commented 2025-10-13 23:06:16 +00:00
Author
Owner
Copy link

Just tryin' to keep things parallel :)

Just tryin' to keep things parallel :)
Vectornaut marked this conversation as resolved
app-proto/src/assembly.rs
@ -501,0 +571,4 @@
// frozen, then freeze its norm component:
let mut coords = [0.0; Axis::CARDINALITY];
let mut nset: usize = 0;
for &MatrixEntry {index, value} in &(problem.frozen) {
Vectornaut commented 2025-10-13 22:35:51 +00:00
Member
Copy link

Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as { index, value } elsewhere, but during review we discussed potentially changing this convention in the future.

Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as `{ index, value }` elsewhere, but during review we discussed potentially changing this convention in the future.
glen commented 2025-10-13 23:08:12 +00:00
Author
Owner
Copy link

It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.

It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.
👍 1
Vectornaut marked this conversation as resolved
app-proto/src/assembly.rs
@ -868,2 +959,3 @@
#[test]
#[should_panic(expected = "Subjects should be indexed before inversive distance regulator writes problem data")]
#[should_panic(expected = "Subject \"sphere1\" must be indexed before \
inversive distance regulator writes problem data")]
Vectornaut commented 2025-10-13 22:42:37 +00:00
Member
Copy link

When I originally wrote this test, it bothered me that the identifiers sphere0 and sphere1 were assigned but never tested. The new, more informative indexing error message makes them part of the test, which is very satisfying.

When I originally wrote this test, it bothered me that the identifiers `sphere0` and `sphere1` were assigned but never tested. The new, more informative indexing error message makes them part of the test, which is very satisfying.
Vectornaut marked this conversation as resolved
Vectornaut merged commit 2c8c09d20d into main 2025-10-13 22:52:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Vectornaut
Labels
Clear labels   
bug

Something is not working

  
design

Issue that entails architecture/design decisions

  
duplicate

This issue or pull request already exists

  
engine

Issue concerning the engine

  
enhancement

New feature

  
maintenance

Refactors andother behavior-orthogonal tasks

  
prospective

This will become 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
engine
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#118
No description provided.
Delete branch "glen/dyna3:pointCoordRegulators"

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?