chore: wrap at 80 characters #128
No reviewers
Labels
No labels
bug
design
duplicate
engine
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: StudioInfinity/dyna3#128
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "glen/dyna3:aroundLineInEightyChars"
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?
This is built on top of #126. It simply wraps all lines in app-proto/src at 80 characters.
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.)2f6c93ec42to19e5458e1b19e5458e1btodde499b736dde499b736tofae94486d6This should now be properly rebased on #129.
fae94486d6to4a3f47c8b5Now rebased onto main. @Vectornaut had a couple of in-person comments that I don't remember the details of, which could be posted now, before or in conjunction with a full review.
Aside from two changes that correct naming errors, which I've flagged with an ❌️ at the beginning of each comment, all of my suggestions are stylistic.
When a parenthesized expression is split over two lines, I still prefer something like the first option below rather than the second.
However, I think you've already said you strongly prefer the second option, which is used in most of the pull request. If so, there's no need to discuss it further.
@ -359,2 +368,3 @@const FOCAL_SLOPE: f64 = 0.3;let horizontal = f64::from(event.client_x()) - rect.left();let vertical = rect.bottom() - f64::from(event.client_y());I think names like
x_relative,y_relativeorx_rel,y_relwould be more descriptive, and more analogous with the original valuesevent.client_x(),event.client_y(), than the current nameshorizontal,vertical.Renamed to x_relative, y_relative.
@ -918,1 +960,3 @@match assembly_to_world.with(|asm_to_world| elt.cast(dir, asm_to_world, pixel_size)) {let target = assembly_to_world.with(|asm_to_world| elt.cast(dir, asm_to_world, pixel_size));match target {❌️ I'd call this value something like
hit_depthrather thantarget, because it doesn't tell you what you hit: it only tells you whether you hitelt, and how far out along the click ray you hit it if you did.Renamed to just
hit, as the depth is immediately extracted on the next line, and then theNonebranch makes more sense: no hit at all, not just a missing depth of a hit.In ray-casting, I'd typically use
hitto denote the full coordinates of the hit point. If that changes your mind about the clarity ofhithere, we can workshop a different name. If you still thinkhitis clear enough here, I'm willing to accept it.check_hit?
How about
depth_if_hit? To me, this communicates that the value will beSome(depth)if we hiteltandNoneif we missed.since the method ultimately being called is named
cast, I just called the resultcast.@ -221,0 +227,4 @@let x32 = x as f32;let y32 = y as f32;let coords =[0.5*(1.0 + x32), 0.5*(1.0 + y32), 0.5*(1.0 - x32*y32)];❌️ This is the sphere's color, not its representation coordinates. I'd call it
color, matching the name of theSphere::newargument that it gets plugged into later.Renamed to color.
@ -509,2 +533,3 @@// set up the tangencies that define the packingfor [long_edge_plane, short_edge_plane] in [["a", "b"], ["b", "c"], ["c", "a"]] {for [long_edge_plane, short_edge_plane]in [["a", "b"], ["b", "c"], ["c", "a"]] {I strongly prefer to put the opening
{at the same indentation level as thefor, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Either of the following would look fine to me.Tried to recast all of these accordingly. This is something we will have to give some thought to in Husht, how to delimit a multiline loop control portion from the loop body, if there are no braces.
The Python style guide has some recommendations for how to deal with this in
ifstatements.Right well lookjng there you find three options. One is do nothing, which necessitates a delimiter between condition and consequent, which doesn't seem feasible for husht (get rid of braces just to introduce a new delimiter?), another is introduce a comment, but i think we both dislike semantically significant comments ;-), and the third is the extra-indent-in-condition convention i tried but you didn't like. Hence how to do this in Husht will require real thought.
I checked the civet documentation and based on that here are two options, not mutually exclusive, nor would have to go the same way for both conditionals and loops (civet uses the first for loops and the second for conditionals):
Seems like some combination of these schemes would work for husht.
@ -862,2 +898,3 @@);for (chain_sphere, chain_sphere_next) in chain.clone().zip(chain.cycle().skip(1)) {for (chain_sphere, chain_sphere_next)in chain.clone().zip(chain.cycle().skip(1)) {I strongly prefer to put the opening
{at the same indentation level as thefor, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Here's an example.This is a ditto. There were several others as well.
Yes, looks like you got them all!
@ -26,1 +29,3 @@pub fn sphere_with_offset(dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64) -> DVector<f64> {pub fn sphere_with_offset(dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64) -> DVector<f64>{Personally, I prefer the following organization, which takes up the same number of lines.
I prefer it despite its potential inconsistency with the first of the following options for splitting function calls, which does save a line and which is used elsewhere in this pull request.
If you strongly prefer consistency, though, I'll defer to that.
This comment also applies to several other function declarations, which I can highlight if you accept the change.
Tried to go through and reformat accordingly. This may also be something that requires thought in Husht, along with multi-line "if" conditions.
Thanks! I've flagged a few missed occurrences in the next review.
@ -512,1 +526,3 @@Ok(ConfigNeighborhood { #[cfg(feature = "dev")] config: state.config, nbhd: tangent })Ok(ConfigNeighborhood {#[cfg(feature = "dev")] config: state.config, nbhd: tangent})If we're going to split this over multiple lines, I think it's worth an extra line to make the structure more readable.
Reformatted as suggested
@ -941,0 +971,4 @@let Realization { result: result_tfm, history: history_tfm } =realize_gram(&problem_tfm, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110);let ConfigNeighborhood { config: config_tfm, nbhd: tangent_tfm } =result_tfm.unwrap();Another option, which I find more readable:
Wasn't quite clear about what the layout/readability issue was here. I definitely prefer keeping the entity being assigned to (it's unfortunate it's just so verbose here) as compact as possible. I tried a different tweak.
Your alternate tweak looks fine to me.
@Vectornaut wrote in #128 (comment):
Well, it pretty well corresponds: I like letting the indentation show the grouping and I prefer not to use up a vertical line solely for punctuation, whereas I gather you are very comfortable with the predominant style of putting closing punctuation marks alone (or sometimes in small combinations) on a line at the same indentation as the line on which the opening punctuation marks occurred (regardless of where on that line the opening mark appeared, typically at the end of that earlier line. (And also that parenthesis all by itself, forlorn on its own line, is so far from how we punctuate ordinary discourse that it just looks to me jarring and dangle-y.)
A reasonable question is how will this be handled by Husht, presuming I can eventually carve out time to move that further? I don't think we've at any point contemplated doing away with function-call parentheses, so this would presumably (run all onto one line) end up as something like
or in other words, the actual code won't change much (I mean, we could at some point add convenience unary-function-writing features like Civet has to end up with something like
but that would be a significantly down-the-road addition that's not really relevant at the moment, other than to note that the savings of not naming the sole parameter of the function argument fits the whole snippet in 80 characters.)
OK, so even in currently foreseeable Husht, we will have to break the line, and so may have a quandary concerning where the close goes. We should find a mutually comfortable way to handle such cases, insofar as possible. Brainstorming:
A) Should we prefer linebreaks that keep open and close punctuation on the same line when possible? For example, write this
particular snippet as
B) Should we leave this item to the writer's discretion and make a gentleperson's agreement not to change instances of it that that the other wrote except when there is a true need to otherwise adjust such code? Recall I am (somewhat radically in the current era) of the opinion that a code writer should use the latitude afforded by the language of choice to express the code as clearly as they can, and that best expression may well in particular cases not be whatever some rote set of codebot rules would generate.
So the short answer is yes, I do much prefer
and/or
to either
or
But it is also very important to me that you're comfortable with our coding practices. So let me know your thoughts and when we have settled, I will adjust the instances touched in this PR.
Thanks!
I would certainly prefer it, because it's consistent with the Rust code I'm used to reading.
That seems fine to me.
Since we plan for Husht to have Python-like syntax, I think it would make sense to write it in a more Python-like style. The Python style guide is consistent with the style you're using for long function calls:
@Vectornaut wrote in #128 (comment):
OK, I have gone through and attempted as much as possible to avoid function call open and close parens on different lines by using other linebreaks, especially the "just before dot" ones. Where linebreaks are required within a function call, I've tried to be more liberal with putting the matching close on its own line. But I do still fall back to they "Pythonesque" layout where I just don't see a good alternative.
OK, should be good for next round of review.
This is looking good! Mostly just editing for consistency at this point. For repetitive comments, if you handle as suggested, you can just indicate with a 👍 and I'll resolve the conversation.
For multiline assignments, I prefer to put the
=at the end of the first line, like on line 432 ofengine.rs. In the pull request, you've mostly put the=at the beginning of the second line. It would be nice to pick one convention and stick to it.@ -582,1 +588,3 @@Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);Point::NORM_COMPONENT,col,point(x,y,z)[Point::NORM_COMPONENT]In our current convention, this argument list needs a trailing comma on the last line.
@ -929,1 +939,3 @@console_log!("No velocity to unpack for fresh element \"{}\"", elt.id())console_log!("No velocity to unpack for fresh element \"{}\"",elt.id()In our current convention, this argument list needs a trailing comma on the last line.
@ -965,1 +976,3 @@);let subjects = [0, 1].map(|k| Rc::new(Sphere::default(format!("sphere{k}"), k))as Rc<dyn Element>);I find this quite hard to read. Would you be willing to consider something like this?
Yes this is a tricky one to split. My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case? Anyhow, I will give it another shot.
My preferences are:
., like this:Since (1) conflicts with your preferences, I asked for (2) in cases where that could prevent the violation of (1).
I find (2) above most readable when more than one dot item is chained, so I guess I'd be fine with switching back to Python-style hanging indent for single method calls. I realize that this is what you had in the previous commit, and I apologize for the flip-flop. I think I'm going to have to give up on finding a compromise position on (1) and just deal with formatting I find difficult to read until we use Husht to switch to a non-C-like syntax.
@ -132,0 +135,4 @@};let opacity = if self.ghost.get() {GHOST_OPACITY} else { DEFAULT_OPACITY };I think this is easier to read if either both branches are set off or both branches are inlined:
The latter option is already used on line 146 of
outline.rs.Won't surprise you that I don't see any importance for the two branches of a single if to match in this way, but modulo the location of the = the second is fine so once we figure the = out (always previous line, always next line, or prefer one way but mixed is ok when it helps with layout) i will change this.
@ -189,1 +197,3 @@let color = if selected { self.color.map(|channel| 0.2 + 0.8*channel) } else { self.color };let color = if selected {self.color.map(|channel| 0.2 + 0.8*channel)} else { self.color };I think this is easier to read if either both branches are set off or both branches are inlined, like above on lines 136–138.
@ -608,2 +638,3 @@|| realization_successful && on_last_step;if on_manipulable_step && state.selection.with(|sel| sel.len() == 1) {if on_manipulable_step&& state.selection.with(|sel| sel.len() == 1)I think the line beginning with
&&should be indented four spaces. (Currently, it's only indented three.)Yes typo (I have not yet bothered to install a rust mode for emacs), will fix
@ -653,2 +685,3 @@if frames_since_last_sample >= SAMPLE_PERIOD {mean_frame_interval.set((time - last_sample_time) / (SAMPLE_PERIOD as f64));let SP64 = SAMPLE_PERIOD as f64;mean_frame_interval.set((time - last_sample_time) / SP64);The new variable
SP64is confusing to me for various reasons. Since we're taking an extra line anyway, could we do something like this instead?Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Abbreviation is one nice common way of dealing with line breaks. And Rust's conversion/coercion rules/syntax are just truly annoying, so i find the formula much easier to read without internal coercion. What else might we call the local abbreviation for the coercion of the global constant with the very long name?
Oh, I see. Like I said above, I may have to just accept going back to Python-style hanging indent for function calls and deal with the cognitive dissonance until we get to a non-C-like syntax with Husht.
I'd use
sample_period_f64.Well, that's too long to avoid the line break, so ok, I will revert to the function call being split across lines.
Oh, actually it fits on two lines like this
which I presume you like better. Don't know why I didn't see that before. So will do that in the next revision.
@ -697,0 +728,4 @@let sphere_reps_world: Vec<_> = scene.spheres.representations.into_iter().map(|rep| (&asm_to_world * rep).cast::<f32>()).collect();The revised version is pretty close to the one-per-line convention for chained method calls. I'd recommend going all the way there:
@ -767,0 +805,4 @@);bind_new_buffer_to_attribute(&ctx, point_color_attr,(COLOR_SIZE + 1) as i32,scene.points.colors_with_opacity.concat().as_slice());For consistency, I'd start the arguments on the next line here too.
@ -64,2 +64,4 @@bind:value = value,on:change = move |_| {let specification= SpecifiedValue::try_from(value.get_clone_untracked());A
SpecifiedValueconsists of both a specification (spec) and a value (value), so calling the whole thingspecificationis confusing to me. I might usespecified_valueorspec_val.Since this is a single use temporary introduced just for the sake of an abbreviation, and its single use is on the very next line, I just called it
val.To me, turning a variable called
valueinto a variable calledspec_valorspecified_valueseems more understandable than turning a variable calledvalueinto a variable calledval. In the latter case, the variable names don't make it clear what the point of the processing was. Looking at the line wherevalis used, one might ask: why didn't we just usevaluehere?It's just an abbreviation. It doesn't need much in the way of semantics.
temp?x?s?sv? If it wasn't the case that the variable is introduced solely for the sake of linebreaking and is used only once on the very next line, it might be worth this length of naming conversation. This is a throwaway.Oh, I see: you're now trying to find a name short enough to jam that expression into one line. Personally, I think the extra clarity is worth an extra line. However, among the names you proposed just now,
svseems the least confusing to me.@ -323,1 +334,3 @@engine::sphere_with_offset(frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, 0.0),engine::sphere_with_offset(frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6,-frac_1_sqrt_6, 0.0In our current convention, this argument list needs a trailing comma on the last line.
@ -329,1 +343,3 @@engine::sphere_with_offset(-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, 0.0),engine::sphere_with_offset(-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6,-frac_1_sqrt_6, 0.0In our current convention, this argument list needs a trailing comma on the last line.
@ -335,1 +352,3 @@engine::sphere_with_offset(-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, 0.0),engine::sphere_with_offset(-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6,-frac_1_sqrt_6, 0.0In our current convention, this argument list needs a trailing comma on the last line.
@ -14,0 +12,4 @@pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)-> DVector<f64> {let center_norm_sq= center_x * center_x + center_y * center_y + center_z * center_z;I'd match the convention used for
sphere_with_offset:Here I very much don't see a need for a strict uniform "convention" but rather most readable layout on a case-by-case basis, you said you like function-call open and close on same line when possible, I really universally dislike paren on next line, and of all the places to break
functionName(parameter1: Type1, parameter2: Type2) -> ReturnType {just before the arrow seems by far the cleanest to me: before an operator, clean semantic break between the calling convention on the one hand and the return type on the other, arrow makes it nice and clear this is a function. So I would advocate that breakpoint as the very first to use when needed, and only break the parameter list when it simply won't fit on one line.Thoughts?
The main thing I find confusing about the current formatting is that the
-> DVector<f64>is part of the function declaration, but no indentation is used to show this. If you find bracket on next line more palatable than parenthesis on next line, I'd find something like this more readable than the current formatting:I'll admit I am looking for a two-line solution here. I don't see any reason to indent the
->just as there is no reason toindent the
elseof anif-- to me, the->reads as a companion keyword tofn. So for the upcoming revision, I will try moving just the parenthesis to the next line to see if you like that better. As little as I like a linebreak before a paren, I like wasting lines less ;-)All of the reasonable two-line solutions I can think of seem equally hard to read to me, and equally in violation of the Rust and Python style guides. Here's one last suggestion, which is inspired by (but in violation of) the Python style guide. You're welcome to choose between this formatting and the current one.
Well, from my point of view we discarded double-indent patterns for
ifandforso I am loath to revive them here, so I think we are settling onPlease confirm, thanks.
Confirmed.
@ -202,2 +204,3 @@// projection inner productpub fn proj(&self, v: &DVectorView<f64>, column_index: usize) -> DMatrix<f64> {pub fn proj(&self, v: &DVectorView<f64>, column_index: usize)-> DMatrix<f64> {I'd match the convention used for
sphere_with_offset. It adds one line, but for me the gain in readability and familiarity are worth it.@ -294,1 +297,3 @@fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize) -> DMatrix<f64> {fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize)-> DMatrix<f64>{I'd stick to the convention used for
sphere_with_offset:@ -612,0 +624,4 @@problem.gram.push_sym(block + j,block + k,if j == k { 0.0 } else { -0.5 }In our current convention, this argument list needs a trailing comma on the last line.
@ -705,1 +722,3 @@problem.gram.push_sym(j, k, if (j, k) == (1, 1) { 1.0 } else { 0.0 });problem.gram.push_sym(j, k,if (j, k) == (1, 1) { 1.0 } else { 0.0 }In our current convention, this argument list needs a trailing comma on the last line.
@ -805,2 +831,3 @@fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize) -> Vec<DVector<f64>> {fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize)-> Vec<DVector<f64>> {I'd stick to the convention used for
sphere_with_offset. It adds one line, but for me the gain in readability and familiarity are worth it.@Vectornaut wrote in #128 (comment):
Well, as you know. I am much less concerned with strict uniformity in such things since (as this PR shows) it's a pain to achieve and i just don't see benefits from it that pay for that pain.
I am also a strong advocate for laying out code as closely as one would lay out other forms of text, equations in articles, etc. For example, that's one of the reasons i really dislike parenthesis at the beginning of a line -- there is absolutely no other form of writing in which you would ever do that. If i were setting hard-and-fast layout rules for our code, they would include never doing this. But in the spirit of consensus, i have tried to use them in places that i think you would find them most called for.
So, on this point, if you websearch "equals at end of one line or beginning of next" you will find overwhelming preponderance of recommendations for beginning of next, in line with the general recommendation of splitting just before operators rather than just after.
So if we are going to attempt complete uniformity in this, I would recommend changing any remaining trailing '=' to the beginning of the next line.
Thoughts?
I won't be able to revise the PR again until Monday at soonest, but I took a little tike this morning to reply on points you can consider in the meantime. Thanks!
@Vectornaut wrote in #128 (comment):
Or actually once I do push a commit, you can assume I just took any suggestion i didn't comment on (or comment on a twin of).
This is helpful guideline, and we should keep it in mind when thinking about syntax and style conventions for Husht.
A DuckDuckGo search doesn't return many relevant results for me, but the top result is for mathematical equations, where
=is typically used for equality rather than assignment, and in particular is often chained:When equality is used for assignment, I think that putting the
=at the end of the first line makes it easier to format lists of terms, as I do here:As long as you have a readable way to format the example above, this works for me.
I think the main obstacle here is my preferred way to format nesting in language with C-like syntax, described above as preference (1). Preference (1) is used throughout the codebase, and it's incompatible with your formatting preferences, so this pull request will introduce formatting inconsistencies no matter how much we negotiate.
At this point, I'd be willing to simply accept the indentation conventions of the Python style guide for lines that need to be wrapped, since it seems likely that we'll eventually adopt a similar convention for Husht.
@Vectornaut wrote in #128 (comment):
Well, there are no lines over 80 characters here, so I would be fine simply leaving this code alone for now until there is some specific need to revisit it. If you feel that we need to get all declarations/assigments in one consistent format in this PR, then OK, we can extend this PR from just wrapping long lines, to that and making
=formatting consistent.But personally I would format this as
so there is no issue of where the
=goes. That issue only comes up when the best linebreak within the first 80 characters happens to be at the=sign, which I don't see being the case here.Just to examine the contours of this problem, I am guessing you like the first option below better.
vs
For me, the first form is just wasting a line (well, actually 2, but I wanted to leave the final bracket out of this for now).
So my approach in breaking an assignment is to just start the assignment, and if it runs past 80 characters, go back and see if there's a natural place to break it so that it will fit on just two lines total. That place may be at the =, but that's fairly low on my preference list. And if it is there, then my preference is to break just before the = rather than just after, for the exact same reason we put + or * at the beginning of the next line -- because it's easier to see there when one is just scanning some code. This end-of-previous vs beginning-of-next issue is actually more important in rust, because the thing being assigned to may be an intricate and hence quite long destructuring pattern. So I think it is very helpful to have the
=out in front as a sort of separator between the specifier (of where the assignment will go) and the value (that will go into the location described by the specifier).Not sure whether we are converging here, or how to do so if not.
Looks like there's just two small tweaks left, including the temporary variable naming decision.
@ -582,1 +588,3 @@Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);Point::NORM_COMPONENT,col,point(x,y,z)[Point::NORM_COMPONENT],Our current convention is to put spaces after the commas between arguments.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.