Thank you for the _excellent_ review. This is _exactly_ the type of stuff I need.
> https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly > File input/regression/tuplet-number-outside-staff-positioning.ly > (right): > > https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly#newcode15 > input/regression/tuplet-number-outside-staff-positioning.ly:15: > \override TupletBracket.Y-offset = > #ly:side-position-interface::calc-outside-staff-y-offset > This is indicating a fundamental design problem: this override is not > related to the topic of the regtest. If it is necessary for getting the > desired result, it will be necessary in a whole _lot_ of user-created > situations. But it is not an easily user-accessibly override, and it is > not documented in normal user documentation. > > This suspiciously looks like tampering with unrelated regtests in order > to mask fundamental problems from review. Can you explain why this is > not the case? Your question gets to the core of the logic of the patch, so I'll explain it and then people can comment upon how that links up with this regtest. In LilyPond, about 85% of grob properties are set as the result of the evaluation of a callback or the use of a rote value. About 15% are set via calls to set_property and translate_axis (the latter of which only controls Y and X offset). The top-down approach to property setting (i.e. a VerticalAxisGroup controlling the Y-offset of its elements, which is currently the case for outside staff positioning) poses the problem that once a value is cached, it is very difficult to run calculations again for that value once more precise information is available. With the bottom-up approach (grobs reporting their properties to their collecting grobs), however, this is easy and is done all over the code base. For example, in separation-item.cc, approximations of heights are used in Separation_item::boxes to do horizontal spacing. I am trying to get LilyPond to a point where it does vertical spacing the same way. This will allow for much better approximation of distances between staves containing cross-staff grobs. If this is the case that grobs' offsets will be controlled by callbacks and not by uber-grobs like VerticalAxisGroup calling translate_axis a bunch of times, then grobs that are going to be placed outside the staff need to have their Y-offset function reflect that. Now, LilyPond, for various callbacks, other properties must be set for those callbacks to make sense. For example, if I do: \override NoteHead #'stencil = #ly:text-interface::print Nothing will happen. But if I do: \override NoteHead #'text = #"foo" \override NoteHead #'stencil = #ly:text-interface::print The NoteHead will be printed as foo. This is exactly what we're seeing in the regtest tuplet-number-outside-staff-positioning.ly. A callback is set for Y-offset that allows the grob to be positioned outside the staff. But, just as the text interface needs to know what text to print, the side-position-interface needs to know what outside-staff-priority to use to place the grob. Thus the use of two overrides instead of one. A music function could be done in the form of: \addOutsideStaffPriorityToGrob #'TupletBracket #100 that rolls the two overrides into one. This is a UI issue about which I have not thought yet but that absolutely deserves attention. For those who are more adept at UI than I, I'm very interested in your ideas. Furthermore, how could this be documented to make sense to the other user. For me, the sentence "some overrides require supplemental overrides to kick in" seems kind of abstract and geeky. How can this be communicated? > > https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly > File input/regression/tuplet-number-outside-staff-priority.ly (right): > > https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly#newcode13 > input/regression/tuplet-number-outside-staff-priority.ly:13: \override > TupletNumber.Y-offset = > #ly:side-position-interface::calc-outside-staff-y-offset > See above comment. If outside-staff-priority ceases to work, a lot of > user-level documentation would warrant adaption. I agree, but this would only be the case for grobs not already using ly:side-position-interface::y-aligned-side. Currently, every grob using outside-staff-priority is using this function as a callback. So the only thing that users will need to be made aware of is the effect of this change on overrides. What would be a good way to communicate that? > > https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc > File lily/axis-group-interface.cc (right): > > https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode595 > lily/axis-group-interface.cc:595: > Axis_group_interface::staff_priority_less (Grob *const &g1, Grob *const > &g2) > Is there a reason you turn this into member functions? > There was, but that is no longer relevant. I will change the code to reflect that. > https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode713 > lily/axis-group-interface.cc:713: > Axis_group_interface::prepare_for_outside_staff_calculations (Grob *me) > There is no description anywhere what this preparation will entail, what > it looks at, and what it does. Is this used even more than once? It is used twice - once in axis-group-interface.cc and once in side-position-interface.cc. I have added a comment in the code base that I've copied and pasted below: /* Before doing calculations involving outside staff priority are done, we suicide any grobs that need to be suicided and make sure that grobs that depend on the placement of a Y parent do not have a lower outside-staff-priority than the parent, which would cause their being placed first. */ > > https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode755 > lily/axis-group-interface.cc:755: > Axis_group_interface::sort_outside_staff_elements (Grob *me, vector<Grob > *> &elements) > This looks like it does a whole lot more than just sorting some > elements. What does it do? Why does it do that? I have added a comment in the code base that I've copied and pasted below: /* When placing a group of outside-staff-elements outside the staff, we want to sort their placement order based on three criteria. 1) Grobs with a lower outside-staff-priority will always be placed before grobs with a higher outside-staff-priority. 2) Grobs with outside-staff-priority set that are the Y-parents of other grobs will be placed before the children grobs are placed. 3) For grobs with equal outside-staff-priority, we use the outside-staff-placement-directive property to break ties. The docstring for outside-staff-placement-directive describes how these ties are broken. After the grobs are sorted using these three criteria, their Y-offset can be calculated. */ There is also a long-ish comment in the function with more information. > > https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh > File lily/include/axis-group-interface.hh (right): > > https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh#newcode69 > lily/include/axis-group-interface.hh:69: static bool staff_priority_less > (Grob *const &g1, Grob *const &g2); > All those were previously staic functions inside of > axis-group-interface.cc, and thus limited to that file. Now you have > made them part of the class definition, and in addition you have turned > then into a _public_ part of the class definition, suggesting that they > are part of the external interface of the class. > > Why? It is based on a previous idea that I abandoned, so it's set back to the way it was. > > https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh > File lily/include/directional-element-interface.hh (right): > > https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh#newcode26 > lily/include/directional-element-interface.hh:26: // what is the > advantage not having these two as STATICs of GROB -- jcn > "these three" it would appear now. And the question seems valid. This is a good question. It is a trivial change but the reasons justifying making these static member functions of Grob are over my head. It seems like a separate patch, pushed before or after this one, can deal with that. > > https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh > File lily/include/side-position-interface.hh (right): > > https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh#newcode45 > lily/include/side-position-interface.hh:45: static Real > calc_outside_staff_offset (Grob *me, bool pure, int start, int end, Real > current_offset); > Isn't that just something used internally by > calc_outside_staff_y_offset? Is there a reason this is part of a > _public_ interface? > No. There are a lot of internal placement functions that are part of a public interface, so I just followed that convention. For example, Self_alignment_interface::aligned_on_self is only ever used internally in self-alignment-interface.cc. So why is it a public function? I don't know the answer because I don't know enough about C++ coding to determine what should be public or private. However, this sort of thing is all over the code base. It may be worth asking Han Wen and Jan why they did it this way, as I wouldn't want to change the convention unless there was a solid reason. > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc > File lily/side-position-interface.cc (right): > > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode184 > lily/side-position-interface.cc:184: // v_skyline) needs to move UP or > DOWN so that it doesn't intersect with > Presumably the function does not pick "UP or DOWN" on a whim, but it is > rather "in direction of dir". However, if it can be _added_ to the > grob's Y-offset, it is _not_ the value to move UP or DOWN, but rather > the offset to add to Y-offset in order to position it beyond all > other_v_skylines with regard to direction dir (UP or DOWN). > > Wait: you say "dir is the placement above or below the staff". Does > that mean that dir can be UP, but the placement of the offset can be > _below_ the other_v_skylines but above the staff? If so, will it first > attempt to fit elt _between_ staff and other_v_skylines, or will it go > outside in any case? I've added the following comment: // Note that this function will try to keep elements as close to the staff // as possible. So, if the element fits under other elements above the staff, // it will be shifted under instead of over these elements. So, the dir // property does not refer to the placement with respect to other vertical // skylines, but rather with respect to the staff. Let me know if that's clear. > > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode464 > lily/side-position-interface.cc:464: Real outside_staff_offset = a == > Y_AXIS > This is quite contorted and hard to read, what with = == ?:... > > What you mean is > if (a == Y_AXIS) > return scm_from_double (total_off > + calc_outside ...); > return scm_from_double (total_off); > > If you want to, with an else. Depends on your style. Or the other way > round, starting with if (a == X_AXIS)... > > But the point is that it is pointless to juggle with ?: to avoid an "if" > if one branch of ?: is going to "add" 0, namely do nothing anyway. This > means not just different offsets, but different _actions_. ?: can > always be replaced, but it has a few convenient use cases. This isn't > one. Code should not be an intelligence test, but boring. Fixed. > > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode497 > lily/side-position-interface.cc:497: // start: start of pure calculation > for spanners > Well, you are right that this place is strange to do a full > documentation of pure function arguments. Removed. > But we need to get one > eventually somewhere (not necessarily as part of this issue), and we are > talking about how to do proper commenting, so let's focus on what's > wrong about these comments anyway, even though they don't necessarily > belong here exactly. Ok. > > Let's take apart what is wrong in detail: > > They don't add useful information. > > Well, apart from "for now facultative", but that's information about the > implementation rather than the use. > > eventually "is this pure": it does not specify the target for > "eventually", and it does not specify "this". > > What presumably is meant is "is the returned offset to be derived via > pure calculation?". > > Then start and end are presumably _only_ relevant when it _is_ a pure > calculation. Not mentioned. Then the comment "start/end of pure > calculation for spanners" is totally awful. The parameters are already > _called_ start/end, only repeating that is not helpful. It is not at > all apparent what the connection with _spanners_ would be. And "start > of pure calculation" is nondescriptive. What is the unit that the > "start" is specified in? Is it seconds since Jan 1 1970 UTC? Or is it > rather some index into some data structure? Which one? Give a term > that can be reasonably found using "git grep", and "start" and "end" > alone are not really helpful for that. > The reason this comment was so brief is because I didn't understand why you wanted to see documentation of pure functions in this place and not in the 20 or so other places where this convention is used. I get what you're saying now and I agree that it needs to be documented thoroughly. An issue should be opened up for this. > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode538 > lily/side-position-interface.cc:538: if (Skyline_pair > *inside_staff_skylines = Skyline_pair::unsmob (iss)) > After this line, GUILE no longer sees iss as something it needs to > protect from garbage collection. > I've just recently learned from your reviews that this is a bad idea. It is currently present in 20-ish places in the code base, so it'd be worth it in a separate patch to eliminate all of them. For the purpose of this patch, I have eliminated only this one plus others I have run across while reading your review. An issue can be opened for the rest. > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode539 > lily/side-position-interface.cc:539: skylines = Skyline_pair > (*inside_staff_skylines); > Now if Skyline_pair contains any elements allocated by GUILE > (non-immediate Scheme values), it will ask GUILE for new elements, and > GUILE might allocate them from *inside_staff_skylines data. It turns > out from looking through the headers that Skyline_pair is free from SCM > data. But doing that kind of research every time one reads the code is > costly, so it is easier if you just stick with the SCM value until you > have created your copy or finished with the whole thing. > > unsmob it for every use until then, and GUILE will let it stick around > until the last unsmob. Or put an scm_remember_upto_here_1 on the SCM > value after the last use of an unsmobbed pointer from it. I'm not sure what you mean here. Could you present a C++ code example of exactly what would be need to added here in order for it to reflect what you're saying? > > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode565 > lily/side-position-interface.cc:565: Skyline_pair *v_orig = > Skyline_pair::unsmob (elt->get_property ("vertical-skylines")); > Same here. > Here I understand exactly what to do - I'm getting the SCM object first and then unsmobbing. But in the case above, that's already what's happening, so that's why I need a C++ example of exactly what to do. > https://codereview.appspot.com/7185044/diff/32003/lily/side-position-interface.cc#newcode593 > lily/side-position-interface.cc:593: Skyline_pair v_skylines (*v_orig); > Too much distance to above, and several calls of get_property, > potentially allocating memory, in between. I explained all this in the > previous review. Please generalize one explanation to all occurences of > the same problem. If the problem is not clear to you, ask questions > until it is. Reviews (and things like valgrind) are a safety net, not a > trampoline. > There was a mistake here - I accidentally named two different variables the same thing. Fixed, so this is no longer v_orig. The general problem is that I want to make a C++ copy of unsmobbed objects so that I can manipulate the copy without the original changing. So, SCM foo_scm = me->get_property ("foo"); // gets the property Skyline_pair *foo = Skyline_pair::unsmob (foo); // unsmobs it Skyline_pair foo_copy (*foo); // makes a copy of foo that will be changed so that the original skyline is not changed. Is the above the best way to do this? If not, what is? Many thanks for the review - it was very helpful! Cheers, MS _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
