feat: Point coordinate regulators #118
|
|
@ -514,8 +514,7 @@ impl ProblemPoser for HalfCurvatureRegulator {
|
|||
pub enum Axis {X = 0, Y = 1, Z = 2}
|
||||
|
||||
impl Axis {
|
||||
|
glen marked this conversation as resolved
Outdated
|
||||
pub const N_AXIS: usize = (Axis::Z as usize) + 1;
|
||||
pub const NAME: [&str; Axis::N_AXIS] = ["X", "Y", "Z"];
|
||||
pub const NAME: [&str; Axis::CARDINALITY] = ["X", "Y", "Z"];
|
||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
The associated constant 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`.
|
||||
}
|
||||
|
||||
pub struct PointCoordinateRegulator {
|
||||
|
|
@ -553,17 +552,17 @@ impl ProblemPoser for PointCoordinateRegulator {
|
|||
let col = self.subject.column_index().expect(
|
||||
"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
|
||||
let mut coords = [0.0; Axis::N_AXIS];
|
||||
// Check if all three spatial coordinates have been frozen, and if so,
|
||||
// freeze the norm component as well
|
||||
let mut coords = [0.0; Axis::CARDINALITY];
|
||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
DesignThis kind of interaction between problem posers is something we didn't anticipate when designing the TestingTo 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 with I tried this out locally, and it confirms that the interaction system is working in at least some cases. CommentingI'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 ### 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
@Vectornaut wrote in #118 (comment):
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
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
I agree that the comment in issue #90 is enough to remind us about this.
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.
|
||||
let mut nset: usize = 0;
|
||||
for &MatrixEntry {index, value} in &(problem.frozen) {
|
||||
if index.1 == col && index.0 < Axis::N_AXIS {
|
||||
if index.1 == col && index.0 < Axis::CARDINALITY {
|
||||
nset += 1;
|
||||
|
Vectornaut
commented
For consistency with
For consistency with `InversiveDistanceRegulator`, I'd change the panic message to:
> Subject should be indexed before point coordinate regulator writes problem data
glen
commented
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".
glen
commented
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
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.
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
To me, the placement of the new helper is kind of weird to me. It's right in the middle of the 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
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
Great!
The main thing I'd like to clarify, if possible, is that > 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.
|
||||
coords[index.0] = value
|
||||
}
|
||||
}
|
||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
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. (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
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
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
Thanks for reflowing and rewording the comment!
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 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
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
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
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:
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
Sounds good!
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.
|
||||
if nset == Axis::N_AXIS {
|
||||
if nset == Axis::CARDINALITY {
|
||||
let [x, y, z] = coords;
|
||||
problem.frozen.push(
|
||||
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
|
||||
|
|
@ -773,6 +772,7 @@ impl Assembly {
|
|||
/* DEBUG */
|
||||
// log the Gram matrix
|
||||
console_log!("Gram matrix:\n{}", problem.gram);
|
||||
console_log!("Frozen entries:\n{}", problem.frozen);
|
||||
|
||||
/* DEBUG */
|
||||
// log the initial configuration matrix
|
||||
|
|
|
|||
|
|
@ -123,10 +123,11 @@ impl OutlineItem for HalfCurvatureRegulator {
|
|||
|
||||
impl OutlineItem for PointCoordinateRegulator {
|
||||
fn outline_item(self: Rc<Self>, _element: &Rc<dyn Element>) -> View {
|
||||
let name = format!("{} coordinate", Axis::NAME[self.axis as usize]);
|
||||
view! {
|
||||
li(class = "regulator") {
|
||||
div(class = "regulator-label") { (Axis::NAME[self.axis as usize]) }
|
||||
div(class = "regulator-type") { "Coordinate" }
|
||||
div(class = "regulator-label") // for spacing
|
||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
In the outline items for previously implemented regulators, we use the 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
done in latest commit done in latest commit
Vectornaut
commented
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.
|
||||
div(class = "regulator-type") { (name) }
|
||||
RegulatorInput(regulator = self)
|
||||
div(class = "status")
|
||||
}
|
||||
|
|
|
|||
|
|
@ -46,7 +46,7 @@ pub fn project_sphere_to_normalized(rep: &mut DVector<f64>) {
|
|||
|
||||
// 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]); //FIXME: This 3 should be Point::WEIGHT_COMPONENT
|
||||
rep.scale_mut(0.5 / rep[3]);
|
||||
|
Vectornaut marked this conversation as resolved
Outdated
Vectornaut
commented
I've been flagging desired edits like this with 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
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
I'd prefer to just add a
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 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 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
This is now covered by issue #120. Thanks for posting that! This is now covered by issue #120. Thanks for posting that!
|
||||
}
|
||||
|
||||
// --- partial matrices ---
|
||||
|
|
|
|||
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).
This spacing convention is widely used in Rust code, and it's recommended in the Rust style guide:
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.
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.