note-spacing: stretch somewhat uniformly; issue 3304 (issue 36830045)
https://codereview.appspot.com/36830045/diff/20001/lily/note-spacing.cc File lily/note-spacing.cc (right): https://codereview.appspot.com/36830045/diff/20001/lily/note-spacing.cc#newcode117 lily/note-spacing.cc:117: ret.set_inverse_stretch_strength (base_space); Looked at it, but I have no idea what any of this stuff means. If you understand this stuff, could you put a comment in lily/include/spring.hh as to what inverse_compress_strength and inverse_stretch_strength are? https://codereview.appspot.com/36830045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
grob-property: if callback is independent of layout, call just once (issue 42190043)
https://codereview.appspot.com/42190043/diff/1/lily/unpure-pure-container.cc File lily/unpure-pure-container.cc (right): https://codereview.appspot.com/42190043/diff/1/lily/unpure-pure-container.cc#newcode45 lily/unpure-pure-container.cc:45: Looking good. The only thing is - how do we verify that this actually ignores start and end? For example, let's say that there is a scheme function with optional arguments for start and end that is used as the pure and unpure function. It'll evaluate just fine for unpure cuz begin and end are optional. But begin and end may be useful when it is pure. If this is the only function passed to the constructor, it will return true for the function above and yet perhaps be an operation that we don't want to cache. Conversely, it is possible to do: (ly:make-unpure-pure-container foo foo) that should be cached but it would return false for this function. So, my proposal would be: 1) if there is a single function, verify that it takes only one argument 2) if there are two functions, verify that the second takes only one argument All of this is of course predicated, like I said above, on being able to check the number of arguments a scheme function takes, which may or may not be possible... Or create a new type predicate for a new class of functions instead of doing a boolean test (which seems like a pain...so I like the boolean approach above). https://codereview.appspot.com/42190043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows inner-markup spacing using skylines. (issue 13341044)
The point is to allow users to achieve the same snug skyline spacing between stencils that is achieved between grobs. I agree that it is inefficient as skylines are never cached, but as it is not default beahvior anywhere in the code base, it allows people to decide on the efficiency versus snugness-of-placement trade-off. For people creating complex markups, this is useful. https://codereview.appspot.com/13341044/diff/1/lily/stencil.cc File lily/stencil.cc (right): https://codereview.appspot.com/13341044/diff/1/lily/stencil.cc#newcode271 lily/stencil.cc:271: add_at_edge (a, d, s, padding, false); On 2013/08/28 21:17:20, dak wrote: That's nonsensical. C++ knows defaults arguments for functions. Done. https://codereview.appspot.com/13341044/diff/1/lily/stencil.cc#newcode313 lily/stencil.cc:313: offset = (*first_skyp)[d].distance ((*second_skyp)[-d]); On 2013/08/28 21:17:20, dak wrote: This is an expensive non-cached operation, and yet this calculates skyline _pairs_ and ignores half of them. Made a Skyline-only function. https://codereview.appspot.com/13341044/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/13341044/diff/1/scm/define-markup-commands.scm#newcode2038 scm/define-markup-commands.scm:2038: (ly:stencil-combine-at-edge m1 axis dir m2 0.0 #t))) On 2013/08/28 21:17:20, dak wrote: Why use the same function in the first place when the last argument is never used as a variable? What would be a better way to achieve this? https://codereview.appspot.com/13341044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds dimension stencil command to correct with-dimension (issue 12957047)
https://codereview.appspot.com/12957047/diff/46001/lily/paper-system.cc File lily/paper-system.cc (right): https://codereview.appspot.com/12957047/diff/46001/lily/paper-system.cc#newcode55 lily/paper-system.cc:55: if (head == ly_symbol2scm (skyline-stencil)) On 2013/08/24 16:19:27, dak wrote: It seems awkward and error-prone to go through a number of stencil types here just for fishing out footnotes. Of course, this is just another layer of ugliness on top of existing ugliness, but it would seem that we actually want a separate _backend_ for fishing out footnotes, so that all the default interpretations of stencils are done correctly. Agreed - it's a problem in general of markups being implemented differently than grobs, which has plagued footnotes from the beginning. https://codereview.appspot.com/12957047/diff/46001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/12957047/diff/46001/lily/stencil-integral.cc#newcode915 lily/stencil-integral.cc:915: SCM skyline_stencil = scm_cadr (expr); On 2013/08/24 16:19:27, dak wrote: Why would this traverse the skyline_stencil rather than the real one? Because we are using the skyline stencil as a substitute for the dimensions of the real one. https://codereview.appspot.com/12957047/diff/46001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/12957047/diff/46001/scm/define-markup-commands.scm#newcode730 scm/define-markup-commands.scm:730: (make-box-skyline-stencil On 2013/08/24 16:19:27, dak wrote: Stupid question: _iff_ you store a _whole_ replacement skyline anyway, shouldn't pad-around just shift the original left/right/up/down skylines left/right/up/down by the given amount (don't ask me what to do about the corners, though)? Of course this would be incompatible with previous behavior, but more likely matching the expectations? I am not sure that replacement skylines are the right thing anyway, but it seems sort of pointless _if_ you propose they are to not use them here. It's a good idea, but stencils don't have left/right/up/down, so it'd be hard to figure out how to add this padding to the stencils without additional plane-geometry functions. https://codereview.appspot.com/12957047/diff/46001/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/12957047/diff/46001/scm/stencil.scm#newcode689 scm/stencil.scm:689: (let ((sur (make-filled-box-stencil x y))) On 2013/08/24 16:19:27, dak wrote: A filled-box stencil seems like serious overkill here when all you want is setting the dimensions. A stencil operator for just setting dimensions seems less awkward. There are two possibilities: have it produce an empty stencil expression but with dimensions, and then use make-skyline-stencil on it. Or have it contain an inner expression anyway. In that case, make-box-skyline-stencil (what an awkward name) does not need to call either make-filled-box-stencil nor contain skyline-stencil anyway. It means that one needs one more stencil primitive _if_ one wants to support make-skyline-stencil. But it avoids juggling with an obscure combination of stencil operations if one doesn't. The empty stencil wouldn't work, as stencil-integral.cc operates on the contents of the stencil (lines, curves, etc.), not on the dimension. So, it would read an empty stencil as empty and not take it into account in the skyline, irrespective of the dimension. I'm not exactly sure what you mean in the rest of the comment, nor do I see why the filled-box-stencil is overkill. We need to draw a box around the stencil at some time, and this gets the job done pretty efficiently (there's not much overhead in stencil creation). I'm open to other implementation suggestions, though. https://codereview.appspot.com/12957047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds dimension stencil command to correct with-dimension (issue 12957047)
I had time to implement everything but the delayed markup stuff. I'll try to get to that soon... https://codereview.appspot.com/12957047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds dimension stencil command to correct with-dimension (issue 12957047)
It was easier to implement the delayed stuff than I expected, so I got it done. https://codereview.appspot.com/12957047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
side-position-interface: use real positions of cross-staff grobs; issue 3363 (issue 9426044)
LGTM - just make sure to check this against the dynamics-avoid-cross-staff-stem regtests. https://codereview.appspot.com/9426044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Flags cross-staff semi ties. (issue 8857043)
On 2013/04/19 12:40:43, Neil Puttock wrote:
Hi Mike,
Generally LGTM, but since there's nothing in the code which requires
C++ code I
favour adding a callback to output-lib.scm:
(define-public (semi-tie::calc-cross-staff grob)
(let* ((note-head (ly:grob-object grob 'note-head))
(stem (ly:grob-object note-head 'stem)))
(and (ly:grob? stem)
(ly:grob-property stem 'cross-staff #f
BTW, this surprised me a bit while fiddling with the regression test:
\new Staff = up \relative c' {
f8
\change Staff = down
c e\f %oops
\change Staff = up
f
}
\new Staff = down { \clef bass s2 }
Cheers,
Neil
Thanks Neil!
I wonder why this is a problem in this test you're doing and not the
dynamics-avoid-cross-staff regtests...will investigate...
Cheers,
MS
https://codereview.appspot.com/8857043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add callback factory grob::inherit-parent-property (issue 8726045)
Hey, I hadn't seen this. I just finished writing an equivalent patch. Yours is better, so keep it. You can use this for the inherit-X-parent-visibility and eliminate the inherit-Y-parent-visibility callback, which is cruft and can be replaced with the X one. https://codereview.appspot.com/8726045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add changes entry for Mike's work on skylines. (issue 8613043)
https://codereview.appspot.com/8613043/diff/19001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/8613043/diff/19001/Documentation/changes.tely#newcode153 Documentation/changes.tely:153: @item I would add a note that, if there is too-snug spacing for an object, setting its vertical skylines to #'() will generally restore old behavior. https://codereview.appspot.com/8613043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add changes entry for Mike's work on skylines. (issue 8613043)
On 2013/04/16 07:51:46, dak wrote: On 2013/04/16 07:40:19, Trevor Daniels wrote: On 2013/04/16 07:20:59, dak wrote: On 2013/04/16 06:05:42, MikeSol wrote: I would add a note that, if there is too-snug spacing for an object, setting its vertical skylines to #'() will generally restore old behavior. I am not really enthused about telling people that, since too snug is a bug and needs to get fixed rather than worked around. We'll get a proliferation of LilyPond sources irretrievably tied into the bugs of particular versions if we tell people to work around bugs rather than report them. Indeed. That is why we do not document bugs or add workarounds to the documentation (with a few exceptions, usually when it is agreed a fix is most unlikely to materialise, e.g. the grace timing problem.) Bugs (and their workarounds) should be recorded in the Bug DB. I hate it when I get last-minute realizations. Here is another thing we need to do for the stable release: go through all problems of the too snug kind and work out defaults that avoid them. It's ok to tell people in our documentation how to get stuff move closer in case of necessity, but if we aren't talking about running text and similar things, looser spacing than necessary is _not_ a bug. Closer spacing than appropriate _is_ a bug. One can publish a score with loose spacing, but one can't publish a score where things are sitting on each other. So for a stable release, we need conservative settings. Settings that won't _force_ people to pepper their sources with overrides and tweaks just to throw them all out again when the next version arrives. This is a great time to ping the user list. People who have been using the development version for a while now have had a chance to get used to scores with this spacing, and if they have anything that they consider too tight, then we can use simple skylines for those things. We've already had some back-and-forth about text grobs but not much else. I think it is a percentage game, more than anything else. Meaning what percent of users need to consider spacing too close before it is inappropriate as a default. If 90% like the new spacing and 10% think it is too close, then it is important to do something like Trevor is talking about that gives them a way out. However, if it is 50/50, then it's better to err on the side of conservative. Anyway, we'll know none of these things without consulting people making real scores. I prefer all the spacing improvements in my scores so no complaints here. But I'm willing to admit of course that I wouldn't have done any of this work unless there were some internal motivation stemming from my own aesthetic preferences. https://codereview.appspot.com/8613043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: rewrite Self_alignment_interface (issue 7768043)
I'd recommend adding a sort of padding property to the self-alignment-interface to get it completely there. That is, imagine that we right align to a grob and we want to be padded by 0.1 We should be able to do that. That'd allow self-alignment-interface to be used for grobs like InstrumentName. https://codereview.appspot.com/7768043/diff/46001/lily/new-fingering-engraver.cc File lily/new-fingering-engraver.cc (right): https://codereview.appspot.com/7768043/diff/46001/lily/new-fingering-engraver.cc#newcode291 lily/new-fingering-engraver.cc:291: // don't overwrite offset property if it was overridden by the user Consider chaining an offset callback here. See chain_callback and chain_offset_callback. https://codereview.appspot.com/7768043/diff/46001/lily/self-alignment-interface.cc File lily/self-alignment-interface.cc (right): https://codereview.appspot.com/7768043/diff/46001/lily/self-alignment-interface.cc#newcode152 lily/self-alignment-interface.cc:152: This would be the place to tack on padding. https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm#newcode143 scm/define-grobs.scm:143: (X-extent . (-0.1 . 0.1)) ;UGH! how to get ambitusline extent from its stencil? Why can't you use the standard X extent function given in grob.cc? https://codereview.appspot.com/7768043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: reorganize self_alignment_interface (issue 7768043)
In general, I see a lot of reassigning of parents. What is the goal with this (sorry if you've explained this already)? https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc File lily/fingering-engraver.cc (right): https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc#newcode142 lily/fingering-engraver.cc:142: Self_alignment_interface::aligned_on_x_parent (fingerings_[i]-self_scm ())); This needs to be a chained offset procedure. See chain_offset_callback elsewhere in the code. https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc File lily/paper-column.cc (right): https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc#newcode222 lily/paper-column.cc:222: /* Good work! https://codereview.appspot.com/7768043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Eliminates side poisition calculations for system-start-text grobs. (issue 7574048)
On 2013/03/24 19:59:38, dak wrote:
On 2013/03/24 19:06:49, mike7 wrote:
On 24 mars 2013, at 14:59, mailto:d...@gnu.org wrote:
https://codereview.appspot.com/7574048/diff/5001/input/regression/instrument-name-x-align.ly
File input/regression/instrument-name-x-align.ly (right):
https://codereview.appspot.com/7574048/diff/5001/input/regression/instrument-name-x-align.ly#newcode33
input/regression/instrument-name-x-align.ly:33: \set Staff .
instrumentName = \markup \right-column {
Make no mistake: the previous code looked suspiciously asymmetric.
Is
your change a cosmetic one, or is it related to other strange
behaviors
like the \left-column/\column mismatch we had noticed elsewhere?
https://codereview.appspot.com/7574048/
The latter - I noticed it clashed and have run into problems with
right column
in the past and decided to get rid of it.
I can put it in a different commit before pushing this one.
In general, separate issues should be separate commits even if you
don't want to
start a tracker issue of its own for it.
Since one wants to be able to pinpoint stuff by bisection, the regtest
change
should be in a commit before the rest.
Will do - I'll push this as two separate commits.
Cheers,
MS
https://codereview.appspot.com/7574048/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Removes outside-staff-priority from dynamic-related objects in Dynamic context (issue 7655045)
On 2013/03/27 21:50:02, janek wrote:
I see that these overrides solve the problem, but it would be nice to
know why
this works. Can you write an explanation? What was wrong with the
skylines
that caused this bug? Maybe this is just one instance of a more
general
problem.
Objects that are placed inside the staff were being placed outside the
staff. Conceptually, dynamics are inside a staff in Dynamics
contexts, even if there is no staff.
https://codereview.appspot.com/7655045/diff/2001/input/regression/dynamics-outside-staff-priority.ly
File input/regression/dynamics-outside-staff-priority.ly (right):
https://codereview.appspot.com/7655045/diff/2001/input/regression/dynamics-outside-staff-priority.ly#newcode18
input/regression/dynamics-outside-staff-priority.ly:18:
I suggest to minimize this example to
\new Staff = Test {
\tempo Andante c'1
}
\new Dynamics \with { alignAboveContext = Test } {
s1\f
}
Done
https://codereview.appspot.com/7655045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Page numbering: first page format only on first page of a book (issue 7974046)
LGTM https://codereview.appspot.com/7974046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)
Hey all, Following up to my comment on the tracker, I'd like to not push this until everyone has had a chance to think through David's e-mails about 2.18. I anticipate this needing 2-3 months of user testing to catch and fix any bugs. Given that this is my first big patch since August, it'll likely take me another 6-7 months to do the next phase on top of this patch. Comfortably after 2.18 (hopefully). I am confident in the patch and think it is a step in the right direction for numerous reasons, but it's obviously not worth pushing it if people want to get 2.18 out in the next couple months. If I did push it, I'd send an e-mail to lilypond-user once the next development version came out encouraging people to compile big scores and report back any anomalies. I haven't seen any in my scores. Cheers, MS https://codereview.appspot.com/7185044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows slurs to break at barlines. (issue 7424049)
The most recent patch set copies direction from SlurEvents and PhrasingSlurEvents, but this doesn't seem to work as intended (it fails silently). Everything else is operational. https://codereview.appspot.com/7424049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: report a programming error when trying to align on empty parent (issue 7533046)
LGTM https://codereview.appspot.com/7533046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: don't abort aligning when grob's parent is a PaperColumn (issue 7564044)
https://codereview.appspot.com/7564044/diff/9001/lily/self-alignment-interface.cc File lily/self-alignment-interface.cc (right): https://codereview.appspot.com/7564044/diff/9001/lily/self-alignment-interface.cc#newcode130 lily/self-alignment-interface.cc:130: // TODO: should this function be defined someplace else, e.g. in note-column.cc? I like where you're going with this. I would recommend creating a function algined_on_grobs that accepts a grob, an vector of grobs to align to, and an axis. Then algined_on_parent becomes a subset of this where the vector is of length one and is the parent. Then, you can create a function Self_alignment_interface::lyrics_algined_on_note_columns that invokes aligned_on_grobs with a vector of note columns returned using the search method in this function. This function could be implemented in paper-column.cc as get_generic_interface_extent such that it recurses through a paper columns' element list, finds all grobs implementing interface X and returns them. https://codereview.appspot.com/7564044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: reorganize self_alignment_interface (issue 7768043)
https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc File lily/self-alignment-interface.cc (right): https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode128 lily/self-alignment-interface.cc:128: /* LilyPond positions every grob relative to its parent in respective axis. In general, try to avoid using the word LilyPond in comments, as it is in the LilyPond code base. https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode133 lily/self-alignment-interface.cc:133: I don't see staff space being looked up anywhere in the function. https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode144 lily/self-alignment-interface.cc:144: With respect to my last review, this function should accept a pointer to a vector of grob pointers: vectorGrob * hims so that you can do the note column stuff for lyrics. https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode144 lily/self-alignment-interface.cc:144: I don't have the energy to trigger another comment flame war, but a comment like this may wind up diverging from what the code is doing (it already does with the staff space thing). Can you write the bare minimum and comment the code below in places? The code is already clear: I can tell what you're doing and why you're doing it, so it speaks for itself. https://codereview.appspot.com/7768043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows minimum-length to work for end-of-line spanners. (issue 7453046)
Reviewers: lemzwerg, dak, mike7, Message: On 2013/03/11 10:18:59, dak wrote: On 2013/03/10 00:32:43, mike7 wrote: Why is this override needed for the regtest? The other overrides are obvious user-accessible overrides for triggering the tested functionality. But should _this_ override not be the default? https://codereview.appspot.com/7453046/ Perhaps open a tracker issue for this? The question is not only valid for text spanners but also hairpins, glissandos, etc. Last time I looked, this issue purportedly Allows minimum-length to work for end-of-line spanners. And according to the regtest, it does not do the job. Without additional messing with springs-and-rods it does not allow minimum-length to work for end-of-line-spanners. [...] Additional messing with springs and rods is because minimum-length is currently implemented by four different interfaces (lyric-hyphen, multi-measure-rest, ottava-bracket and spanner) and is also looked up in lyric-extender in a way that does not correspond to its docstring. So, certain uses of minimum length require the additional override whereas others don't. I do not think this is ideal, which is why I proposed a few weeks ago standardizing property names across interfaces. It seems like the issues you are raising above and below have less to do with this patch and more to do with the multiple implementation of minimum-length and the use of the springs-and-rods property. This is a typical case of Somebody Else's Problem. If there is a party and you place a chair in the worst possible place, like somewhere in a doorway, people will walk around it, squeeze themselves through, get annoyed again and again. That's our current state. Now someone wants to do a polonaise and since the chair is in the way, he proposes putting a plank across it. Now we are moving from ridiculous to absurd, and it becomes harder to do the right thing. Yes, I am fully aware that you are not responsible for the chair in the way of your polonaise. Bit it is time to move it aside rather than taking it into account in our planning and make it even harder to get into a proper state. In particular since your coding style requires a lot of reverse engineering in order to figure out the hidden assumptions and dependencies. So that makes it doubly desirable to not add further dependencies on broken behavior but first clean up the foundations. Yes, that's not a problem caused by your patch, but the consequences are acerbated by it. The only thing more frustrating than missing functionality is purportedly available functionality that needs non-user-comprehensible trickery to actually work. I do not have a problem with the current need to set the springs-and-rods separately. Of course you don't, and nobody keeps you from applying this sort of patch to your own private code base without doing the necessary cleanup before. But you are not the metric for what goes into LilyPond's own repository and what not. The functionality that goes into LilyPond has to work for the average user encountering that problem space. The solutions have to have a meaningful correspondence in complexity to the problem and not require don't think about it steps. I'm positive it would because of the way that minimum-length is multiply defined. That is why this patch is intended for the some layout objects discussed above like the TextSpanner. I agree that the multiple use of the minimum-length property should be changed, but this seems like the business of another patch. Sure. But that patch will be harder to do once this patch _relies_ on the broken behavior, and people write code _relying_ on this patch. If the regtest is bothering you that much, I can just eliminate it from this patch. There is no point in hiding the symptoms of a problem away. That only makes things even harder in future. I don't think this is a problem blocking the current patch. You are right that the current multiple functions of minimum-length are problematic. I'm not arguing that this is someone else's problem - I am arguing that this (like all bugs) is the community's problem. Yes, the regtest that I've proposed uses an override that corresponds to that used in the Documentation which is, as you have pointed out, a suboptimal way of doing things. It may take months or years to sort out the multiple naming of properties. This shouldn't block this patch from being pushed. So when you say But this patch will be harder to do once this _relies_ on the broken behavior., all this patch will rely on is an override that you are arguing should be the default. This patch fixes behavior that would exist even if the springs-and-rods override became the default. Imagine it this way. Patients are taking a suboptimal drug for a disease. With RD, a company could invent a better one in a few years. Mike INC. proposes a
Re: Adds Ferneyhough hairpins to LilyPond. (issue 7615043)
On 2013/03/13 21:38:59, thomasmorley65 wrote:
Hi Mike,
sorry to be such an inch pincher.
https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm#newcode1061
scm/output-lib.scm:1061: (thick (* thick (layout-line-thickness
grob)))
At first sight I was surprised about `layout-line-thickness´. Than I
remembered,
it is defined in bar-line.scm
How about making it public, moving it to lily-library.scm?
It could be used then in local custom-definitions.
Same with `layout-blot-diameter´.
I know, it's another topic, though, what do you think about it?
Great idea!
https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm#newcode1078
scm/output-lib.scm:1078:
What do you think about predefining sth like
ferneyhoughHairpinOn/Off
constanteHairpinOn/Off
in /lyproperty-init.ly?
i.e.
ferneyhoughHairpinOn =
\override Hairpin #'stencil = #ferneyhough-hairpin
I'm not against this idea at all - I'd like to see it proposed in a
different patch set, if possible. I am teerible at judging UI
issues because I hack so much. It is true that shortcuts help, but
there are many, many overrides that are shortcut-worthy. I'm not sure
what standard determines which overrides should get shortcuts and which
shouldn't, so I leave that to a future (and important!) conversation.
https://codereview.appspot.com/7615043/diff/23001/scm/output-lib.scm#newcode1082
scm/output-lib.scm:1082: (define-public constante-hairpin
I'm not happy with constante-hairpin.
Look at the output from:
dimMolto =
#(make-dynamic-script (markup #:normal-text #:italic dim. molto))
\relative c'' {
\dynamicUp
\override Hairpin #'stencil = #constante-hairpin
a1 a4\dimMolto\ a a a\!
}
The hook is placed at the left pointing up.
Wouldn't it be better to have the hook _always_ at the right?
Perhaps by adding a boolean variable to `my-c-p-s´. Doing scaling only
while
(and scalable? decresc?).
Is it possible to make the hook-direction depending on the position of
the
Hairpin i.e below or above the staff?
The constante indication means that the dynamic should not change at
all, so the decision to use a hairpin that grows in a given direction is
admittedly a hack. The idea of dimMolto (or crescMolto) with constante
is a contradiction (get very quiet while not changing dynamic). Insofar
as a hairpin represents a change in dynamics, constante should, in
theory, be a different grob. My solution is not ideal, but I think it
is an OK middle-ground short of the creation of a separate grob. If
people think it is too hackish, I will propose a new patch with a
Constante grob (or make the hairpin grow-direction = #CENTER default to
constante, which'd be difficult but doable).
https://codereview.appspot.com/7615043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds Ferneyhough hairpins to LilyPond. (issue 7615043)
https://codereview.appspot.com/7615043/diff/1/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/7615043/diff/1/scm/output-lib.scm#newcode929
scm/output-lib.scm:929: form. @code{x} is the portion of the width
consumed for a given line
On 2013/03/08 06:26:29, lemzwerg wrote:
Please use two spaces after a full stop.
Done.
https://codereview.appspot.com/7615043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
Thanks for the review!
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):
https://codereview.appspot.com/7310075/diff/53001/input/regression/skyline-point-extent.ly#newcode5
input/regression/skyline-point-extent.ly:5: even though the
@code{NoteHead} stencils are empty. The @code{Stem_engraver}
On 2013/03/08 14:24:11, dak wrote:
This talks about the NoteHead stencil being empty, but the actual code
uses a
point-stencil (which is _not_ empty).
Done.
https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc
File lily/separation-item.cc (right):
https://codereview.appspot.com/7310075/diff/53001/lily/separation-item.cc#newcode148
lily/separation-item.cc:148: Interval extra_width = robust_scm2interval
(elts[i]-get_property (extra-spacing-width),
On 2013/03/08 14:24:11, dak wrote:
This is not related to this patch, but isn't it complete nonsense to
use an
Interval here rather than a Drul_array or other form of offset pair?
The LEFT and RIGHT values don't form an interval as far as I can see.
you're right
https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc
File lily/skyline.cc (right):
https://codereview.appspot.com/7310075/diff/53001/lily/skyline.cc#newcode649
lily/skyline.cc:649: return ret;
On 2013/03/08 14:24:11, dak wrote:
Why not just
return *this;
here? The function does not return a reference, so a copy is
constructed
anyway. There is no point in doing yet another copy, is there?
Done.
https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode472
scm/output-lib.scm:472: 1000))
On 2013/03/08 14:24:11, dak wrote:
Don't we have some constants for the full range START and END values?
I don't think we have one for integers. I made one in lily-library.scm
https://codereview.appspot.com/7310075/diff/53001/scm/output-lib.scm#newcode478
scm/output-lib.scm:478: (define-public
pure-from-neighbor-interface::unobtrusive-height
On 2013/03/08 14:24:11, dak wrote:
unobtrusive is a bit obscure: I suspected the only-a-bit-empty
interval at
first. height-if-pure would be quite more descriptive.
Done.
https://codereview.appspot.com/7310075/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Uses only unpure-pure containers to articulate unpure-pure relationships. (issue 7377046)
Some code from my patch for issue 3161 is copied over to this patch. The fact that Grob::pure_height returns Interval (0,0) as default causes point stencils to be placed in the skyline, potentially effecting vertical spacing. Separation_item::boxes uses this (0,0) extent to add extra spacing height to certain grobs. To allow this to still happen, we give the grobs without pure height a small empty extent so that the (almost) full compliment of extra spacing height is taken into account. This is why small-empty-interval exists in lily-library.scm. https://codereview.appspot.com/7377046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Make ly:make-unpure-pure-container accept a single callback (issue 7382046)
LGTM, with one suggestion: the difference between unpure and pure functions in LilyPond is that unpure functions accept n arguments whereas pure functions accept n+2. This shortcut you're proposing only works when n=1, which you document, but it'd be nice if it worked for an arbitrary number of input arguments. It'd only be possible to do this, though, if there were a way in C++ to determine the number of arguments a Scheme function took. https://codereview.appspot.com/7382046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Spacing with empty contexts; issue 1669 (issue 4515158)
https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc File lily/align-interface.cc (right): https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newcode222 lily/align-interface.cc:222: dy = max (dy, min_distance); On 2011/06/09 02:55:55, Keith wrote: On 2011/06/07 04:48:01, joeneeman wrote: If pure is true, there may be some staves that are going to be removed. However, these staves won't be removed until after line-breaking. ... and, as you say, these staves are not yet removed when Y-extent-estimates for the page breaker are calculated, so this patch messes up the page breaker. If the page breaker depends on empty, but living, staves disappearing for vertical spacing purposes, then maybe empty staves should be treated that way consistently. That would be tantamount to giving all empty-able contexts the hara-kiri engraver, as you suggested much earlier. The obvious alternative would be to have the empty staves decide whether to suicide before page breaking, but my suspicion is that is awkward, or it would have been done that way at first. Having minimum_distances() change behavior based on whether 'pure' is true is too tricky for me. (how do we know that any staff that is empty we are pure will suicide later) I'll think on it, but mark the patch as abandoned for now. Couldn't there just be a vectorbool ignore_me that contains the result request_suicide for all elements (when pure)? Then, the for loop would be continued for these elements and they wouldn't have to be removed in get_skylines. https://codereview.appspot.com/4515158/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Standardizes use of empty extents in pure heights and skylines. (issue 7310075)
New patch set coming after I finish running the regtests.
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly
File input/regression/skyline-point-extent.ly (right):
https://codereview.appspot.com/7310075/diff/10/input/regression/skyline-point-extent.ly#newcode7
input/regression/skyline-point-extent.ly:7: }
On 2013/02/10 03:08:04, Keith wrote:
... The accents should follow the descending melody line, even though
the
note-head stencils are empty.}
{ \override NoteHead #'stencil = #point-stencil
c'2.- b8-- a-- g1- }
#(ly:set-option 'debug-skylines)
Regtest changed. I've removed the Stem_engraver in the regtest so that
the only possible side support element is the note head.
https://codereview.appspot.com/7310075/diff/10/lily/grob.cc
File lily/grob.cc (right):
https://codereview.appspot.com/7310075/diff/10/lily/grob.cc#newcode488
lily/grob.cc:488: Interval iv = robust_scm2interval (iv_scm, Interval
());
On 2013/02/10 03:08:04, Keith wrote:
I assume you will apply this change and the change in
separation-item.cc in a
separate commit, the empty extents in pure-heights part of the patch
set.
I'm not sure if these changes can be separated from the skyline stuff
without causing regtest havoc.
I can do this, but it'd take me more time (figure out what changes to
isolate, run regtests, etc.). Is it necessary?
https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc
File lily/separation-item.cc (right):
https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcode153
lily/separation-item.cc:153: x[LEFT] += extra_width[LEFT];
On 2013/02/10 03:08:04, Keith wrote:
// The conventional empty extent is (+inf.0 . -inf.0)
// but (-inf.0 . +inf.0) is used as extra-spacing-height
// on items that must not overlap other note-columns.
// If these two uses of inf combine, leave the empty extent.
if (!isinf(x[LEFT]) x[LEFT] += extra_width[LEFT];
if (!isinf(x[RIGHT]) x[RIGHT] += extra_width[RIGHT];
Done.
https://codereview.appspot.com/7310075/diff/10/lily/separation-item.cc#newcode159
lily/separation-item.cc:159: // empty interval with infinity at either
end
On 2013/02/10 03:08:04, Keith wrote:
Other than the addition above, what other 'operation' can produce a
NaN ?
Comment eliminated with change above.
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc
File lily/skyline.cc (right):
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode59
lily/skyline.cc:59: /* If we start including very thin buildings,
numerical accuracy errors can
On 2013/02/10 03:08:04, Keith wrote:
Did you find where these numerical accuracy errors occured? I don't
see
anything obvious. The most likely seems to be the way we step through
the two
skylines in internal_merge_skylines();
No idea...
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode61
lily/skyline.cc:61: than epsilon wide. In merges, we omit them.
On 2013/02/10 03:08:04, Keith wrote:
Any such buildings are created in merge_skyline() are omitted from the
merged
result.
Done.
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode104
lily/skyline.cc:104: Interval start_end = fatten_skinny_buildings
(start, end);
On 2013/02/10 03:08:04, Keith wrote:
The old-fashioned way would be
// Buildings all have width at least EPS
end = min(end, start + EPS);
and the same in other constructors, but I know how you hate
code-duplication.
But this adds the EPS arbitrarily to the right instead of adding an
equal amount to the right and left...
And I hate code duplication.
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode119
lily/skyline.cc:119:
On 2013/02/10 03:08:04, Keith wrote:
// Buildings all have width at least EPS
end = min(end, start + EPS);
See above.
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode497
lily/skyline.cc:497: Boxes should have fatness in the horizon_axis,
otherwise they are ignored.
On 2013/02/10 03:08:04, Keith wrote:
This comment would no longer belong here
Changed to represent use of EPS as minimum fatness.
https://codereview.appspot.com/7310075/diff/10/lily/skyline.cc#newcode519
lily/skyline.cc:519: Buildings should have fatness in the horizon_axis,
otherwise they are ignored.
On 2013/02/10 03:08:04, Keith wrote:
This comment would no longer belong here
Changed to represent use of EPS as minimum fatness.
https://codereview.appspot.com/7310075/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)
Hey all, After letting this newest round sit for a few days, I'm pretty confident that the patch is ready for review so I'd ask people to review it. First, if anyone needs more comments in the code to let them know what's going on, lemme know where and I'll add it. Second, if people have performance concerns, lemme know - I haven't noticed a slow down but I haven't done rigorous testing. Third, I'm interested to see if people like this approach. The long-term goal is to get rid of translate_axis as much as possible, as using it destroys downstream possibilities to do pure height calculations on grobs. Cheers, MS https://codereview.appspot.com/7185044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)
People can review this if they have time...it is a lot of code, but it is more a progress report than anything else. To resume a bit of what's been said before: once translate_axis is called, the dim_cache of grobs is set and it is very difficult to work with approximations. This makes cross-staff grobs very difficult to deal with in vertical placement. The goal is to eliminate this property call for outside-staff placement so that pure skylines can be used in a first pass without setting the Y-offset. Then, in the second pass, real skylines will be used and the Y-offset will be set. https://codereview.appspot.com/7185044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Eliminates the Hara_kiri_engraver. (issue 7061062)
sneaky callback is not a real concern...it's a pedantic concern. If we're following the model to a T, grob properties should not be read in engravers, just set. The idea is that the engravers are creating the grobs and fixing them up with properties if need be. I prefer duplicating a context and grob property rather than just having the grob property, but people seem against this. It is a trivial fix if people become for it. I think Keith's comment is fixed in this patch set (although I must admit that I don't completely follow it either...this is a new corner of the code for me). https://codereview.appspot.com/7061062/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Eliminates the Hara_kiri_engraver. (issue 7061062)
Reviewers: ,
Message:
This doesn't crash in the test case Keith proposed:
\new StaffGroup \with { \RemoveEmptyStaves
} { b1 b1 b1}
{ b1 b1 \break b1 }
Description:
Eliminates the Hara_kiri_engraver.
Uses a haraKiri context property in the Axis_group_engraver to
acheive the same functionality.
Please review this at https://codereview.appspot.com/7061062/
Affected files:
M lily/axis-group-engraver.cc
D lily/hara-kiri-engraver.cc
D lily/include/axis-group-engraver.hh
M lily/include/lily-proto.hh
M ly/context-mods-init.ly
M ly/engraver-init.ly
M scm/define-context-properties.scm
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Better alignment of MetronomeMark to MultiMeasureRest (issue 6972044)
Reviewers: Keith,
Message:
You're right...it was mostly out of laziness and desperation that I did
the quick fix...it worked for a piece I was typesetting. I have a long
plane ride on the 28th to fix this correctly.
https://codereview.appspot.com/6972044/diff/1/lily/metronome-engraver.cc
File lily/metronome-engraver.cc (right):
https://codereview.appspot.com/6972044/diff/1/lily/metronome-engraver.cc#newcode134
lily/metronome-engraver.cc:134: has_note_column_ = true;
On 2012/12/20 02:40:11, Keith wrote:
Why not just
\override Score.MetronomeMark #'non-break-align-symbols =
#'(note-column-interface multi-measure-rest-interface)
Good call...will do.
Description:
Better alignment of MetronomeMark to MultiMeasureRest
Please review this at https://codereview.appspot.com/6972044/
Affected files:
M lily/break-alignment-interface.cc
M lily/metronome-engraver.cc
Index: lily/break-alignment-interface.cc
diff --git a/lily/break-alignment-interface.cc
b/lily/break-alignment-interface.cc
index
242e39eacc7c35692d113ef7ea97d946f0a8da97..4a3ac4966213054b90f292cdff61c4e5f5c5a370
100644
--- a/lily/break-alignment-interface.cc
+++ b/lily/break-alignment-interface.cc
@@ -265,7 +265,15 @@ Break_alignable_interface::self_align_callback (SCM
grob)
Grob *me = unsmob_grob (grob);
Item *alignment = dynamic_castItem * (me-get_parent (X_AXIS));
if (!Break_alignment_interface::has_interface (alignment))
-return scm_from_int (0);
+{
+ // If we're aligning to a break aligned, then we
+ // give up and shift as to-the-right as possible of its break
alignment.
+ // If there is none, we give up entirely.
+ if (Break_alignment_interface::has_interface (alignment-get_parent
(X_AXIS)))
+return scm_from_double (alignment-get_parent (X_AXIS)-extent
(alignment-get_parent (X_AXIS), X_AXIS)[RIGHT]);
+ else
+return scm_from_int (0);
+}
SCM symbol_list = me-get_property (break-align-symbols);
vectorGrob * elements = Break_alignment_interface::ordered_elements
(alignment);
Index: lily/metronome-engraver.cc
diff --git a/lily/metronome-engraver.cc b/lily/metronome-engraver.cc
index
b4cd2ce328b89707ac7497ed0cff6ca18a6ab1ec..84567b6575a275efcc3b70b5281d9223f0cf02a0
100644
--- a/lily/metronome-engraver.cc
+++ b/lily/metronome-engraver.cc
@@ -37,6 +37,8 @@ class Metronome_mark_engraver : public Engraver
Item *text_;
Grob *support_;
Grob *bar_;
+ Grob *break_alignment_;
+ bool has_note_column_;
Stream_event *tempo_ev_;
public:
@@ -49,6 +51,7 @@ protected:
DECLARE_ACKNOWLEDGER (break_aligned);
DECLARE_ACKNOWLEDGER (break_alignment);
DECLARE_ACKNOWLEDGER (grob);
+ DECLARE_ACKNOWLEDGER (note_column);
DECLARE_TRANSLATOR_LISTENER (tempo_change);
};
@@ -59,6 +62,8 @@ Metronome_mark_engraver::Metronome_mark_engraver ()
support_ = 0;
bar_ = 0;
tempo_ev_ = 0;
+ break_alignment_ = 0;
+ has_note_column_ = false;
}
IMPLEMENT_TRANSLATOR_LISTENER (Metronome_mark_engraver, tempo_change);
@@ -106,6 +111,8 @@ Metronome_mark_engraver::acknowledge_break_alignment
(Grob_info info)
support_
dynamic_castItem * (g))
text_-set_parent (g, X_AXIS);
+
+ break_alignment_ = g;
}
void
@@ -122,15 +129,22 @@ Metronome_mark_engraver::acknowledge_grob (Grob_info
info)
}
void
+Metronome_mark_engraver::acknowledge_note_column (Grob_info /* info */)
+{
+ has_note_column_ = true;
+}
+
+void
Metronome_mark_engraver::stop_translation_timestep ()
{
if (text_)
{
if (text_-get_parent (X_AXIS)
text_-get_parent (X_AXIS)-internal_has_interface
(ly_symbol2scm (multi-measure-rest-interface))
- bar_)
+ (bar_ || break_alignment_))
+//text_-set_parent (break_alignment_ ? break_alignment_ : bar_,
X_AXIS);
text_-set_parent (bar_, X_AXIS);
- else if (!support_)
+ if (!support_ has_note_column_)
{
/*
Gardner Read Music Notation, p.278
@@ -150,7 +164,9 @@ Metronome_mark_engraver::stop_translation_timestep ()
support_ = 0;
bar_ = 0;
tempo_ev_ = 0;
+ break_alignment_ = 0;
}
+ has_note_column_ = false;
}
void
@@ -172,6 +188,7 @@ Metronome_mark_engraver::process_music ()
ADD_ACKNOWLEDGER (Metronome_mark_engraver, break_aligned);
ADD_ACKNOWLEDGER (Metronome_mark_engraver, break_alignment);
ADD_ACKNOWLEDGER (Metronome_mark_engraver, grob);
+ADD_ACKNOWLEDGER (Metronome_mark_engraver, note_column);
ADD_TRANSLATOR (Metronome_mark_engraver,
/* doc */
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Checks for recursive element behavior (issue 6943072)
Reviewers: ,
Message:
Hey all,
I'm ok w/ this on the countdown but can someone check out David's
suspicion that this slows stuff down by O(n^3)? I definitely won't push
this if it slows LilyPond down to a crawl.
Cheers,
MS
Description:
Checks for recursive element behavior
Please review this at https://codereview.appspot.com/6943072/
Affected files:
M lily/engraver.cc
M lily/include/pointer-group-interface.hh
M lily/pointer-group-interface.cc
Index: lily/engraver.cc
diff --git a/lily/engraver.cc b/lily/engraver.cc
index
db1303d63ce3b918f95e5d9f254c589a84bf89ea..11d080eb6e061a26c23356a825c89e4b8d434731
100644
--- a/lily/engraver.cc
+++ b/lily/engraver.cc
@@ -123,7 +123,6 @@ Engraver::internal_make_grob (SCM symbol,
SCM handle = scm_sloppy_assq (ly_symbol2scm (meta), props);
SCM klass = scm_cdr (scm_sloppy_assq (ly_symbol2scm (class), scm_cdr
(handle)));
-
if (klass == ly_symbol2scm (Item))
grob = new Item (props);
else if (klass == ly_symbol2scm (Spanner))
Index: lily/include/pointer-group-interface.hh
diff --git a/lily/include/pointer-group-interface.hh
b/lily/include/pointer-group-interface.hh
index
e265bedc79d0cf29d72debc26af7c7eaa0d242b2..1970b1073fbddea5d63d6e02118bcd5ad0c906d2
100644
--- a/lily/include/pointer-group-interface.hh
+++ b/lily/include/pointer-group-interface.hh
@@ -34,6 +34,7 @@ public:
static void set_ordered (Grob *, SCM, bool);
static Grob_array *get_grob_array (Grob *, SCM);
static Grob *find_grob (Grob *, SCM, bool (*pred) (Grob *));
+ static bool has_in_element_chain (Grob *needle, Grob *hay);
};
vectorGrob * const internal_extract_grob_array (Grob const *elt, SCM
symbol);
Index: lily/pointer-group-interface.cc
diff --git a/lily/pointer-group-interface.cc
b/lily/pointer-group-interface.cc
index
045563d457b9acd572a203788fb5dea3d4dab336..11deb42bee9f12e0d166b0db749d6d111d9cabb6
100644
--- a/lily/pointer-group-interface.cc
+++ b/lily/pointer-group-interface.cc
@@ -21,6 +21,7 @@
#include grob-array.hh
#include grob.hh
+#include international.hh
int
Pointer_group_interface::count (Grob *me, SCM sym)
@@ -68,9 +69,32 @@ Pointer_group_interface::find_grob (Grob *me, SCM sym,
bool (*pred) (Grob *))
return 0;
}
+bool
+Pointer_group_interface::has_in_element_chain (Grob *hay, Grob *needle)
+{
+ if (!needle || !hay)
+return false;
+
+ if (needle == hay)
+return true;
+ extract_grob_set(hay, elements, haystack);
+ for (vsize i = 0; i haystack.size (); i++)
+{
+ if (has_in_element_chain (haystack[i], needle))
+return true;
+}
+ return false;
+}
+
void
Pointer_group_interface::add_grob (Grob *me, SCM sym, Grob *p)
{
+ if (sym == ly_symbol2scm (elements) has_in_element_chain (p, me))
+{
+ me-programming_error (_f (Cowardly refusing to add %s to the
elements list of %s, which would result in recursive behavior., p-name
().c_str (), me-name ().c_str ()));
+ return;
+}
+
Grob_array *arr = get_grob_array (me, sym);
arr-add (p);
}
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
Reviewers: aleksandr.andreev,
Message:
I think a user should be able to use Kievan notation and normal
stems/flags/beams if she so chooses. I'll define something like
startKievanNotation = {
%% Set glyph styles.
\override NoteHead #'style = #'kievan
\override Rest #'style = #'mensural
\override Accidental #'glyph-name-alist =
#alteration-kievan-glyph-name-alist
\override Dots #'style = #'kievan
\override Slur #'stencil = ##f
%% There are beams in Kievan notation, but they are invoked manually
\set autoBeaming = ##f
}
stopKievanNotation = {
\revert NoteHead #'style
\revert Rest #'style
\revert Accidental #'glyph-name-alist
\revert Dots #'style
\revert Slur #'stencil
\unset autoBeaming
}
and then use it in the documentation.
Description:
Various clean-ups in stems and beams.
*) Eliminates code dups for Kievan work.
*) Transfers functions that are called a lot to C++ to speed things up.
*) Eliminates unused variables.
No regtest changes.
Please review this at https://codereview.appspot.com/6584045/
Affected files:
A input/regression/kievan-notation.ly
M lily/include/beam.hh
M lily/include/stem.hh
M lily/stem.cc
M ly/engraver-init.ly
M scm/define-grobs.scm
M scm/output-lib.scm
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Caches grob properties before evaluation. (issue 6573066)
Reviewers: ,
Message:
This slows down LilyPond - I haven't done comprehensive tests of how
much. I'm pretty sure it works (the regtest works as expected).
Irrespective of how multiple passes are done, this seems like a
necessary first step.
Note that this patch would not have a time impact if it used properties
stashed in immutable_properties_alist_. If all side effects were
eliminated from LilyPond (which would be awesome), then this could be
done. However, this would be very difficult - for example, tweaks are
given to grobs after their immutable_properties_alist_ is fixed (see the
tweak engraver).
Description:
Caches grob properties before evaluation.
We cannot simply use the original value stored in the
immutable_properties_alist_ because simple closures may be created
at the engraver stage via chain_offset_callback and related functions.
Instead, we grab the property data at the moment of evaluation and cache
it.
Please review this at https://codereview.appspot.com/6573066/
Affected files:
A input/regression/restore-property-to-original-value.ly
M lily/grob-property.cc
M lily/grob-scheme.cc
M lily/grob-smob.cc
M lily/grob.cc
M lily/include/grob.hh
M lily/include/lily-guile-macros.hh
Index: input/regression/restore-property-to-original-value.ly
diff --git a/input/regression/restore-property-to-original-value.ly
b/input/regression/restore-property-to-original-value.ly
new file mode 100644
index
..1a7c1df1e1e7bd190f844ad5b6e4d7068147e775
--- /dev/null
+++ b/input/regression/restore-property-to-original-value.ly
@@ -0,0 +1,24 @@
+\version 2.17.4
+
+\header {
+ texidoc = Properties can be restored to their original values.
+Below, the @code{NoteHead} stencil should print but its extent
+should not be taken into account in horizontal spacing, as horizontal
+spacing happens between calls to @code{before-line-breaking} and
+@code{after-line-breaking}.
+
+}
+
+\relative c'' {
+ \override NoteHead #'before-line-breaking =
+#(lambda (grob)
+ ; trigger cache
+ (ly:grob-property grob 'stencil)
+ ; change the property
+ (ly:grob-set-property! grob 'stencil #f))
+ \override NoteHead #'after-line-breaking =
+#(lambda (grob)
+ ; restore the property
+ (ly:grob-restore-property-to-original-value! grob 'stencil))
+ a
+}
Index: lily/grob-property.cc
diff --git a/lily/grob-property.cc b/lily/grob-property.cc
index
3afe182c0ef07793ecf1fd0d09a5ea147c9bd4e1..826b1e3f11d19d4aaaca7e13de65f20326e1b9d7
100644
--- a/lily/grob-property.cc
+++ b/lily/grob-property.cc
@@ -164,6 +164,15 @@ SCM
Grob::internal_get_property (SCM sym) const
{
SCM val = get_property_data (sym);
+ // if this is our first lookup, set the cache
+ SCM cached_handle = scm_sloppy_assq (sym,
+ cached_mutable_property_alist_);
+ if (cached_handle == SCM_BOOL_F)
+{
+ Grob *me = ((Grob *)this);
+ me-internal_set_value_on_alist (me-cached_mutable_property_alist_,
+ sym, val);
+}
#ifndef NDEBUG
if (val == ly_symbol2scm (calculation-in-progress))
@@ -211,6 +220,21 @@ Grob::internal_get_maybe_pure_property (SCM sym, bool
pure, int start, int end)
return pure ? internal_get_pure_property (sym, start, end) :
internal_get_property (sym);
}
+void
+Grob::internal_restore_property_to_original_value (SCM sym)
+{
+ SCM handle = scm_sloppy_assq (sym, cached_mutable_property_alist_);
+ // We could issue warning if handle == SCM_BOOL_F, but it is possible
+ // that one wants to restore an original property before the property
+ // has been evaluated. By definition, this means that the original
+ // property is still there.
+
+ if (handle != SCM_BOOL_F)
+internal_set_value_on_alist (mutable_property_alist_,
+ sym, scm_cdr (handle));
+
+}
+
SCM
Grob::try_callback_on_alist (SCM *alist, SCM sym, SCM proc)
{
Index: lily/grob-scheme.cc
diff --git a/lily/grob-scheme.cc b/lily/grob-scheme.cc
index
81ff7864ff36b4b4ff972bb0fc3051d8700f60e5..e144be6f6a0884acc07f7f7408e59834731314e5
100644
--- a/lily/grob-scheme.cc
+++ b/lily/grob-scheme.cc
@@ -179,6 +179,20 @@ LY_DEFINE (ly_grob_set_object_x, ly:grob-set-object!,
return SCM_UNSPECIFIED;
}
+LY_DEFINE
(ly_grob_restore_property_to_original_value_x, ly:grob-restore-property-to-original-value!,
+ 2, 0, 0, (SCM grob, SCM sym),
+ Restore the property for property @var{sym} of @var{grob}.
+to its original value.)
+{
+ Grob *sc = unsmob_grob (grob);
+
+ LY_ASSERT_SMOB (Grob, grob, 1);
+ LY_ASSERT_TYPE (ly_is_symbol, sym, 2);
+
+ sc-restore_property_to_original_value (sym);
+ return SCM_UNSPECIFIED;
+}
+
/* TODO: make difference between scaled and unscalead variable in
calling (i.e different funcs.) */
LY_DEFINE (ly_grob_layout, ly:grob-layout,
Index: lily/grob-smob.cc
diff --git a/lily/grob-smob.cc
Re: spacing-spanner: rods for non-adjacent paper-columns; issue 1700 (issue 6489107)
LGTM - looking forward to the skyline version, as that'll more accurately reflect where columns are overhanging. http://codereview.appspot.com/6489107/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Corrects beamed cross-staff stems' pure heights (issue 6543046)
Reviewers: Keith,
Message:
I'm gonna hold off on posting to this one as it evolves constantly with
my work on issue 2801, which is revealing several issues in cross-staff
stem calculations. Irrespective of how issue 2801 pans out, I'll take
all the relevant code from stem.cc and either push it as a separate
commit or include it in this Rietveld if my patch for 2801 isn't LGTM'd.
Description:
Corrects beamed cross-staff stems' pure heights
An incorrect subtraction made it such that the pure heighs were
too long for stems on extremal staves and too short on interior
staves.
Please review this at http://codereview.appspot.com/6543046/
Affected files:
M lily/stem.cc
Index: lily/stem.cc
diff --git a/lily/stem.cc b/lily/stem.cc
index
8069a456c814e48cf24bddec96bf9d8fae2279ac..43894123d7fc329bde7f9694edba29f042a8d298
100644
--- a/lily/stem.cc
+++ b/lily/stem.cc
@@ -323,6 +323,7 @@ Stem::internal_pure_height (Grob *me, bool calc_beam)
vectorInterval heights;
vectorGrob * my_stems;
extract_grob_set (beam, normal-stems, normal_stems);
+ // find the internal pure heights of all stems pointing in this
direction
for (vsize i = 0; i normal_stems.size (); i++)
if (get_grob_direction (normal_stems[i]) == dir)
{
@@ -332,23 +333,27 @@ Stem::internal_pure_height (Grob *me, bool calc_beam)
heights.push_back (iv);
my_stems.push_back (normal_stems[i]);
}
- //iv.unite (heights.back ());
- // look for cross staff effects
+
vectorReal coords;
Grob *common = common_refpoint_of_array (my_stems, me, Y_AXIS);
Real min_pos = infinity_f;
Real max_pos = -infinity_f;
+
+ // find the offset of various staves as well as the minimum and
maximum offset
for (vsize i = 0; i my_stems.size (); i++)
{
coords.push_back (my_stems[i]-pure_relative_y_coordinate
(common, 0, INT_MAX));
min_pos = min (min_pos, coords[i]);
max_pos = max (max_pos, coords[i]);
}
+ // Tack on the difference of the current stem's minimum-translation
and the
+ // lowest or highest minimum translation to get the stem up to the
axis
+ // group where the beam lies
for (vsize i = 0; i heights.size (); i++)
{
heights[i][dir] += dir == DOWN
- ? coords[i] - max_pos
- : coords[i] - min_pos;
+ ? min_pos - coords[i]
+ : max_pos - coords[i];
}
for (vsize i = 0; i heights.size (); i++) iv.unite (heights[i]);
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows user to set ChordName text (issue 6496085)
Reviewers: dak,
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode83
lily/chord-name-engraver.cc:83: || ly_is_procedure
(chord_name_-get_property (text)));
On 2012/09/06 08:50:40, dak wrote:
If it is a procedure, shouldn't it be called with the calculated
value?
Right you are - I should have used get_property_data. Will fix.
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149: ly_is_equal (chord_as_scm,
last_chord_))
On 2012/09/06 08:50:40, dak wrote:
If one is doing the chord calculation manually, you can't make the
decision of
whether a chord changed based on the automatic calculation. For
better or
worse, you need to compare the computed chord versions/text.
To respond to your points above, I don't throw away the values above
because they're used here. As for the present point, that is an
interesting conundrum...I'll drum up some logic for that. That may
eliminate the need for guarding the values above having to do with chord
changes, in which point the if else statement will be able to be
simplified.
Description:
Allows user to set ChordName text
Please review this at http://codereview.appspot.com/6496085/
Affected files:
A input/regression/chord-name-override-text.ly
M lily/chord-name-engraver.cc
Index: input/regression/chord-name-override-text.ly
diff --git a/input/regression/chord-name-override-text.ly
b/input/regression/chord-name-override-text.ly
new file mode 100644
index
..4acd549394eff095065c0b9605473424ef13c649
--- /dev/null
+++ b/input/regression/chord-name-override-text.ly
@@ -0,0 +1,13 @@
+\version 2.17.2
+
+\header {
+ texidoc = Users can override the @code{text} property of
+@code{ChordName}.
+
+}
+
+\new ChordNames \chordmode {
+ a b c:7
+ \once \override ChordName #'text = #foo
+ d
+}
\ No newline at end of file
Index: lily/chord-name-engraver.cc
diff --git a/lily/chord-name-engraver.cc b/lily/chord-name-engraver.cc
index
eab4d94d075c52ef95f0ae09449d6d9d184585c5..6aad475ebb5a0f61292a1d58c8fe68aafe64c1c6
100644
--- a/lily/chord-name-engraver.cc
+++ b/lily/chord-name-engraver.cc
@@ -76,7 +76,14 @@ Chord_name_engraver::process_music ()
SCM inversion = SCM_EOL;
SCM pitches = SCM_EOL;
- if (rest_event_)
+ chord_name_ = make_item (ChordName,
+ rest_event_ ? rest_event_-self_scm () :
notes_[0]-self_scm ());
+
+ bool make_markup = !(Text_interface::is_markup
(chord_name_-get_property (text))
+ || ly_is_procedure (chord_name_-get_property
(text)));
+
+ if (rest_event_ !make_markup) { }
+ else if (rest_event_)
{
SCM no_chord_markup = get_property (noChordSymbol);
if (!Text_interface::is_markup (no_chord_markup))
@@ -125,17 +132,17 @@ Chord_name_engraver::process_music ()
pitches = scm_sort_list (pitches, Pitch::less_p_proc);
SCM name_proc = get_property (chordNameFunction);
- markup = scm_call_4 (name_proc, pitches, bass, inversion,
- context ()-self_scm ());
+ if (make_markup)
+markup = scm_call_4 (name_proc, pitches, bass, inversion,
+ context ()-self_scm ());
}
/*
Ugh.
*/
SCM chord_as_scm = scm_cons (pitches, scm_cons (bass, inversion));
- chord_name_ = make_item (ChordName,
- rest_event_ ? rest_event_-self_scm () :
notes_[0]-self_scm ());
- chord_name_-set_property (text, markup);
+ if (make_markup)
+chord_name_-set_property (text, markup);
SCM chord_changes = get_property (chordChanges);
if (to_boolean (chord_changes) scm_is_pair (last_chord_)
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
The speed problem was twofold - some cruft in callbacks coupled with the fact that I wasn't doing make cleans, so something about the way that gcc was putting together the old .o files was slowing things down. I learned my lesson: always do make clean before testing a patch. A full make clean brings compilation time on Keith's test case down to: PATCH real0m7.324s user0m6.944s sys 0m0.192s CURRENT MASTER real0m7.407s user0m6.828s sys 0m0.304s Usually about a 3% increase for scores with lots of cross staff slurs, almost nothing otherwise. Note that this is with an optimized build that is not debugging or profiled enabled. All of those flags add about 4ish%. http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/03 13:46:33, mike7 wrote:
On 3 sept. 2012, at 07:07, mailto:mts...@gmail.com wrote:
On 2012/09/02 20:38:28, Keith wrote:
On 2012/09/02 06:25:58, MikeSol wrote:
It's not a copy of the original slur because it is using
pure heights and offsets.
I saw you interrogating SlurStub regarding its purity, but did not
notice that
SlurStub took any different shape based on 'pure' estimates.
The SlurStubs in the regtest were shaped just like the printed
Slurs,
but now I
see the difference in the Chopin prelude, with
\layout { \context { \Score
\override SlurStub #'color = #green
\override SlurStub #'transparent = ##f
\override Slur #'color = #darkgreen
\override PhrasingSlur #'color = #darkgreen }}
The SlurStubs are not sufficiently accurate to help, and they cost
me
time.
Chopin op45: patch 'skylines' 2.16
real0m21.666s 0m16.245s 0m12.862s
user0m20.814s 0m15.573s 0m12.232s
sys 0m0.240s0m0.254s0m0.217s
I'm working on a new version of the patch. There is no way to
improve
accuracy of the curve, but the Y-offset is often wrong and that can
be
improved.
The time hike seems awfully expensive - there must be a way to
optimize
it. I'll post something that works when I have it and then we can
figure out how to optimize it.
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
mailto:lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
New version posted. I'm also getting a 25% increase on the Op. 45
test case.
I am getting severe hikes across the board, though, even in scores
without cross
staff slurs. What's odd is that the hikes are happening in the
Drawing
systems phase (I think), although I'm positive via prints to the
command line
that the SlurStub's control points and vertical skylines are never
being
calculated.
Lemme know if you see something slipping through the cracks - gprof
shows a hike
in stencil integral and skyline code for my patch, so it must be
generating
extra skylines somewhere.
Cheers,
MS
After doing some digging, it looks like that even if not a single new
grob is created (meaning if I comment out all SlurStub stuff in the
engraver), the time increase is still there. It is tough to see where
this is coming from using profiling tools, but it seems like the hike is
in the Drawing Systems phase. I'm guessing that it may have to do with
all of the maybe_pure stuff. If anyone has time to do profiling I'd
appreciate help on this patch - it leads to an appreciable improvement
in several scores I've checked out w/ cross-staff slurs, but it
obviously can't be put on a countdown until the performance hit gets
taken care of.
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
Reviewers: Keith, Message: On 2012/09/01 23:58:37, Keith wrote: I might have a test case for you at http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776 It seems you copy each slur into a slur-stub, and from those keep only the ones with cross-staff set. Then when figuring system skylines you insert all Grobs with the slur-stub-interface into the skylines for each staff. Why not insert the original Slur into the skylines, if it has cross-staff instead of the SlurStub. For that matter, why not insert all Grobs marked cross-staff? all the cross-staff It's not a copy of the original slur because it is using pure heights and offsets. The original slur can't be inserted into the VerticalAxisGroup skylines because its height and offset depends on the distance between axis groups, which is not known at the time the skyline is created. So we use the pure version (the stub) to build the system skylines in page-layout-problem.cc. Note that we don't use it for the minimum-translations (I made a minimum-translation-vertical-skylines property for that) because we don't want cross-staff grobs to affect the distance between two axis groups in the same system. We want them just to appear in the aggregate of all vertical axis groups smooshed into one system, created in the function Page_layout_problem::build_system_skyline. The test case shows that the patch is not ready - I need to make it overshoot less. But the general idea will remain the same. Description: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. The approximation is almost always an overshoot for the time being. A more robust strategy would be to create SlurStubs for every VerticalAxisGroup on which the cross-staff slur encompassed a note column. Then, the skyline added would be the union of all these skylines where the top skyline acted as a top bound and the low skyline acted as a lower bound, so if intermediary skylines went over these bounds, those parts of the skyline would be thrown out. This can be done in a separate commit: it'd require making a Skyline::crop function that crops one skyline by another and would require creating multiple SlurStubs in the engraver as well as figuring out the right amount to shift them for each vertical axis group. Please review this at http://codereview.appspot.com/6498077/ Affected files: A input/regression/cross-staff-slur-vertical-spacing.ly M lily/align-interface.cc M lily/axis-group-interface.cc M lily/figured-bass-position-engraver.cc M lily/grob.cc M lily/include/axis-group-interface.hh M lily/include/grob.hh M lily/include/slur-scoring.hh M lily/include/slur.hh M lily/melody-engraver.cc M lily/phrasing-slur-engraver.cc M lily/slur-engraver.cc M lily/slur-scoring.cc M lily/slur.cc M lily/tab-tie-follow-engraver.cc M scm/define-grob-interfaces.scm M scm/define-grob-properties.scm M scm/define-grobs.scm M scm/output-lib.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
Thanks for the review! http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right): http://codereview.appspot.com/6498077/diff/21/lily/axis-group-interface.cc#newcode419 lily/axis-group-interface.cc:419: */ On 2012/09/02 15:59:00, dak wrote: See above. Removed - was old code I needed to read to make sure I waas getting stuff right. Forgot it waas there. http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc File lily/page-layout-problem.cc (right): http://codereview.appspot.com/6498077/diff/21/lily/page-layout-problem.cc#newcode1004 lily/page-layout-problem.cc:1004: if (is_spaceable (staves[i])) On 2012/09/02 15:59:00, dak wrote: This indentation is simply wrong and does not match the program structure. Cruft. Fixed. http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc File lily/phrasing-slur-engraver.cc (right): http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode119 lily/phrasing-slur-engraver.cc:119: if (slur_stubs_.find (slurs_[j]) == slur_stubs_.end ()) On 2012/09/02 15:59:00, dak wrote: It is quite nonsensical to have slur_stubs be a map indexed via slurs_[j] rather than just an array indexed through j. That is inefficient use of time, memory, and complexity. It is sensical because: --) It avoids having two vectors (slur_stubs_ and end_slur_stubs_ would be needed), thus making the code easier to read and maintain. --) It has a find method built into it. Unless people are crunching scores with thousands of slurs on Strong Bad's original Tandy 400, I'm not too worried about the tradeoff. http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode332 lily/phrasing-slur-engraver.cc:332: mapGrob *, Grob * vag_to_slur; On 2012/09/02 15:59:00, dak wrote: Again: why a map here? find method, see above. I know that find exists for vectors in algorithm, but I think this is easier to read and more consistent. http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc File lily/script-column.cc (right): http://codereview.appspot.com/6498077/diff/21/lily/script-column.cc#newcode60 lily/script-column.cc:60: if (unsmob_grob (i1-get_object (slur)) unsmob_grob (i2-get_object (slur))) On 2012/09/02 15:59:00, dak wrote: If both scripts have a field slur that is a grob, return #t is the avoid-slur property of the second script is outside or around while that of the first is neither. Why don't you write this in a comment? Why this complex code? And what is the ultimate purpose? I didn't put it in a comment because it can be understood by reading it, as you explain with your eloquent prose :-) This exists to avoid the situation where a grob has a lower slur priority than another but is typeset outside a slur whereas the other is put inside, leading to a contradiction. Priority is given to the slur directive. http://codereview.appspot.com/6498077/diff/21/lily/slur.cc File lily/slur.cc (right): http://codereview.appspot.com/6498077/diff/21/lily/slur.cc#newcode623 lily/slur.cc:623: thickness On 2012/09/02 15:59:00, dak wrote: No entry for surrogate? scm/define-grob-interfaces.scm http://codereview.appspot.com/6498077/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Dynamics do not unnecessarily horizontal shift for stems. (issue 6493073)
Reviewers: dak,
Message:
Thanks for the review!
http://codereview.appspot.com/6493073/diff/1/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):
http://codereview.appspot.com/6493073/diff/1/lily/self-alignment-interface.cc#newcode213
lily/self-alignment-interface.cc:213: vector_sort (vais, lessint ());
On 2012/09/02 16:06:30, dak wrote:
Seriously? If dir is UP, you are interested in the minimum, and if it
is DOWN,
you are interested in the maximum. And you create a vector and sort
it for
that?
Totally inefficient as well as incomprehensible.
Good catch - changed to Interval_tint
Description:
Dynamics do not unnecessarily horizontal shift for stems.
Please review this at http://codereview.appspot.com/6493073/
Affected files:
A input/regression/dynamics-avoid-cross-staff-stem-2.ly
M lily/self-alignment-interface.cc
Index: input/regression/dynamics-avoid-cross-staff-stem-2.ly
diff --git a/input/regression/dynamics-avoid-cross-staff-stem-2.ly
b/input/regression/dynamics-avoid-cross-staff-stem-2.ly
new file mode 100644
index
..e549646f75df457d18513c7b0bc5f9f24adcad1b
--- /dev/null
+++ b/input/regression/dynamics-avoid-cross-staff-stem-2.ly
@@ -0,0 +1,23 @@
+\version 2.17.2
+
+\header {
+ texidoc = Dynamics do not horizontally shift when attached to
+an axis-group extremal cross staff grob that's extremal side
+(UP or DOWN) is the same as its direction.
+
+}
+
+\new PianoStaff
+ \new Staff = up {
+s1 |
+ }
+ \new Staff = down {
+\clef bass
+\stemDown
+% keep staff alive
+c,, c,8 [ c,, c,8_\f
+\change Staff = up
+g' g' ]
+r2 |
+ }
+
Index: lily/self-alignment-interface.cc
diff --git a/lily/self-alignment-interface.cc
b/lily/self-alignment-interface.cc
index
a37b5871007eec8ef9368976958997b6a4c84a98..2a033340ff27c283350c43cb3fc7ffba7cedf2d6
100644
--- a/lily/self-alignment-interface.cc
+++ b/lily/self-alignment-interface.cc
@@ -199,7 +199,24 @@ Self_alignment_interface::avoid_colliding_grobs (Grob
*me, Axis a, Real offset)
Interval iv = me-extent (me, a) + offset;
for (vsize i = 0; i colls.size (); i++)
-ivs.push_back (colls[i]-extent (refp, a));
+{
+ int my_vai = Grob::get_vertical_axis_group_index (colls[i]);
+ Direction dir = get_grob_direction (colls[i]);
+ // if coll is cross staff but extremal and poiting in the
+ // direction of the extrema, we don't take it into consideration
+ if (Grob *beam = unsmob_grob (colls[i]-get_object (beam)))
+{
+ Interval_tint vais;
+ extract_grob_set (beam, normal-stems, stems);
+ for (vsize j = 0; j stems.size (); j++)
+vais.add_point (Grob::get_vertical_axis_group_index
(stems[j]));
+ // ugh...up and down are different for VerticalAxisGroup order...
+ if ((my_vai == vais[DOWN] dir == UP)
+ || (my_vai == vais[UP] dir == DOWN))
+continue;
+}
+ ivs.push_back (colls[i]-extent (refp, a));
+}
Interval_minefield minefield (Interval (iv.center (), iv.center ()),
iv.length ());
for (vsize i = 0; i ivs.size (); i++)
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)
On 2012/09/02 20:38:28, Keith wrote:
On 2012/09/02 06:25:58, MikeSol wrote:
It's not a copy of the original slur because it is using
pure heights and offsets.
I saw you interrogating SlurStub regarding its purity, but did not
notice that
SlurStub took any different shape based on 'pure' estimates.
The SlurStubs in the regtest were shaped just like the printed Slurs,
but now I
see the difference in the Chopin prelude, with
\layout { \context { \Score
\override SlurStub #'color = #green
\override SlurStub #'transparent = ##f
\override Slur #'color = #darkgreen
\override PhrasingSlur #'color = #darkgreen }}
The SlurStubs are not sufficiently accurate to help, and they cost me
time.
Chopin op45:patch 'skylines' 2.16
real 0m21.666s 0m16.245s 0m12.862s
user 0m20.814s 0m15.573s 0m12.232s
sys0m0.240s0m0.254s0m0.217s
I'm working on a new version of the patch. There is no way to improve
accuracy of the curve, but the Y-offset is often wrong and that can be
improved.
The time hike seems awfully expensive - there must be a way to optimize
it. I'll post something that works when I have it and then we can
figure out how to optimize it.
http://codereview.appspot.com/6498077/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
bar-line interface part 2/2 (issue 6498052)
LGTM http://codereview.appspot.com/6498052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Ledger-line-spanner: symmetric extents; issue 2493 (issue 6490043)
http://codereview.appspot.com/6490043/diff/7001/lily/ledger-line-spanner.cc File lily/ledger-line-spanner.cc (right): http://codereview.appspot.com/6490043/diff/7001/lily/ledger-line-spanner.cc#newcode50 lily/ledger-line-spanner.cc:50: Paper_column::get_rank (previous_column))) I'm having trouble following this naming convention. It seems that previous_column should have a rank falling before current column, not after. Also, it seems like if the variables are called this, then the order should be correct when they are passed into the function (meaning the one supposed to come before the other comes before the other). Otherwise a programming error should be raised. What would be the case where the ranks of the columns would be reversed and what would justify that happening? http://codereview.appspot.com/6490043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
Thanks for the performance tests! I have no problem changing the
function avoid_outside_staff_collisions to be faster - it's just that I
haven't wrapped my head around how distance alone can do it.
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode945
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/18 02:42:37, Keith wrote:
On 2012/08/17 17:16:25, MikeSol wrote:
On 2012/08/17 08:12:56, Keith wrote:
If you comment out the rider business, the vertical-skylines will
not be
correct
for axis-group-interface.
That is very subtle, very minor, changes only the debug output, not
what would
normally be printed from that example, and is different from the
comment
indicates.
\relative c'' {
\override TupletBracket #'outside-staff-priority = #1
\override TupletNumber #'font-size = #5
\times 2/3 { a4\trill a\trill^foo a\trill }
}
I added this as a regtest. If you comment out the rider business,
you'll see that foo is printed on top of the tuplet number.
http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode697
lily/axis-group-interface.cc:697: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance ((*pair)[-d]));
On 2012/08/18 02:42:37, Keith wrote:
When d = dir, the direction we intend to move, this tells us how far
we need to
move, but when d = -dir, this tells us by how far we have encroached
into the
next obstacle, which we will eventually need to hop over.
What you really want to put on the bumps list is how far we need to
move to hope
over this next obstacle. But we only tested the relevant skylines for
'intersection' ... if only we had stached them distance().
I'm not sure what you mean when you say we only tested the relevant
skylines for intersection ... if only we had stached them distance() -
one because I don't know what stached means in this context (from urban
dictionary: The art of placing a ficticious mustache on an individual or
oneself, drawing laughter and humor to this occurance.) and two because
every intersection call is followed by a distance call, so there is
bumping going on in this case.
http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode701
lily/axis-group-interface.cc:701: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance (to_flip));
On 2012/08/18 02:42:37, Keith wrote:
This finds the distance to move such that the top skyline of the new
object just
encloses, Russian egg doll style, one of the already-placed skylines.
We never
want that to be the final position, so don't put it on the bumps list.
That is a possible scenario, but Russian egg doll style enclosing is
tested for below, so even if this does happen it'd be rectified on the
next pass.
http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc#newcode732
lily/axis-group-interface.cc:732: pair-raise (min_bump);
On 2012/08/18 02:42:37, Keith wrote:
printf(bumping %.3f\n, min_bump);
shows that for a simple case
\relative f {f'^mouse c'' f,,^K d''}
we bump around quite a lot:
mouse
bumping 0.567
bumping 1.894
EEEK
bumping 0.443
bumping 0.048
bumping 0.095
bumping 0.191
bumping 0.382
bumping 0.459
bumping 1.344
bumping 0.161
bumping 0.322
bumping 0.578
Agreed. I'd love for this to go faster.
I am for getting this into current master ASAP so that people can work
on speeding it up - I'm not at all adverse to the idea of using distance
the way you're talking about, but I don't completely understand how it'd
work yet and it seems like something that could be done in a separate
commit. What'd be nice about having this in master is that it'd be
easier to spot in the regtests if efficiency changes to the algorithm
have a layout impact. Of course, one could run regtests with this patch
as a baseline, but it is a pain.
http://codereview.appspot.com/5626052/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
I've forgotten why I added the horizontal skyline stuff so I've taken it
out in a new version that I'll post after regtests.
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode658
lily/axis-group-interface.cc:658: are avoided
On 2012/08/17 11:19:26, joeneeman wrote:
Have you measured the costs of not having exempt?
I've decided to scrap exempt in a new version - will upload soon.
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode780
lily/axis-group-interface.cc:780: while (dirty);
On 2012/08/17 08:12:56, Keith wrote:
I am beginning to understand the new code. Would you consider again
using
distance() instead of the repeated calls to intersects() in the
while(dirty)
loop ? It might look simpler now that you've been away from the code
for a
while.
The Skyline::distance()s have all the information you want. If you
are trying
to place, by moving UP, an object with skylines 'pair' among a set of
skylines
'forest[j]' then the distance you need to move has several constraints
floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
ceilings[j] = forest[j][DOWN]~distance~pair[UP]
You know the solution will be to move an amount equal to one of the
floors[j],
and you want to find the smallest that fits, so make
trials = sort (floors)
and find the smallest among trials that satisfies
trials[n] 0
for all j (trials[n] floors[j]
|| trials[n] ceilings[j] )
Somebody might complain that this is quadratic in the number of
objects to
place, but the problem placing N objects while looking for holes left
in earlier
placements is a quadratic task. But now the inner loop is simple
comparisons,
rather than re-checking of skyline intersections and distances with
the
while(dirty) method.
In any case, the number N seems to be sufficiently limited in practice
(thanks
maybe to 'exempt') based on reported score compilation times.
Hey Keith,
I am not adverse to using distance if possible - my trouble case is the
following.
A
B
Image that object A above is placed first and LilyPond is now
considering whether it should leave B where it is or place it above A.
The distance function would dictate that it needs to be shifted above A
because its DOWN skyline is below A's UP. However, it never intersects,
so there's no need to shift it. This is why I'm looking for
intersections. Maybe I'm thinking of it incorrectly, though?
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode945
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/17 08:12:56, Keith wrote:
If I remove all additions to the 'riders' array,
'tuplet-number-outside-staff-priority.ly' stays the same.
I tried, but failed, to create a case where keeping track of 'riders'
helps.
\relative c'' {
\override TupletBracket #'outside-staff-priority = #1
\times 2/3 { a8\trill a\trill a\trill } }
What case did you use ?
\version 2.17.0
#(ly:set-option 'debug-skylines #t)
\relative c'' {
\override TupletBracket #'outside-staff-priority = #1
\times 2/3 { a4\trill a\trill a\trill } }
If you comment out the rider business, the vertical-skylines will not be
correct for axis-group-interface. Comment it back in and they will.
http://codereview.appspot.com/5626052/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right): http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode778 lily/axis-group-interface.cc:778: Note we only use 75% of padding and apply the full compliment only if On 2012/08/14 04:21:33, Keith wrote: It looks like you add 50% padding here, then pass to avoid_outside_staff_collision() which removes 37.5% of padding away. Probably not what you meant to do. The verbs 'use' and 'apply' are vague enough that the comment didn't help me see what you meant to do. Sorry, I see the problem now (I missed the 0.75). I brought back the logic of the comment above it looks like it obviates the need for this whole 50%-of-padding thing. Running regtests now. http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
Thanks for the review!
http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly
File input/regression/text-script-vertical-skylines.ly (right):
http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly#newcode5
input/regression/text-script-vertical-skylines.ly:5: kerning.
On 2012/08/14 04:21:33, Keith wrote:
It is not obvious we want this for TextScripts,
{b'^Élever b'^brusquement}
so don't enshrine the requirement in a regtest.
Done.
http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly
File input/regression/volta-bracket-vertical-skylines.ly (right):
http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly#newcode4
input/regression/volta-bracket-vertical-skylines.ly:4: texidoc = Volta
brackets are vertically kerned to objects below them.
On 2012/08/14 04:21:33, Keith wrote:
Good, but fit not kerned
To me, 'to kern' includes making many aesthetic judgments to come up
with
individual adjustments for each fitting pair.
Done.
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode629
lily/axis-group-interface.cc:629: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:
I don't understand the comment, but what I needed to figure out was :
Given pre-padded vertical skylines 'pair' for an outside-staff Grob,
this
figures out the vertical position of the Grob. It raises the skylines
in 'pair'
and shifts (along the skyline) 'horizontal_skylines' by the same
amount.
Using horizontal_skylines (with the buildings pointing horizontally)
to
determine vertical position, is a new concept worth mentioning.
I'm still figuring out 'exempt', '*_forest', etc.
Better comment added.
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode658
lily/axis-group-interface.cc:658: check for horizontal edges jinnying up
On 2012/08/14 04:21:33, Keith wrote:
I learned the local dialect 200 miles north, so I don't know what this
means. Is
this the case where the edges lie side a by each, or kitty corner ?
Elaine Gould uses the phrase jinnying up in behind bars, and given
that we are using that as one of our base texts, I'm ok using the phrase
here.
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode728
lily/axis-group-interface.cc:728: We need to take the padding away now
that it has been shifted. Otherwise
On 2012/08/14 04:21:33, Keith wrote:
I can't find where the padding was ever added.
The call to grow in add_grobs_of_one_priority.
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode751
lily/axis-group-interface.cc:751: edge of the just-placed grob).
Otherwise, we skip it until the next pass.
On 2012/08/14 04:21:33, Keith wrote:
This comment describes the old way of solving the problem you saw with
'midi-dynamics.pdf' but you removed the old code. Maybe you want to
use the old
method, but make code and comment agree one way or another.
Excellent observation - I'd completely forgotten this. Perhaps this
takes care of the midi-dynamics.ly problem altogether...will
investigate.
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode778
lily/axis-group-interface.cc:778: Note we only use 75% of padding and
apply the full compliment only if
On 2012/08/14 04:21:33, Keith wrote:
It looks like you add 50% padding here, then pass to
avoid_outside_staff_collision() which removes 37.5% of padding away.
Probably
not what you meant to do.
The verbs 'use' and 'apply' are vague enough that the comment didn't
help me see
what you meant to do.
I think that both remove 50%, no?
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode843
lily/axis-group-interface.cc:843: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:
Does this part of the comment apply to the code below ?
The comment's blech...I'll redo it in a later patchset (remind me if I
don't).
http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc#newcode890
lily/axis-group-interface.cc:890: feature present in one regrest :(
On 2012/08/14 04:21:33, Keith wrote:
You tease us. Which regression test ?
Done.
http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc#newcode1368
lily/beam.cc:1368: shift += 0.01;
On 2012/08/14 04:21:33, Keith wrote:
* beamdir ?
Done.
http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc
File lily/skyline-pair.cc (right):
http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc#newcode103
lily/skyline-pair.cc:103: // other[UP] then we will not intersect.
On
Re: Set indent based on instrument name (issue 6457049)
Hey Phil! First and foremost, congrats on this work. I'm thrilled to see you venturing into the C++ side. You're tackling an issue that, while just a few lines of code, uses a lot of advanced LilyPond structures, so it's not easy. My suggestions don't have to do with the math (all of it is fine) but rather a way to restructure your work so that it fits better into how LilyPond code works. At the engraver level, it is generally not a good idea to touch layout information. When this happens, it is usually to kill grobs or set properties. Avoid measuring extents when engraving is happening because they could be dependent on other callbacks which could trigger many layout decisions before engraving is finished. Here, what I would do is use the Pointer_group_interface to add two grob arrays to the root system of instrument names - one that contains all InstrumentName grobs with instrumentName and one that contains all InstrumentName grobs with shortInstrumentName. You can stash these in two separate vectors and then loop through the vectors, invoking Pointer_group_interface::add_grob in a finalize method for the engraver. Then, create two properties of the system grob called instrument-name-length and short-instrument-name-length and two callbacks that look up the appropriate grob array and do the X-extent calculations you do in the engraver. To avoid code-duping, they can likely use an internal function for most of their work. Properties, in LilyPond, are the best way to stash information. You almost never want to create global variables or class member variables to do this. Then, in paper-score.cc, create functions Real Paper_score::get_instrument_name_length () and Real Paper_score::get_short_instrument_name_length () These function will check to see if there is one or many systems and, if so, glean the instrument name length or short instrument name length from that. Otherwise, it will return 0. Then, for line_dimensions_int (Output_def *def, int n), make two extra arguments where you pass in the results of these functions. These will replace the global variables you're creating in output-def.cc. It works because line_dimensions_int is only ever called when a Paper_score is available, so you can always interrogate the Paper_score for the appropriate indent values before making the function call. These changes will make the code more maintainable and less prone to kick up bugs in other areas. Let me know if you have any questions! Cheers, MS http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc File lily/instrument-name-engraver.cc (right): http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc#newcode116 lily/instrument-name-engraver.cc:116: SCM Text_scheme = Text_interface::interpret_markup (layout-self_scm (), Ditto for variable names - use lowercase. http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc#newcode121 lily/instrument-name-engraver.cc:121: Real Text_len = Text_int[MAX]-Text_int[MIN]; Real text_len = text_int.length (); http://codereview.appspot.com/6457049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
It's back... The only thing that doesn't the work the way I'd expect it is Skyline::padded. It seems to not be adding the padding correctly (Joe - any ideas)? This causes tighter than desired spacing in alignment-order.ly. Other than that it is all reviewable! For those who were keeping score before, files like size16.ly, size20.ly etc. were a major nuisance and are now kept at bay. Cheers, MS http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc
File lily/skyline.cc (right):
http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode693
lily/skyline.cc:693: Skyline::padded (Real horizon_padding) const
On 2012/07/01 23:04:17, Keith wrote:
The horizon_padding built in to Skyline::Skyline had nice beveled
corners; does
this result in nice beveled corners ?
It did - I want to get Joe's feedback first before moving back large
chunks of the old code, as he was the one who created a lot of this new
Skyline stuff and I want to make sure I'm understanding it correctly.
http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode699
lily/skyline.cc:699: if (last_end -infinity_f)
On 2012/07/01 23:04:17, Keith wrote:
I don't see any assignments to 'last_end' so this seems to be always
false.
True - that's one of the problems. The other is that height is always
an empty interval. I've rewritten it as:
Skyline
Skyline::padded (Real horizon_padding) const
{
vectorBox pad_boxes;
Real last_end = -infinity_f;
for (listBuilding::const_iterator i = buildings_.begin (); i !=
buildings_.end (); ++i)
{
if (last_end -infinity_f)
{
// Add the box that pads the left side of the current
building.
Real start = last_end - horizon_padding;
Real height = i-height (last_end);
if (height -infinity_f)
pad_boxes.push_back (Box (Interval (start, last_end),
Interval (min (height - 0.01,
height), max (height - 0.01, height;
}
if (i-end_ infinity_f)
{
// Add the box that pads the right side of the current
building.
Real start = i-end_;
Real end = start + horizon_padding;
Real height = i-height (start);
if (height -infinity_f)
pad_boxes.push_back (Box (Interval (start, end),
Interval (min (height - 0.01,
height), max (height - 0.01, height;
}
}
// We created pad_boxes assuming that sky_ is UP. To get padded to
use
// the UP side of the boxes, we need to create it pointing UP even if
// it doesn't really.
Skyline padded (pad_boxes, X_AXIS, UP);
padded.sky_ = sky_;
padded.merge (*this);
return padded;
}
and still no result as expected...
http://codereview.appspot.com/5626052/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issues warning for negative-spanning line spanner. (issue 6302097)
Reviewers: Keith, Message: Thanks for the review! http://codereview.appspot.com/6302097/diff/1/lily/line-spanner.cc File lily/line-spanner.cc (right): http://codereview.appspot.com/6302097/diff/1/lily/line-spanner.cc#newcode373 lily/line-spanner.cc:373: me-warning (_ (Line spanner's left point is to the right of its right point.)); On 2012/06/20 05:31:50, Keith wrote: if 'me' supports me-origin()-warning() it will tell the user which line spanner(s) ::origin is only defined for stream events, music, context defs and books. Description: Issues warning for negative-spanning line spanner. Please review this at http://codereview.appspot.com/6302097/ Affected files: M lily/line-spanner.cc Index: lily/line-spanner.cc diff --git a/lily/line-spanner.cc b/lily/line-spanner.cc index 2d43a71fa4f0006943990e89e390fcdf4c71b28b..f42628820e6e64b6a0ba019d93f0bf8666125472 100644 --- a/lily/line-spanner.cc +++ b/lily/line-spanner.cc @@ -21,6 +21,7 @@ #include axis-group-interface.hh #include font-interface.hh #include grob-interface.hh +#include international.hh #include item.hh #include lily-proto.hh #include line-interface.hh @@ -368,6 +369,8 @@ Line_spanner::print (SCM smob) arrows[LEFT], arrows[RIGHT])); } + else +me-warning (_ (Line spanner's left point is to the right of its right point.)); line.translate (Offset (-me-relative_coordinate (commonx, X_AXIS), simple_y ? 0.0 : -me-relative_coordinate (my_common_y, Y_AXIS))); ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Treat accidentals parentheses as cautionary (issue 6310065)
LGTM http://codereview.appspot.com/6310065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 1320: Scheme bar line interface (issue 6305115)
Good work - two overall things. 1) In Scheme, try to avoid `do'. Use map and reduce. 2) Have you tested performance hits on large scores? It may be worth it to leave bar-line.cc as the default and have your script as a Scheme implementation, not unlike the way we do bezier curves. Cheers, MS http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc File lily/bar-line.cc (right): http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc#newcode31 lily/bar-line.cc:31: #include set Is it worth it to keep this file? I'd just move what's left over to Scheme at that point. http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc File lily/span-bar.cc (right): http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc#newcode36 lily/span-bar.cc:36: Pointer_group_interface::add_grob (me, ly_symbol2scm (elements), b); Ditto - you can likely get rid of this as well. http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm File scm/bar-line.scm (right): http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode80 scm/bar-line.scm:80: (cons (- height half-staff) (+ height half-staff)) (interval-widen half-staff height) http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode94 scm/bar-line.scm:94: (stencil empty-stencil)) As you use let*, you can move all the set! to redefinitions of stencil in the let*. It seems slightly more in keeping w/ coding practice (could be wrong, though). http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode118 scm/bar-line.scm:118: To be more scheme-ic, try a map to get an array of stencils w/ the correct offsets and then using reduce on the list of stencils to smoosh them together via ly:stencil-add. http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode150 scm/bar-line.scm:150: stencil) Ditto - as a general rule, avoid `do'. Map and reduce. http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode214 scm/bar-line.scm:214: X RIGHT colon-stil kern)) Can you turn the above into the result of a call to a recursive function that uses ly:stencil-combine-at-edge? http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode495 scm/bar-line.scm:495: ;; bulky side. Rewritten by Han-Wen. Ported from c++ to Scheme by Marc Hohl. Can you get this down to the 80 character line width? http://codereview.appspot.com/6305115/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/12 13:22:10, dak wrote: On 2012/06/12 12:54:40, MikeSol wrote: On 2012/06/12 12:49:45, dak wrote: http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc File lily/grob.cc (right): http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472 lily/grob.cc:472: real_ext[d] += offset; On 2012/06/12 12:32:37, dak wrote: I don't understand this. The only way to get a nan from adding an offset to infinity is by adding another nan or an infinite offset with different sign. What case is this supposed to catch? So what you actually meant to say was if (!real_ext.is_empty ()) real_ext.translate (offset); If that's what you mean, why don't you write it instead of some puzzle? This is not what I mean. An empty interval is, in LilyPond, an interval whose left is greater than it's right. I disagree. An empty interval is an interval not containing any point. The details are not important. So (3 . 2) is empty, but (3 . 2) can be translated by an infinite value in either direction and not result in nan. What meaning is there in translating an empty interval and afterwards getting an interval no longer being empty since its lower bound no longer is smaller than its upper bound? Are you, by chance, confusing an _interval_ with a tuple of points? You can _implement_ a closed interval as a tuple of points, but that does not lend meaning to shifting an empty interval. So just checking for emptiness isn't enough. For mimicking all side effects of the current implementation, it isn't. But if some code relies on those side effects, it is broken in my opinion. If you want to manipulate a two-dimensional vector without inherent relation between its two elements, use a Drul, not an Interval. And frankly, shouldn't we rather put this into flower/include/interval.hh instead of trying to catch this in arbitrary places in our code? I had toyed with this idea - this is a bit out of my league, as I don't know if LilyPond will take a performance hit if we add extra operations to something as elemental as translate or is_empty. Every piece of inherent safety takes a performance hit. Personally I think that the only case where we have is_empty true is for (+inf, -inf). And I don't mean that we should change the test: the test is fine. There just isn't a combination of interval bounds that makes more sense. We could equally well define the empty interval as (0, -1) and get consistent behavior from that. But while the interval is empty, its bounds should not be interpreted as conveying meaning. If all empty intervals are the same then, by design, we need to make sure that all of them equal (+inf.0 . -inf.0). This means that: --) Interval should have a method set_to_empty () that sets it to (+inf.0 . -inf.0) (maybe it already does). --) Anytime an interval is set to its left being greater than its right such that it is not infinitely empty, a programming error should be issued. --) Perhaps all math done on empty intervals should raise programming errors. I'm all for making the code more coherent. It sounds like we're talking about a much deeper problem than this patch, and perhaps it's wiser to come up w/ a solution to that first before pushing this. Cheers, MS http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/11 07:10:48, dak wrote: On 2012/06/11 05:34:23, MikeSol wrote: This shows a case where stem height cannot be determined by stem stencil presence. The first version of the patch works under the assumption that information about height cannot be gleaned from the stencil and must be made explicit through overrides. I think that, even though this requires redundancy (specifying no stencil and an empty height), if LilyPond treats cases where there is no stencil and a non-empty height (like beam-stem-stemlet.ly), then the method in the first patch set is better. What do you mean with treats cases? That it implements some useful behavior based on the exact given height? Or merely that it catches the case? Personally, I consider it an accident waiting to happen if downing the stencil is not enough to suppress its consideration. _Iff_ there is a situation where it is really required to have a non-zero height, then setting the stencil to ##f is the wrong measure: it should be a point-stencil instead. So when stencil is #f for stems, the height should always be empty? In this case, patch set 2 was the correct approach, and the fact that it failed regtests reveals a hidden bug in the handling of beamed rests under tuplets. If you think this is the best way to tackle it, then I'll do that. It'll likely mean a larger patch as it'll unearth some bugs that didn't surface with the stem work in 2.15. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sets TabVoice Stem height to ##f (issue 6303065)
On 2012/06/11 07:48:34, dak wrote: On 2012/06/11 07:24:33, MikeSol wrote: On 2012/06/11 07:10:48, dak wrote: Personally, I consider it an accident waiting to happen if downing the stencil is not enough to suppress its consideration. _Iff_ there is a situation where it is really required to have a non-zero height, then setting the stencil to ##f is the wrong measure: it should be a point-stencil instead. So when stencil is #f for stems, the height should always be empty? In this case, patch set 2 was the correct approach, and the fact that it failed regtests reveals a hidden bug in the handling of beamed rests under tuplets. Not necessarily a hidden bug, but rather a hidden expectation. If LilyPond will, when setting the stencil to #f for some reason of its own, accompany this with consistent height data, then the behavior might be consistent and not exhibit problems. Passing information with that sort of secret contract, however, is a bad idea when users and/or other programmers are expected to be able to unstencil grobs. In that case, passing information in the height (or otherwise manipulating it) without taking the nonexistence of the stencil into consideration is a rather bad idea. If you think this is the best way to tackle it, then I'll do that. It'll likely mean a larger patch as it'll unearth some bugs that didn't surface with the stem work in 2.15. As I said: these may not necessarily bugs but rather larvae: things that are rather certain to grow into bugs under reasonably natural conditions. I think it would be worth the trouble to get rid of them. In case we have code that relies on passing information through the Y-height of a stencil even when the stencil has been set to #f by the user, this will require finding a different solution. My compromise solution in the most recent patch set is to use Keith's idea but to leave a comment so that people know that beam, stencil-less stems are treated as having a height of 0 at the point of the beam whereas unbeamed stems are heightless. The former is the case because otherwise the beam doesn't know where to go - it seems like a reasonable exception. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Footnotes correctly printed on grobs at first timestep. (issue 6306064)
Reviewers: ,
Message:
This fixes Issue 2574 but also deals with the footnote-break-visibility
regtest, which currently does not register the property change (this may
have something to do with the footnote engraver being on the Score
level).
Cheers,
MS
Description:
Footnotes correctly printed on grobs at first timestep.
Please review this at http://codereview.appspot.com/6306064/
Affected files:
M input/regression/footnote-break-visibility.ly
M lily/system.cc
Index: input/regression/footnote-break-visibility.ly
diff --git a/input/regression/footnote-break-visibility.ly
b/input/regression/footnote-break-visibility.ly
index
0f88e2df276599a66ff346c58cdd521e31e1dec6..4626553982a90a4808ac1b7807ae09fb0c9525e3
100644
--- a/input/regression/footnote-break-visibility.ly
+++ b/input/regression/footnote-break-visibility.ly
@@ -18,7 +18,7 @@ This behavior can be overridden.
\time 3/4
\break \pageBreak
c2.
-\once \override Staff . FootnoteItem #'break-visibility = ##(#f #f #t)
+\once \override Score . FootnoteItem #'break-visibility = ##(#f #f #t)
\footnote foo #'(0 . 2) #'TimeSignature bar \default
\time 4/4
\break \pageBreak
Index: lily/system.cc
diff --git a/lily/system.cc b/lily/system.cc
index
fdcc2b133d1b056f78482087218aac7abbc559ff..5d4cae8a57b8f86333bc49da7e267d2f7e8a81b6
100644
--- a/lily/system.cc
+++ b/lily/system.cc
@@ -254,6 +254,8 @@ System::get_footnote_grobs_in_range (vsize start, vsize
end)
if (Item *item = dynamic_castItem *(at_bat))
{
+ end_of_line_visible = item-break_status_dir () == LEFT;
+
if (!Item::break_visible (item))
continue;
// safeguard to bring down the column rank so that end of line
footnotes show up on the correct line
@@ -275,6 +277,8 @@ System::get_footnote_grobs_in_range (vsize start, vsize
end)
continue;
if (!at_bat-is_live ())
continue;
+ if (find (out.begin (), out.end (), at_bat) != out.end ())
+continue;
out.push_back (at_bat);
}
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Footnotes correctly printed on grobs at first timestep. (issue 6306064)
On 2012/06/10 16:09:03, dak wrote: On 2012/06/10 15:58:25, MikeSol wrote: This fixes Issue 2574 but also deals with the footnote-break-visibility regtest, which currently does not register the property change (this may have something to do with the footnote engraver being on the Score level). I'll readily admit that the footnote engraver being at Score level may be a source for new problems. However, automatic footnotes get a number for each time they hit an engraver, and footnotes may occur at pretty much any level. So I don't really see a way around having the engraver registered at Score level by default. Moving it to lower levels may be a user-level option, but it comes at the cost of some elements no longer being available for footnoting. So it would be good if we could get the Score-level footnote engraver working well. It's worth mentioning in the change log and perhaps a convert-ly NOT_SMART rule. A once-over of the footnote regtests wouldn't hurt either if you have a bit of time just to make sure they're doing what they claim to be doing. http://codereview.appspot.com/6306064/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Sets TabVoice Stem height to ##f (issue 6303065)
Reviewers: ,
Message:
Another way of going about this would be changing the Stem::height
function so that it returned an empty interval for stencil-less stems.
Then the overrides wouldn't be necessary. It's a design question more
than anything else.
Description:
Sets TabVoice Stem height to ##f
Please review this at http://codereview.appspot.com/6303065/
Affected files:
M lily/grob.cc
M ly/engraver-init.ly
M ly/property-init.ly
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index
cc39c979f0e9cf146b40975b01f82ed9441d08bc..828ae7f07b40687457f8031475f0a42d9992dc67
100644
--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -466,7 +466,10 @@ Grob::extent (Grob *refp, Axis a) const
((Grob *)this)-dim_cache_[a].extent_ = new Interval (real_ext);
}
- real_ext.translate (offset);
+ // We never want nan, so we avoid shifting infinite values.
+ for (LEFT_and_RIGHT (d))
+if (!isinf (real_ext[d]))
+ real_ext[d] += offset;
return real_ext;
}
Index: ly/engraver-init.ly
diff --git a/ly/engraver-init.ly b/ly/engraver-init.ly
index
9bb731883cba5d9754fdc7c2030508b49bde06f4..2c28af1f23f0472b34887bed7e3cbf3fa577d330
100644
--- a/ly/engraver-init.ly
+++ b/ly/engraver-init.ly
@@ -825,6 +825,9 @@ context.
%% after all, the stubs of the stems may still be visible, so ...
\override Stem #'stencil = ##f
\override Flag #'stencil = ##f
+ %% and they certainly don't have heights...
+ \override Stem #'Y-extent = ##f
+ \override Flag #'Y-extent = ##f
%% automatic beams should be suppressed for similar reasons ...
autoBeaming = ##f
%% remove beams, dots and rests ...
Index: ly/property-init.ly
diff --git a/ly/property-init.ly b/ly/property-init.ly
index
c0b352254ad0f195cb20d99465180ca5c706440a..edd117518a458c76460a277b48982c24cd048f2d
100644
--- a/ly/property-init.ly
+++ b/ly/property-init.ly
@@ -451,6 +451,8 @@ tabFullNotation = {
\revert TabVoice.Stem #'details
\revert TabVoice.Stem #'stencil
\revert TabVoice.Flag #'stencil
+ \revert TabVoice.Stem #'Y-extent
+ \revert TabVoice.Flag #'Y-extent
\override TabVoice.Stem #'stencil =
#tabvoice::draw-double-stem-for-half-notes
\override TabVoice.Stem #'X-extent =
#tabvoice::make-double-stem-width-for-half-notes
\set TabVoice.autoBeaming = ##t
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Returns an empty interval for stencilless Stem height (issue 6303065)
On 2012/06/11 03:44:32, Keith wrote: http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/6303065/diff/4001/lily/stem.cc#newcode703 lily/stem.cc:703: if (calc_beam !unsmob_stencil (me-get_property (stencil))) beam-stem-stemlet.ly shows that LilyPond produces no stencil for the invisible stems on beamed rests, yet she depends on stem extents for tuplet brackets. Maybe if (calc_beam !beam !stenxil) ? This shows a case where stem height cannot be determined by stem stencil presence. The first version of the patch works under the assumption that information about height cannot be gleaned from the stencil and must be made explicit through overrides. I think that, even though this requires redundancy (specifying no stencil and an empty height), if LilyPond treats cases where there is no stencil and a non-empty height (like beam-stem-stemlet.ly), then the method in the first patch set is better. http://codereview.appspot.com/6303065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows lyrics to slide under TimeSignature when OctaveEight present. (issue 6201068)
On 2012/05/11 08:11:18, dak wrote: m...@apollinemike.com mailto:m...@apollinemike.com writes: On 11 mai 2012, at 09:09, mailto:d...@gnu.org wrote: http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56 lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob *g2) On 2012/05/11 05:23:27, MikeSol wrote: On 2012/05/10 21:40:48, janek wrote: That's problably the most stupid question ever, but why this doesn't it begin with Pure_from_neighbor_engraver:: ? I don't see it being special. It doesn't need to be a class method - it'll never be called outside of this file. It is just a small helper function. Then it should be declared static, right? Why would it be part of the class? It's just a small helper function - architecturally, I don't see it as being part of a class. There are small helper functions like this all over the code base. I did not say it should be declared a class function. I said that it should be declared static since it is not referenced outside of the source file. -- David Kastrup Hey David, I don't completely understand the hows and whys of it, and there are many functions in the source code that are only used in one file but are not declared static. If all of these should be declared static, I'd recommend opening a tracker issue describing the general problem. Cheers, MS http://codereview.appspot.com/6201068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows lyrics to slide under TimeSignature when OctaveEight present. (issue 6201068)
Reviewers: janek, carl.d.sorensen_gmail.com,
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc
File lily/pure-from-neighbor-engraver.cc (right):
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode56
lily/pure-from-neighbor-engraver.cc:56: in_same_column (Grob *g1, Grob
*g2)
On 2012/05/10 21:40:48, janek wrote:
That's problably the most stupid question ever, but why this doesn't
it begin
with Pure_from_neighbor_engraver:: ? I don't see it being special.
It doesn't need to be a class method - it'll never be called outside of
this file. It is just a small helper function.
http://codereview.appspot.com/6201068/diff/1/lily/pure-from-neighbor-engraver.cc#newcode119
lily/pure-from-neighbor-engraver.cc:119:
Pointer_group_interface::add_grob
(need_pure_heights_from_neighbors[pos[j]][k], ly_symbol2scm
(neighbors), pure_relevants_[i]);
On 2012/05/10 21:40:48, janek wrote:
could you reduce line width, please? This one is way over regular 80
chars
limit.
Done.
Description:
Allows lyrics to slide under TimeSignature when OctaveEight present.
Please review this at http://codereview.appspot.com/6201068/
Affected files:
A input/regression/lyric-octave-eight.ly
M lily/pure-from-neighbor-engraver.cc
Index: input/regression/lyric-octave-eight.ly
diff --git a/input/regression/lyric-octave-eight.ly
b/input/regression/lyric-octave-eight.ly
new file mode 100644
index
..d3ed333bf54f4a7d5a3aa512cc4d5d090d75bdff
--- /dev/null
+++ b/input/regression/lyric-octave-eight.ly
@@ -0,0 +1,16 @@
+\version 2.15.37
+
+\header {
+ texidoc = Lyrics should still slide under @code{TimeSignature} when an
+@code{OctaveEight} is present.
+
+}
+
+\new Staff {
+ \clef treble_8
+ b
+}
+\addlyrics {
+ \set stanza = 1.
+ aaa
+}
Index: lily/pure-from-neighbor-engraver.cc
diff --git a/lily/pure-from-neighbor-engraver.cc
b/lily/pure-from-neighbor-engraver.cc
index
a6e7b5f32fe4539867f3ddbabe136357b8ba9335..413bfe5215d2b5c29e8b08e7799981071aa32bab
100644
--- a/lily/pure-from-neighbor-engraver.cc
+++ b/lily/pure-from-neighbor-engraver.cc
@@ -52,6 +52,17 @@ Pure_from_neighbor_engraver::acknowledge_item (Grob_info
i)
pure_relevants_.push_back (i.item ());
}
+bool
+in_same_column (Grob *g1, Grob *g2)
+{
+ return (g1-spanned_rank_interval ()[LEFT]
+ == g2-spanned_rank_interval ()[LEFT])
+ (g1-spanned_rank_interval ()[RIGHT]
+ == g2-spanned_rank_interval ()[RIGHT])
+ (g1-spanned_rank_interval ()[LEFT]
+ == g1-spanned_rank_interval ()[RIGHT]);
+}
+
void
Pure_from_neighbor_engraver::acknowledge_pure_from_neighbor (Grob_info i)
{
@@ -80,8 +91,10 @@ Pure_from_neighbor_engraver::finalize ()
temp.push_back (need_pure_heights_from_neighbors_[l]);
for (;
(l need_pure_heights_from_neighbors_.size () - 1
-
(need_pure_heights_from_neighbors_[l]-spanned_rank_interval ()[LEFT]
-== need_pure_heights_from_neighbors_[l +
1]-spanned_rank_interval ()[LEFT]));
+ ((need_pure_heights_from_neighbors_[l]
+ -spanned_rank_interval ()[LEFT])
+== (need_pure_heights_from_neighbors_[l + 1]
+-spanned_rank_interval ()[LEFT])));
l++)
temp.push_back (need_pure_heights_from_neighbors_[l + 1]);
need_pure_heights_from_neighbors.push_back (temp);
@@ -99,15 +112,24 @@ Pure_from_neighbor_engraver::finalize ()
{
while (pos[1] (int) need_pure_heights_from_neighbors.size ()
(pure_relevants_[i]-spanned_rank_interval ()[LEFT]
-
need_pure_heights_from_neighbors[pos[1]][0]-spanned_rank_interval
()[LEFT]))
+ (need_pure_heights_from_neighbors[pos[1]][0]
+-spanned_rank_interval ()[LEFT])))
{
pos[0] = pos[1];
pos[1]++;
}
for (int j = 0; j 2; j++)
-if (pos[j] = 0 pos[j] (int)
need_pure_heights_from_neighbors.size ())
- for (vsize k = 0; k
need_pure_heights_from_neighbors[pos[j]].size (); k++)
-Pointer_group_interface::add_grob
(need_pure_heights_from_neighbors[pos[j]][k], ly_symbol2scm (neighbors),
pure_relevants_[i]);
+if (pos[j] = 0 pos[j]
+ (int) need_pure_heights_from_neighbors.size ())
+ for (vsize k = 0;
+ k need_pure_heights_from_neighbors[pos[j]].size ();
+ k++)
+if
(!in_same_column(need_pure_heights_from_neighbors[pos[j]][k],
+pure_relevants_[i]))
+ Pointer_group_interface::add_grob
+(need_pure_heights_from_neighbors[pos[j]][k],
+ ly_symbol2scm (neighbors),
+ pure_relevants_[i]);
}
need_pure_heights_from_neighbors_.clear ();
___
lilypond-devel mailing
30 day webathon for kickstarter support (issue 6068045)
Reviewers: ,
Message:
Hey all,
My ensemble is launching a Kickstarter project in a day or two to
support our tour in France and Ireland.
We have a sweet plug in the project video for GNU LilyPond and I was
wondering if I could strike up a partnership with LilyPond to put a link
to the project on the LilyPond front page for the duration of the
Kickstarter fundraising drive (30 days). In return, I'd be glad to fix
a couple bounty items (my lack of development time in recent months has
come from the fact that I've been spending all my time on composing and
fundraising).
This patch is more or less what I'd need. I can change the layout based
on whatever people think is best. Currently, the spot for the ensemble
is towards the top of the page and the downloads are horizontally
aligned with the news.
This partnership would be a huge boost for my ensemble and I don't think
it'd divert any € that'd otherwise be going to LilyPond. I'll certainly
do my best to make sure that the tour promotes the software.
Cheers,
MS
Description:
30 day webathon for kickstarter support
Please review this at http://codereview.appspot.com/6068045/
Affected files:
M Documentation/css/lilypond-website.css
M Documentation/web.texi
Index: Documentation/css/lilypond-website.css
diff --git a/Documentation/css/lilypond-website.css
b/Documentation/css/lilypond-website.css
index
e5bf19280dd47d3533d0566051b95ebfe9b11206..806efe58687ea948f857680badce0be58e2760bb
100644
--- a/Documentation/css/lilypond-website.css
+++ b/Documentation/css/lilypond-website.css
@@ -473,7 +473,16 @@ div.news-item {
div#latestVersion {
position: absolute;
- top: 12.4em;
+ top: 16em;
+ right: 0;
+ width: 12em;
+ text-align: center;
+ border-left: 1px solid #5b7f64;
+}
+
+div#ensembleCinq {
+ position: absolute;
+ top: 0.0em;
right: 0;
width: 12em;
text-align: center;
@@ -488,6 +497,14 @@ div#latestVersion {
margin: 0;
}
+#ensembleCinq .subheading {
+ background: #5b7f64;
+ color: #fff;
+ text-align: center;
+ padding: 0 0.5em;
+ margin: 0;
+}
+
/* this might not work in certain browsers */
a[name=Stable] + h4 {
background: #bdee9d url(../pictures/color1-bg.png) repeat-x top left;
Index: Documentation/web.texi
diff --git a/Documentation/web.texi b/Documentation/web.texi
index
b0e2b8311ad0794e729b2f3bda1a64956140364b..daa73b88d68267176ed5e59db9c2f08271f2ca02
100644
--- a/Documentation/web.texi
+++ b/Documentation/web.texi
@@ -150,6 +150,18 @@ Read more in our @ref{Introduction}!
@end ifclear
@ifset web_version
@c make the box:
+@divId{ensembleCinq}
+@subheading Ensemble 101
+
+The Ensemble 101 is going on a European tour where they'll sing
+music typeset using LilyPond. Click @uref{http://www.ensemble101.fr, here}
+to learn more and to help them out on Kickstarter!
+
+@divEnd
+@end ifset
+
+@ifset web_version
+ @c make the box:
@divId{latestVersion}
@subheading Quick links
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Triggers X-extent calculation for NoteColumn before setting stem-begin-position (issue 5934050)
http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/5934050/diff/7001/lily/stem.cc#newcode131 lily/stem.cc:131: Real stem_beg = internal_calc_stem_begin_position (me, false); On 2012/04/02 07:42:55, Keith wrote: I'm assuming something within calc_stem_begin_position (me, calc_beam=false) triggers note-collision resolution before setting the stem-begin position. Yup. It allows calc_stem_position to skip all of the beam business and get to the setting of note positions. http://codereview.appspot.com/5934050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Creates pure conversion for Flag::calc_y_offset (issue 5934050)
Reviewers: Keith,
Message:
There is a nasty bug in LilyPond that this patch only exacerbates - as a
rule of thumb, X positioning functions should not set Y positioning
variables and vice versa. note-collision.cc, which works from an X_AXIS
callback, sets the stem attachment, which is a both X and Y axis value.
This is a recipe for trouble - there even if it means logic / code dup,
these should be separated out into two functions: one that sets the X
value and one that sets the Y value. As a result, I have to trigger the
X extent in Stem::internal_stem_begin_position to get the Y position set
correctly. It may be worth it to fix this now: it'd delay 2.16 further,
but it seems like a sound architecture decision that'd prevent this
sorta problem from happening in the future.
Description:
Creates pure conversion for Flag::calc_y_offset
Please review this at http://codereview.appspot.com/5934050/
Affected files:
A input/regression/stem-begin-position.ly
M lily/stem.cc
Index: input/regression/stem-begin-position.ly
diff --git a/input/regression/stem-begin-position.ly
b/input/regression/stem-begin-position.ly
new file mode 100644
index
..30bd3ff1e3865c53f50a3ab7e03630b62fff5e1f
--- /dev/null
+++ b/input/regression/stem-begin-position.ly
@@ -0,0 +1,10 @@
+\version 2.15.36
+
+\header {
+ texidoc = Stems reach correct begin points of merged noteheads.
+
+}
+
+ { \aikenHeads f'8 } \\ { \aikenHeads f'8 }
+ { \aikenHeads f'4:32 } \\ { \aikenHeads f' }
+ { \aikenHeads e'8 f' s4 } \\ { \aikenHeads e'8 f' s4 }
Index: lily/stem.cc
diff --git a/lily/stem.cc b/lily/stem.cc
index
f662f3cf1dec75637680e3c85f9bcd9fd31a158c..0e8c13cdaa1ff7edc67ffe930c4496fbd841b243
100644
--- a/lily/stem.cc
+++ b/lily/stem.cc
@@ -787,6 +787,14 @@ Stem::internal_calc_stem_begin_position (Grob *me,
bool calc_beam)
if (Grob *head = support_head (me))
{
Interval head_height = head-extent (head, Y_AXIS);
+ // trigger positioning of stems in note column in case of merge
+ if (me-get_parent (X_AXIS)
+ me-get_parent (X_AXIS)-get_parent (X_AXIS))
+(void) me-get_parent (X_AXIS)
+ -extent (me-get_parent (X_AXIS)
+ -get_parent (X_AXIS), X_AXIS);
+ else
+me-programming_error (Stem doesn't belong to NoteColumn or
PaperColumn.);
Real y_attach = Note_head::stem_attachment_coordinate (head, Y_AXIS);
y_attach = head_height.linear_combination (y_attach);
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Axis group interface ignores column rank for pure-from-neighbor-interface (issue 5843063)
Reviewers: Keith, mike_apollinemike.com,
Message:
Running regtests. Will post new patch once it gets a clean bill of
health.
http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neighbor-interface-pure-height.ly
File input/regression/pure-from-neighbor-interface-pure-height.ly
(right):
http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neighbor-interface-pure-height.ly#newcode1
input/regression/pure-from-neighbor-interface-pure-height.ly:1: \version
2.15.34
On 2012/03/21 05:58:42, Keith wrote:
.35
The title makes less sense now. lyrics-spanbarstub.ly ?
Done.
http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neighbor-interface-pure-height.ly#newcode4
input/regression/pure-from-neighbor-interface-pure-height.ly:4: texidoc
= @code{axis-group-interface::pure-height} does not take spanned
On 2012/03/21 05:58:42, Keith wrote:
The description no longer fits. Maybe
empty measures do not confuse SpanBarStub. These lyrics should
remain clear of
the span bars.
Done.
http://codereview.appspot.com/5843063/diff/7/lily/item.cc
File lily/item.cc (right):
http://codereview.appspot.com/5843063/diff/7/lily/item.cc#newcode245
lily/item.cc:245: cache_pure_height (Grob::pure_height (this, 0,
INT_MAX));
On 2012/03/21 05:58:42, Keith wrote:
This commit :
http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commitdiff;h=9fe7859a8592a080413b744e3768db41059dbfe3
depended on having 'start' and 'end' passed through, so we should put
this back.
Caching in this way assumes that the pure_height of an Item is
independent of
where the line-breaks are.
Anything that gets pure_height from its neighbors would break that
assumption,
but now it seems there are no such Items, as they all use
pure-from-neighbors-interface to set their extra-spacing-height.
Tied accidentals break that assumption as well, but that's not our
fault. I'll
write a cautionary comment in a separate commit.
Done.
http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm
File scm/define-grobs.scm (right):
http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm#newcode1883
scm/define-grobs.scm:1883: (Y-extent . (1 . -1))
On 2012/03/21 05:58:42, Keith wrote:
I suggest (Y-extent . #f)
because (1 . -1) looks so much like (-1 . 1), and it also makes
suspicious
people like me think it is an uncommented magic number.
Done.
http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm
File scm/output-lib.scm (right):
http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm#newcode437
scm/output-lib.scm:437: (let* ((height (ly:grob-pure-height grob grob 0
1000))
On 2012/03/21 05:58:42, Keith wrote:
The C code uses INT_MAX to represent the end of the whole score, so it
would be
nice to use the same here, if possible.
There'd have to be a constant defined for INT_MAX in guile. I believe
that INT_MAX can change between computers (not sure if it's the compiler
or system that does this). Probably worth doing in a separate commit
where INT-MAX is defined in the same place as all of the PI constants
and then plugged anywhere where there's a large integer.
Description:
Axis group interface ignores column rank for
pure-from-neighbor-interface
Please review this at http://codereview.appspot.com/5843063/
Affected files:
A input/regression/pure-from-neighbor-interface-pure-height.ly
M lily/item.cc
M lily/pure-from-neighbor-engraver.cc
M scm/define-grobs.scm
M scm/output-lib.scm
Index: input/regression/pure-from-neighbor-interface-pure-height.ly
diff --git a/input/regression/pure-from-neighbor-interface-pure-height.ly
b/input/regression/pure-from-neighbor-interface-pure-height.ly
new file mode 100644
index
..3e7515dfea1cc991fef736e76c9566ffca728c64
--- /dev/null
+++ b/input/regression/pure-from-neighbor-interface-pure-height.ly
@@ -0,0 +1,18 @@
+\version 2.15.34
+
+\header {
+ texidoc = @code{axis-group-interface::pure-height} does not take spanned
+rank interval into account for @code{pure-from-neighbor-interface} grobs.
+
+}
+
+\new StaffGroup
+ \new Staff { \repeat unfold 8 { R1 e'1 } }
+ \addlyrics {
+Worked twice...
+and then
+I continued...
+working... correctly.
+ }
+ \new Staff { R1*16 }
+
Index: lily/item.cc
diff --git a/lily/item.cc b/lily/item.cc
index
6ea5643a5445e7d791a119479e73d54a5fe7aead..19c9926c3b755ae660e3a698da441498d755d5e1
100644
--- a/lily/item.cc
+++ b/lily/item.cc
@@ -242,7 +242,7 @@ Item::pure_height (Grob *g, int start, int end)
if (cached_pure_height_valid_)
return cached_pure_height_ + pure_relative_y_coordinate (g, start,
end);
- cache_pure_height (Grob::pure_height (this, start, end));
+ cache_pure_height (Grob::pure_height (this, 0, INT_MAX));
return cached_pure_height_ + pure_relative_y_coordinate (g, start, end);
}
Index: lily/pure-from-neighbor-engraver.cc
diff --git a/lily/pure-from-neighbor-engraver.cc
Autobeaming works properly with tuplet recheck (Issue 2408) (issue 5844052)
http://codereview.appspot.com/5844052/diff/3001/lily/beaming-pattern.cc File lily/beaming-pattern.cc (right): http://codereview.appspot.com/5844052/diff/3001/lily/beaming-pattern.cc#newcode361 lily/beaming-pattern.cc:361: } My comment has nothing to do with your patch but with this function - it seems like there's a memory leak with *dur (I know very little about memory management, so sorry in advance if I'm totally off...). http://codereview.appspot.com/5844052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Turns off TupletBracket collision avoidance if Script avoids slur (issue 5821051)
Reviewers: , Message: Meh...not my best work, but it is a first step towards fixing this problem. A full solution would do tuplet avoidance for scripts in the same way they're done for slurs. Takers? Description: Turns off TupletBracket collision avoidance if Script avoids slur Please review this at http://codereview.appspot.com/5821051/ Affected files: M lily/tuplet-bracket.cc Index: lily/tuplet-bracket.cc diff --git a/lily/tuplet-bracket.cc b/lily/tuplet-bracket.cc index 40e0c728c5ef2c353de391bd5da025386bce7007..8e4a3c017ce4ee6f0a3b2089810396063f8aa55b 100644 --- a/lily/tuplet-bracket.cc +++ b/lily/tuplet-bracket.cc @@ -669,6 +669,13 @@ Tuplet_bracket::calc_position_and_height (Grob *me_grob, Real *offset, Real *dy) if (scm_is_number (scripts[i]-get_property (outside-staff-priority))) continue; + // assume that if a script is avoiding slurs, it should not get placed + // under a tuplet bracket + SCM avoid = scripts[i]-get_property (avoid-slur); + if (avoid == ly_symbol2scm (outside) + || avoid == ly_symbol2scm (around)) +continue; + Interval script_x (scripts[i]-extent (commonx, X_AXIS)); Interval script_y (scripts[i]-extent (commony, Y_AXIS)); ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Uses derived_mark to avoid segfault (issue 5729051)
http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode35 lily/span-bar-stub-engraver.cc:35: these four contexts. On 2012/03/04 21:35:57, Keith wrote: You said later that only two SpanBarStubs are created, so creation of SpanBarStubs will be contemplated in each of these four contexts. Fixed. http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode160 lily/span-bar-stub-engraver.cc:160: // removes all unused contexts On 2012/03/06 12:19:04, dak wrote: On 2012/03/04 10:47:17, Neil Puttock wrote: Is it feasible to listen for RemoveContext instead? At least then you only need to rebuild axis_groups_ when contexts die. I'd second this suggestion. It should give more reliable lifetimes, and also cause a better correlation of parsed object should be dead messages with realities. Unfortunately this is not possible. I tried, but it looks like listeners in the way you're talking about are not possible in engravers. http://codereview.appspot.com/5729051/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058)
http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc File lily/page-breaking.cc (right): http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc#newcode584 lily/page-breaking.cc:584: } Just a C++ question - do these lines implicitly call the copy constructor to create footnote_stencil so that footnote_stencil is a copy of foot? I'm not familiar with this syntax for copying, so I just wanted to be sure that this is what these lines were doing. http://codereview.appspot.com/5755058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Uses derived_mark to avoid segfault (issue 5729051)
http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode71 lily/span-bar-stub-engraver.cc:71: axis_groups_ = scm_cons (scm_cons (i.grob ()-self_scm (), i.context ()-self_scm ()), axis_groups_); On 2012/03/04 07:26:30, dak wrote: axis_groups_ never gets cleared out again: this keeps grobs and contexts alive at least until the engraver is collected. This is the point. I was wrong to suggest before that it be cleared out. Unless the score is insane, a context like PianoStaff will only ever house 10ish contexts and 10 vertical axis groups max. So the size will stay tiny. I could clear this out, but the problem is that LilyPond doesn't provide a mechanism to signal when VerticalAxisGroups are no longer being used. If announce_end_spanner were called on them, I could acknowledge that in the engraver. A separate tracker issue could be opened for that. http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode81 lily/span-bar-stub-engraver.cc:81: if (!vertical_alignment) // we are at the beginning of a score, so no need for stubs On 2012/03/04 07:26:30, dak wrote: If we are at the beginning of the score is a valid state here, scm_caar (axis_groups_) is likely to throw an exception. This is not possible because process_acknowledged is called after all other process calls, so the list will be populated. However, I agree that an extra check here couldn't hurt. http://codereview.appspot.com/5729051/diff/1001/lily/span-bar-stub-engraver.cc#newcode127 lily/span-bar-stub-engraver.cc:127: for (vsize j = 0; j affected_contexts.size (); j++) On 2012/03/04 07:26:30, dak wrote: While you are at it: how about throwing out all of the reverses and instead letting the loop run backwards? This is vestigial code from when I was doing sorting on the span bar vector. It is no longer necessary, so removed. http://codereview.appspot.com/5729051/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Uses derived_mark to avoid segfault (issue 5729051)
On 2012/03/04 20:59:22, Keith wrote: http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode46 lily/span-bar-stub-engraver.cc:46: be engraved in contexts where BarLines are engraved. So, in the example with the piano staff earlier in the comment, Four SpanBarStubs are /created/ at each barline, Two are /engraved/ (those in the Lyrics and Dynamics context which have the Pure_from_neightbor_engraver, but not those in the Staffs which have the Bar_line_engraver) and Zero are printed. Right? Close. Two are created, two are engraved, and 0 are printed. The Pure_from_neighbor_engraver would catch them in Staves too, but they are not sent to staves or anything else that has BarLines, as BarLines fulfill the role of SpanBarStubs in contexts that have them. The line that makes sure that contexts with barlines do not get SpanBarStubs is: find (bar_axis_indices.begin (), bar_axis_indices.end (), j) == bar_axis_indices.end () Or, in other words, only create the grob (and thus do the engraving) if the context's index is not in the list of indices of contexts that have bar lines (== bar_axis_indices.end () is another way of saying that it is not in the vector, so find returns a pointer to the vector's end). http://codereview.appspot.com/5729051/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Uses derived_mark to avoid segfault (issue 5729051)
Reviewers: ,
Message:
Hey all,
I don't have time to test this patch tonight (David sent me to bed) but
it should do the trick. The map was overkill - a simple list of pairs
does the trick w/o anymore overhead and allows the use of derived_mark.
Cheers,
MS
Description:
Uses derived_mark to avoid segfault
Please review this at http://codereview.appspot.com/5729051/
Affected files:
M lily/span-bar-stub-engraver.cc
Index: lily/span-bar-stub-engraver.cc
diff --git a/lily/span-bar-stub-engraver.cc b/lily/span-bar-stub-engraver.cc
index
7b26ddbd1e68ed1d0892471d3f465650661e1bdf..35a47207fa5f353519889393ca1a8e948883ac2f
100644
--- a/lily/span-bar-stub-engraver.cc
+++ b/lily/span-bar-stub-engraver.cc
@@ -17,7 +17,6 @@
along with LilyPond. If not, see http://www.gnu.org/licenses/.
*/
-#include map
#include algorithm
#include align-interface.hh
@@ -38,7 +37,7 @@
class Span_bar_stub_engraver : public Engraver
{
vectorGrob * spanbars_;
- mapGrob *, Context * axis_groups_;
+ SCM axis_groups_;
public:
TRANSLATOR_DECLARATIONS (Span_bar_stub_engraver);
@@ -46,10 +45,18 @@ protected:
DECLARE_ACKNOWLEDGER (span_bar);
DECLARE_ACKNOWLEDGER (hara_kiri_group_spanner);
void process_acknowledged ();
+ virtual void derived_mark () const;
};
Span_bar_stub_engraver::Span_bar_stub_engraver ()
{
+ axis_groups_ = SCM_EOL;
+}
+
+void
+Span_bar_stub_engraver::derived_mark () const
+{
+ scm_gc_mark (axis_groups_);
}
void
@@ -61,7 +68,7 @@ Span_bar_stub_engraver::acknowledge_span_bar (Grob_info i)
void
Span_bar_stub_engraver::acknowledge_hara_kiri_group_spanner (Grob_info i)
{
- axis_groups_[i.grob ()] = i.context ();
+ axis_groups_ = scm_cons (scm_cons (i.grob ()-self_scm (), i.context
()-self_scm ()), axis_groups_);
}
void
@@ -70,12 +77,10 @@ Span_bar_stub_engraver::process_acknowledged ()
if (!spanbars_.size ())
return;
- Grob *vertical_alignment = Grob::get_root_vertical_alignment
((*axis_groups_.begin ()).first);
+ Grob *vertical_alignment = Grob::get_root_vertical_alignment
(unsmob_grob (scm_caar (axis_groups_)));
if (!vertical_alignment) // we are at the beginning of a score, so no
need for stubs
return;
- extract_grob_set (vertical_alignment, elements, elts);
-
for (vsize i = 0; i spanbars_.size (); i++)
{
extract_grob_set (spanbars_[i], elements, bars);
@@ -89,8 +94,14 @@ Span_bar_stub_engraver::process_acknowledged ()
vectorContext * affected_contexts;
vectorGrob * y_parents;
vectorbool keep_extent;
- for (vsize j = 0; j elts.size (); j++)
+ for (SCM s = axis_groups_; scm_is_pair (s); s = scm_cdr (s))
{
+ Context *c = unsmob_context (scm_cdar (s));
+ Grob *g = unsmob_grob (scm_caar (s));
+ if (!c || !g)
+continue;
+
+ vsize j = Grob::get_vertical_axis_group_index (g);
if (j bar_axis_indices[0]
j bar_axis_indices.back ()
find (bar_axis_indices.begin (), bar_axis_indices.end (),
j) == bar_axis_indices.end ())
@@ -101,12 +112,11 @@ Span_bar_stub_engraver::process_acknowledged ()
break;
k--;
- keep_extent.push_back (to_boolean (bars[k]-get_property
(allow-span-bar)));
- Context *c = axis_groups_[elts[j]];
if (c c-get_parent_context ())
{
- y_parents.push_back (elts[j]);
+ keep_extent.push_back (to_boolean (bars[k]-get_property
(allow-span-bar)));
+ y_parents.push_back (g);
affected_contexts.push_back (c);
}
}
@@ -122,7 +132,7 @@ Span_bar_stub_engraver::process_acknowledged ()
gi.rerouting_daddy_context_ = affected_contexts[j];
announce_grob (gi);
if (!keep_extent[j])
-it-suicide ();//it-set_property (Y-extent, ly_interval2scm
(Interval (infinity_f, -infinity_f)));
+it-suicide ();
}
}
spanbars_.clear ();
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc File lily/skyline.cc (right): http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode393 lily/skyline.cc:393: Skyline::Skyline (Building b, Real start, Axis horizon_axis, Direction sky) On 2012/02/22 09:34:43, joeneeman wrote: This isn't quite what I had in mind (for one thing, it means that the caller has to be aware of buildings, calculating their slope, etc.) what about Skyline::Skyline (vectorpairPoint, Point const segments, Axis, Direction)? it works similarly to Skyline::Skyline(vectorBox...) except that the resulting skyline shows the outline of the given set of line segments. Done. http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode647 lily/skyline.cc:647: out.merge (to_merge); On 2012/02/22 09:34:43, joeneeman wrote: merge is linear, so this loop is quadratic. It should now be n*log(n). http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc File lily/skyline.cc (right): http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc#newcode362 lily/skyline.cc:362: result.push_front (Building (last_end, -infinity_f, -infinity_f, iv[LEFT] - 2 * horizon_padding)); On 2012/02/23 11:11:47, joeneeman wrote: push_back is constant time for STL lists. No need to push_front and then reverse. I'm not not in favor of this, but why is there a reverse in the other non_overlapping_skyline function? I tried to copy it as closely as possible. http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#newcode932 lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob, simple_vertical_skylines_from_possibly_transparent_stencil, 1); On 2012/02/23 12:29:06, dak wrote: Why do we need this? I think that a _transparent_ stencil is not supposed to create a different skyline. That's the point of setting transparency rather than just #f-ing the stencil. The problem is TabStaff and TabVoice. In both contexts, many grobs are set to transparent as a default. These are grobs that are never supposed to factor into a vertical skyline. I'd be interested in seeing what happens if these were set to ##f instead of transparent, but that'd be a new project. http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
On 2012/02/23 13:35:29, dak wrote: On 2012/02/23 12:37:04, MikeSol wrote: http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#newcode932 lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob, simple_vertical_skylines_from_possibly_transparent_stencil, 1); On 2012/02/23 12:29:06, dak wrote: Why do we need this? I think that a _transparent_ stencil is not supposed to create a different skyline. That's the point of setting transparency rather than just #f-ing the stencil. The problem is TabStaff and TabVoice. In both contexts, many grobs are set to transparent as a default. These are grobs that are never supposed to factor into a vertical skyline. I'd be interested in seeing what happens if these were set to ##f instead of transparent, but that'd be a new project. a) the behavior is expected and consistent b) http://code.google.com/p/lilypond/issues/detail?id=2085 is fixed and verified Setting that stuff to transparent is merely an ugly workaround used for historical reasons. We should not create inconsistent and complex semantics to half-heartedly support it. So, just to be clear, you're saying that the TabVoice/TabStaff everything-is-transparent-when-it-should-be-##f issue is blocking this patch? If so, I'll open up an issue for that and mark this as blocked. http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right): http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc#newcode659 lily/axis-group-interface.cc:659: vectorGrob * *riders) On 2012/02/16 08:09:10, joeneeman wrote: I don't understand why riders are necessary. Is it because of this cyclic dependence stuff? Not cyclical, but imagine that a grob (say tuplet bracket, for example) has its outside staff priority set whereas one of its Y_AXIS children (say tuplet number, for example), doesn't. If the tuplet number's skyline is added to the skyline_pair using add boxes before its parent is shifted, it will be placed too low in the skyline. Thus, it must be added to the skyline only after its parent's outside-staff-priority has been set to SCM_BOOL_F. This is why riders are used. http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc File lily/skyline.cc (right): http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode467 lily/skyline.cc:467: Strips all old sloped material, adds new. On 2012/02/16 08:09:10, joeneeman wrote: You're assuming that all sloped material came from skyline padding. That's true right now, but there's no reason that it will continue to be true. It's probably better to avoid adding padding at creation time altogether, and instead to add it when calling Skyline::distance. Right you are...I agree that this is entirely dependent on the implementation of skylines at the moment. I know that I always respond to you that it's something that can be done in a future patch, but I'd put this in that category as well, as it seems like a separate problem that doesn't need to be tacked on to this patch to be solved. That said, if I forget, nag me! http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668 lily/skyline.cc:668: Skyline::left () const On 2012/02/16 08:09:10, joeneeman wrote: This is linear in the number of buildings, but it should be constant. How would one do this? http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680 lily/skyline.cc:680: Skyline::right () const On 2012/02/16 08:09:10, joeneeman wrote: Ditto Ditto http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc#newcode543 lily/stencil-integral.cc:543: SCM glyph_info = scm_hashq_ref (font_list, scm_string_to_symbol (glyph), SCM_EOL); On 2012/02/16 08:09:10, joeneeman wrote: There isn't much error-checking here. What if the user substitutes an unofficial font which isn't in the list? By the way, lilypond's fonts embed extra data in font tables (look for LILC and LILY in open-type-font.cc). That may be a better way to embed this information than by putting it in a scm file. For example, it would allow unofficial fonts to support integrals by embedding an extra table. I think this is a good idea...I'll have a look at it. Question, though - aren't font tables done as alists? I think it's important to use a hash table here, as there is a lot of data getting thrown around and hash table look ups are in constant time. I'll investigate. http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile File mf/GNUmakefile (right): http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile#newcode231 mf/GNUmakefile:231: $(LILYPOND_BINARY) --verbose $(top-src-dir)/ly/generate-font-integrals $@ On 2012/02/15 10:26:17, Graham Percival wrote: This looks good, but not immediately relevant to the rest of the vertical skylines stuff. Could you make this a separate patch so that it can be pushed sooner? Sorry for not responding to this comment inlined...the response is that it'd be hard as it requires new files and/or changes to old ones to go along w/ it. http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
Sorry for whitespace problems. After receiving several good general comments regarding the structure of this patch, I have incorporated them all into my work and am now putting it on Reitveld for more technical review. Please run: make clean ./autogen.sh --disable-optimising make all To make sure that it works. http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
Hey all, Could people please try this patch out on a couple real world scores and do some benchmarking compared to current master? It adds a lot of calculations to lilypond and a lot of them happen in Scheme, so I wanna make sure lilypond doesn't take a large processing hit. Cheers, MS http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets vertical skylines from grob stencils (issue 5626052)
Should have added before: the most recent patch set is not bug free. I'm fixing all of the regtest issues, but what I need most from other people who have a few minutes are benchmarks. Cheers, MS http://codereview.appspot.com/5626052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Gets vertical skylines from grob stencils (issue 5626052)
Reviewers: , Message: Sorry in advance for the whitespace errors. This patch provides a generic framework for building vertical skylines out of boxes by traversing through a stencil. It currently only implements skylines for three stencil types (draw-line, named-glyph, and glyph-string) and can do more for other glyphs. It is modular, so as skyline estimation improve for a given stencil, that function can be worked on alone w/o touching the other parts of stencil-integral.scm. The implementation for glyph-string is ugly and slightly incorrect for y heights of these stencils...it would be infinitely easier if the stencil could contain information about y extents, but unfortunately, PangoGlyphGeometry does not carry height information. This error does not have a bearing on the visual results in this patch, but it'd be good to find a way to make the dimensions exact. I'm also not sure that line thickness is tacked on correctly for the draw-line boxes, but it seems to yield correct results. Cheers, MS Description: Gets vertical skylines from grob stencils Please review this at http://codereview.appspot.com/5626052/ Affected files: A input/regression/tuplet-bracket-vertical-skylines.ly A input/regression/volta-bracket-vertical-skylines.ly M lily/axis-group-interface.cc A lily/box-scheme.cc M lily/include/box.hh M lily/include/skyline.hh M lily/skyline-pair.cc A lily/skyline-scheme.cc M lily/skyline.cc M lily/stencil-scheme.cc M lily/tuplet-bracket.cc M lily/volta-bracket.cc M scm/define-grobs.scm M scm/lily.scm A scm/stencil-integral.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Overhauls broken bound coordinate checking (issue 5602054)
Hey all, The newest version of this misses up some of the hairpin regtests (hairpin-barline-break.ly, for example). Still not sure why - I don't have time to check it out for a bit, but if anyone has intuition as to why, lemme know! Cheers, MS http://codereview.appspot.com/5602054/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix tuplet subdivision (issue 2243) (issue 5623051)
Haven't had a chance to test it, but the code looks good. Have you tested this out yet with nested tuplets? Cheers, MS http://codereview.appspot.com/5623051/diff/1/lily/beaming-pattern.cc File lily/beaming-pattern.cc (right): http://codereview.appspot.com/5623051/diff/1/lily/beaming-pattern.cc#newcode281 lily/beaming-pattern.cc:281: infos_[i].rhythmic_importance_ = -1; Could you include a comment indicating what the numbers mean in rhythmic importance? http://codereview.appspot.com/5623051/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Overhauls broken bound coordinate checking (issue 5602054)
Reviewers: Neil Puttock,
http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly
File input/regression/metronome-mark-broken-bound.ly (right):
http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode1
input/regression/metronome-mark-broken-bound.ly:1: \version 2.15.27
On 2012/02/01 15:04:47, Neil Puttock wrote:
2.15.28
Fixed.
http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode4
input/regression/metronome-mark-broken-bound.ly:4: texidoc = A broken
spanner must not interfere with @code{\\tempo}
On 2012/02/01 15:04:47, Neil Puttock wrote:
Surely you mean break-alignable grobs shouldn't interfere with broken
spanners?
You have this back-to-front.
I may or may not have copied and pasted this text with out reading it.
http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode12
input/regression/metronome-mark-broken-bound.ly:12: \tempo \markup {
fo } 4 = 90
On 2012/02/01 15:04:47, Neil Puttock wrote:
You might add
\override Score.MetronomeMark #'break-visibility = #all-visible
otherwise you're only checking the left-broken spanners.
Done.
http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode22
input/regression/metronome-mark-broken-bound.ly:22: \ottava #1 \times
1/1 { e'8\\startTextSpan\startTrillSpan\glissando\episemInitium
On 2012/02/01 15:04:47, Neil Puttock wrote:
If you want to test episema, you need to add the engraver (it's not
worth it imo
since you wouldn't use it outside a VaticanaVoice and it would never
be broken).
The code's a bit messy here
Fair nuf - removed.
http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode22
input/regression/metronome-mark-broken-bound.ly:22: \ottava #1 \times
1/1 { e'8\\startTextSpan\startTrillSpan\glissando\episemInitium
On 2012/02/01 15:04:47, Neil Puttock wrote:
If you want to test episema, you need to add the engraver (it's not
worth it imo
since you wouldn't use it outside a VaticanaVoice and it would never
be broken).
The code's a bit messy here
Fair nuf - omitted. What'd be the proper formatting here?
http://codereview.appspot.com/5602054/diff/2001/scm/define-grobs.scm
File scm/define-grobs.scm (right):
http://codereview.appspot.com/5602054/diff/2001/scm/define-grobs.scm#newcode1420
scm/define-grobs.scm:1420: (bound-alignment-forbidden-interfaces .
(metronome-mark-interface))
On 2012/02/01 15:04:47, Neil Puttock wrote:
I question whether you need this property. Would be better just to
exclude
break-alignable-interface in axis-group-interface.cc (so it includes
RehearsalMark and BarNumber - note that these will also interfere with
bounds if
you leave them out here)
If one could never conceive of a case where a bound would need to
include these things, then I could hard code it, but I tend to favor
using properties in case there is a scenario I can't imagine where a
person would want these to be included in NonMusicalPaperColumn bounds.
Description:
Overhauls broken bound coordinate checking
Please review this at http://codereview.appspot.com/5602054/
Affected files:
A input/regression/metronome-mark-broken-bound.ly
M lily/axis-group-interface.cc
M lily/beam.cc
M lily/lyric-hyphen.cc
M lily/ottava-bracket.cc
M lily/tuplet-bracket.cc
M scm/define-grob-properties.scm
M scm/define-grobs.scm
Index: input/regression/metronome-mark-broken-bound.ly
diff --git a/input/regression/metronome-mark-broken-bound.ly
b/input/regression/metronome-mark-broken-bound.ly
new file mode 100644
index
..5ba5064065ab8e3340aca7861f55881ac2fdbb3e
--- /dev/null
+++ b/input/regression/metronome-mark-broken-bound.ly
@@ -0,0 +1,36 @@
+\version 2.15.28
+
+\header {
+texidoc = A @code{MetronomeMark}, @code{RehearsalMark} and
@code{BarNumber}
+should not effect the starting point of spanners.
+
+}
+
+
+ \new Staff {
+ e'1 \time 4/4 \break |
+ \tempo \markup { fo } 4 = 90
+ e'1 |
+ e'1 |
+ }
+
+ \new Staff {
+ \override Score.MetronomeMark #'break-visibility = #all-visible
+ \override TupletBracket #'breakable = ##t
+ \override Beam #'breakable = ##t
+ \override Glissando #'breakable = ##t
+
+ \ottava #1 \times 1/1 { e'8\\startTextSpan\startTrillSpan\glissando
+ [ \override NoteColumn #'glissando-skip = ##t\repeat unfold 22 e'8
+ \revert NoteColumn #'glissando-skip
e'8\!\stopTextSpan\stopTrillSpan ] } |
+ }
+ \addlyrics { ah __ \repeat unfold 21 { \skip 4 } _ rrgh }
+ \addlyrics { ah -- \repeat unfold 21 { \skip 4 } _ rrgh }
+
+
+\layout {
+ \context {
+ \Voice
+ \remove Forbid_line_break_engraver
+ }
+}
Index: lily/axis-group-interface.cc
diff --git a/lily/axis-group-interface.cc b/lily/axis-group-interface.cc
index
Creates a MIDI note length formatter (issue 5576062)
Reviewers: ,
Message:
The idea here is to create a generic framework that allows for
modifications to note lengths (i.e. swing) in the MIDI without having a
typographical impact on the score.
Description:
Creates a MIDI note length formatter
Please review this at http://codereview.appspot.com/5576062/
Affected files:
M lily/note-performer.cc
M scm/define-context-properties.scm
Index: lily/note-performer.cc
diff --git a/lily/note-performer.cc b/lily/note-performer.cc
index
01086620ef5e3f6ea23ce554926c52e7eb8ee338..c22a17199bc2470aa47eeac96ca8623e132aa052
100644
--- a/lily/note-performer.cc
+++ b/lily/note-performer.cc
@@ -53,6 +53,7 @@ Note_performer::process_music ()
Pitch transposing;
SCM prop = get_property (instrumentTransposition);
+ SCM length_formatter = get_property (midiNoteLengthFormatter);
if (unsmob_pitch (prop))
transposing = *unsmob_pitch (prop);
@@ -78,6 +79,13 @@ Note_performer::process_music ()
}
Moment len = get_event_length (n, now_mom ());
+ if (ly_is_procedure (length_formatter))
+{
+ len = *unsmob_moment (scm_call_3 (length_formatter,
+ len.smobbed_copy (),
+ get_property
(measureLength),
+ get_property
(measurePosition)));
+}
Audio_note *p = new Audio_note (*pitp, len,
tie_event, transposing.negated
());
@@ -129,7 +137,10 @@ ADD_TRANSLATOR (Note_performer,
,
/* read */
-,
+instrumentTransposition
+measureLength
+measurePosition
+midiNoteLengthFormatter ,
/* write */
Index: scm/define-context-properties.scm
diff --git a/scm/define-context-properties.scm
b/scm/define-context-properties.scm
index
43079859c638b83acba705060a6eba9413a85d2f..bae534e7a965eb0b4f329fbe8f41313ac00b267f
100644
--- a/scm/define-context-properties.scm
+++ b/scm/define-context-properties.scm
@@ -368,6 +368,7 @@ is used for ottava brackets.)
(middleCPosition ,number? The place of the middle C, measured in
half staff-spaces. Usually determined by looking at
@code{middleCClefPosition} and @code{middleCOffset}.)
+ (midiChannelMapping ,symbol? How to map MIDI channels: per
@code{instrument} (default), @code{staff} or @code{voice}.)
(midiInstrument ,string? Name of the MIDI instrument to use.)
(midiMergeUnisons ,boolean? If true, output only one MIDI note-on
event when notes with the same pitch, in the same MIDI-file track,
overlap.)
@@ -375,7 +376,10 @@ event when notes with the same pitch, in the same
MIDI-file track, overlap.)
@code{midiMinimumVolume}.)
(midiMinimumVolume ,number? Set the minimum loudness for MIDI.
Ranges from 0 to@tie{}1.)
- (midiChannelMapping ,symbol? How to map MIDI channels: per
@code{instrument} (default), @code{staff} or @code{voice}.)
+ (midiNoteLengthFormatter ,procedure? A procedure that takes three
+arguments: the length of a note, the length of the measure in which the
note
+falls, and the position of the note in the measure. The function must
return
+a moment that corresponds to the actual length of the note.)
(minimumFret ,number? The tablature auto string-selecting
mechanism selects the highest string with a fret at least
@code{minimumFret}.)
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
Reviewers: Patrick McCarty, dak, mike_apollinemike.com, janek,
http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc
File lily/grob.cc (right):
http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode177
lily/grob.cc:177: SCM expr = scm_list_3 (ly_symbol2scm (id),
On 2012/01/21 18:15:25, janek wrote:
isn't 'id' a scheme-thingy already? I mean, in line 174, it is
declared as SCM,
so why convert it with ly_symbol2scm?
We checked previously for a grob property called id that needs to be a
string. Here, we are creating a stencil. All stencils are represented
internally by lists where the first element is the name of the stencil
in symbol form.
http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode178
lily/grob.cc:178: id,
On 2012/01/21 18:15:25, janek wrote:
why store id two times?
The first id is the symbol id.
The second is the variable id that contains something else (foobar or
whatever).
http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode179
lily/grob.cc:179: retval.expr ());
On 2012/01/21 18:15:25, janek wrote:
do i understand correcly that retval stands for return value and is
some kind
of an object?
Yup. It should always be another stencil. This is a convention int he
code base.
http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode181
lily/grob.cc:181: retval = Stencil (retval.extent_box (), expr);
On 2012/01/21 18:15:25, janek wrote:
Do we overwrite the retval that was set earlier (in other if's)?
Why?
This is a way of saying the retval is equal to the old retval wrapped
in something new. Here, the new wrapping is an id. Earlier in
grob.cc, it's a color.
http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc
File lily/stencil-interpret.cc (right):
http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#newcode81
lily/stencil-interpret.cc:81: (*func) (func_arg, scm_list_2
(ly_symbol2scm (start-enclosing-id-node), id));
On 2012/01/21 18:15:25, janek wrote:
this line is longer than 80 chars, do we care about it?
I don't think so.
http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#newcode82
lily/stencil-interpret.cc:82: interpret_stencil_expression (scm_caddr
(expr), func, func_arg, o);
On 2012/01/21 18:15:25, janek wrote:
i understand that we extract actual stencil here and interpret it
again, but
what these func's do?
Depends what the func is. It's a way to pass a function as an argument.
Look for interpret_stencil_expression calls and you'll see what's
passed in why.
Description:
Implements DOM-id property for grobs.
Please review this at http://codereview.appspot.com/5504106/
Affected files:
A input/regression/id.ly
M lily/grob.cc
M lily/stencil-interpret.cc
M scm/define-grob-properties.scm
M scm/define-stencil-commands.scm
M scm/output-ps.scm
M scm/output-svg.scm
Index: input/regression/id.ly
diff --git a/input/regression/id.ly b/input/regression/id.ly
new file mode 100644
index
..10c628f3a8a549c286c94d50e2deaf32660e7fee
--- /dev/null
+++ b/input/regression/id.ly
@@ -0,0 +1,9 @@
+\version 2.15.27
+
+\header {
+ texidoc = Shows the id property of a grob being set. This should have
+no effect in the PS backend.
+
+}
+
+{ \override NoteHead #'id = #foo c }
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index
6c2eba1710fb15a283f5ecd50249f3ee7f11320f..ed96f1d13a9b1386a4c64912c2d823684b397097
100644
--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -170,6 +170,17 @@ Grob::get_print_stencil () const
= *unsmob_stencil (scm_call_1 (ly_lily_module_constant
(stencil-whiteout),
retval.smobbed_copy ()));
}
+
+ SCM id = get_property (id);
+ if (scm_is_string (id))
+{
+ SCM expr = scm_list_3 (ly_symbol2scm (id),
+ id,
+ retval.expr ());
+
+ retval = Stencil (retval.extent_box (), expr);
+}
+
}
return retval;
@@ -784,6 +795,7 @@ ADD_INTERFACE (Grob,
cause
color
cross-staff
+ id
extra-X-extent
extra-Y-extent
extra-offset
Index: lily/stencil-interpret.cc
diff --git a/lily/stencil-interpret.cc b/lily/stencil-interpret.cc
index
1d89e032ba2559051bc8d489fc339eacc7450df2..8214af5dcc8e40a427dcd80212a0931a74bcef6f
100644
--- a/lily/stencil-interpret.cc
+++ b/lily/stencil-interpret.cc
@@ -74,6 +74,16 @@ interpret_stencil_expression (SCM expr,
return;
}
+ else if (head == ly_symbol2scm (id))
+{
+ SCM id = scm_cadr (expr);
+
+ (*func) (func_arg, scm_list_2 (ly_symbol2scm
(start-enclosing-id-node), id));
+ interpret_stencil_expression (scm_caddr (expr), func, func_arg,
o);
+ (*func) (func_arg, scm_list_1
Doc: NR 5.5.5 Adv tweaks - Unpure-pure containers (issue 5569050)
Looks good! Much more user-understandable than anything I could have
ever come up with :)
http://codereview.appspot.com/5569050/diff/1/Documentation/notation/changing-defaults.itely
File Documentation/notation/changing-defaults.itely (right):
http://codereview.appspot.com/5569050/diff/1/Documentation/notation/changing-defaults.itely#newcode3714
Documentation/notation/changing-defaults.itely:3714: evaluated
correctly, usually returning a value of @var{0} or
It's not that the function is not evaluated correctly - it is not
evaluated at all, and lilypond subs in a dummy value.
http://codereview.appspot.com/5569050/diff/1/Documentation/notation/changing-defaults.itely#newcode3717
Documentation/notation/changing-defaults.itely:3717:
The definition of a pure function is that it does not result in
properties or objects being set or grob suicides.
http://codereview.appspot.com/5569050/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc File lily/rhythmic-music-iterator.cc (right): http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc#newcode62 lily/rhythmic-music-iterator.cc:62: if (scm_is_true ly_lily_module_constant should only be called for things that don't exist in C++. Otherwise, it's better to use the C++ function. In this case: ly_is_listened_event_class (in translator.cc) http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)
On 2012/01/20 15:33:07, dak wrote: Should I be forking the rhythmic-music-iterator stuff into the separate review I'd say leave it with this - it's easy to forget that two patches need to be tested in concert. http://codereview.appspot.com/5440084/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Broadcast articulations not in EventChord (issue 5528111)
I still think this patch goes outside the model of the way these things are usually designed in LilyPond. I'm not saying that LilyPond's design paradigm is objectively good or bad (I truly have no idea, as I know nothing about any other piece of software), but I think it's best to follow it whenever possible. Currently, this patch is making a modification to music-iterator, the base class from which all others inherit. Normally, be it an iterator, engraver, or grob, whenever a new functionality is needed in LilyPond, a new X is created (where X is iterator, engraver, or grob). There are only a few rare cases where the base class has to be modified (i.e. adding color to a grob, which effects every grob). I'll send you a sketch of a patch that's more in line with the way these things are coded in the current LilyPond style. There's no guarantee that it'll work, but it'll give you an idea of how to modify this one. Cheers http://codereview.appspot.com/5528111/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements rhythmic-music-iterator (issue 5554048)
Reviewers: dak, http://codereview.appspot.com/5554048/diff/2001/lily/rhythmic-music-iterator.cc File lily/rhythmic-music-iterator.cc (right): http://codereview.appspot.com/5554048/diff/2001/lily/rhythmic-music-iterator.cc#newcode41 lily/rhythmic-music-iterator.cc:41: report_event (get_music ()); On 2012/01/18 13:38:39, dak wrote: I suppose that one has to clear articulations here (after fetching them), or they will get treated by both report_event as well as the rhythmic iterator. Good call. A new patch set is up that fixes this. It adds some extra verbage, but it makes sure that: a) Notes are reported without their articulations; and b) Articulations are reported after notes. This should prevent the new-fingering-engraver from seeing the articulations. Furthermore, given the way that the music stream currently looks, articulations are placed after notes in an event chord. In theory this shouldn't matter, but in case it does, this puts the loop after the first report_event. Description: Implements rhythmic-music-iterator Please review this at http://codereview.appspot.com/5554048/ Affected files: A lily/include/rhythmic-music-iterator.hh A lily/rhythmic-music-iterator.cc M scm/define-music-types.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements rhythmic-music-iterator (issue 5554048)
On 2012/01/18 14:27:53, dak wrote: http://codereview.appspot.com/5554048/diff/5001/lily/rhythmic-music-iterator.cc File lily/rhythmic-music-iterator.cc (right): http://codereview.appspot.com/5554048/diff/5001/lily/rhythmic-music-iterator.cc#newcode42 lily/rhythmic-music-iterator.cc:42: get_music ()-set_property (articulations, SCM_EOL); I am not actually comfortable with clearing articulations in the original music event rather than its eventized copy. Are iterators allowed to change their input? I'm not sure if they're allowed by design, but I know they're allowed to proliferate input that wasn't there before. One solution is to just set a property in the event ignore-articulations. That way, when the new-fingering-engraver gets the event, it can check if this is set before it touches articulations. http://codereview.appspot.com/5554048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements rhythmic-music-iterator (issue 5554048)
The tuplet-iterator.cc has a process method that uses similar logic as that above: it does parts of what the report_music function does, because as you correctly state, the function does very little. LGTM - if you can, please adopt this patch so that you can coordinate getting it and others connected to it into the source. http://codereview.appspot.com/5554048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Broadcast articulations not in EventChord (issue 5528111)
I would advise not handling it this way - the function to_event is supposed to take music and return an event. In this patch, it is taking music, returning an event, and maybe broadcasting music and/or sending it to a context. I'd recommend creating a new iterator, rhythmic-music-iterator, that inherits from Simple_music_iterator. Have this be the constructor for the NoteEvent and RestEvent (iterator-ctor). Then, in this iterator, duriing process_music, have something that looks at an articulations callback of the iterator. This callback would handle the articulations list, much like the elements-callback handles the elements list (see the sequential-iterator). This is more in keeping with the way the code is written and would make it easier to follow/debug. Cheers, MS http://codereview.appspot.com/5528111/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sketch for DotColumn not triggering vertical alignment. (issue 5538049)
Reviewers: lemzwerg, carl.d.sorensen_gmail.com, Keith,
http://codereview.appspot.com/5538049/diff/1/lily/staff-symbol-referencer.cc
File lily/staff-symbol-referencer.cc (right):
http://codereview.appspot.com/5538049/diff/1/lily/staff-symbol-referencer.cc#newcode95
lily/staff-symbol-referencer.cc:95: Real y = (pure
On 2012/01/12 11:02:13, lemzwerg wrote:
The outermost parentheses have no effect. I suggest to remove them.
They're for code alignment purposes: it's so that the ? and : don't
align vertically with the -. There are a couple other places in the
code that use this convention (forget where).
http://codereview.appspot.com/5538049/diff/4002/lily/dot-column.cc
File lily/dot-column.cc (right):
http://codereview.appspot.com/5538049/diff/4002/lily/dot-column.cc#newcode216
lily/dot-column.cc:216: // do the X-offset properly.
On 2012/01/15 21:45:27, Keith wrote:
Might this comment now be obsolete? It is about the fix to issue
1088. If so,
we could unravel some of this complication, later.
It very well could be. After this patch is pushed, remind me to
investigate this (I'm writing myself a reminder as well).
Description:
Sketch for DotColumn not triggering vertical alignment.
Please review this at http://codereview.appspot.com/5538049/
Affected files:
A input/regression/dot-column-vertical-positioning.ly
M lily/dot-column.cc
M lily/include/staff-symbol-referencer.hh
M lily/staff-symbol-referencer.cc
Index: input/regression/dot-column-vertical-positioning.ly
diff --git a/input/regression/dot-column-vertical-positioning.ly
b/input/regression/dot-column-vertical-positioning.ly
new file mode 100644
index
..e609c7b4d03b6642f8352127501cb3e2ae8f208d
--- /dev/null
+++ b/input/regression/dot-column-vertical-positioning.ly
@@ -0,0 +1,13 @@
+\version 2.15.24
+
+\header {
+ texidoc = Dot columns should not trigger vertical spacing before
+line breaking. If the regtest issues a programming_error saying that
+vertical spacing has been called before line breaking, it has failed.
+
+}
+
+\context Staff
+ \new Voice { \voiceOne f''8.[ e''16] }
+ \new Voice { \voiceThree r8. a'16}
+
Index: lily/dot-column.cc
diff --git a/lily/dot-column.cc b/lily/dot-column.cc
index
2292d2fe0999b295623f8f9a064d7b96d90f1560..03f368d76b50703cd49055d54820417ec3248f63
100644
--- a/lily/dot-column.cc
+++ b/lily/dot-column.cc
@@ -143,7 +143,14 @@ Dot_column::calc_positioning_done (SCM smob)
}
}
- vector_sort (dots, position_less);
+ /*
+The use of pure_position_less and pure_get_rounded_position below
+are due to the fact that this callback is called before line breaking
+occurs. Because dots' actual Y posiitons may be linked to that of
+beams (dots are attached to rests, which are shifted to avoid beams),
+we instead must use their pure Y positions.
+ */
+ vector_sort (dots, pure_position_less);
for (vsize i = dots.size (); i--;)
{
if (!dots[i]-is_live ())
@@ -170,7 +177,7 @@ Dot_column::calc_positioning_done (SCM smob)
dp.x_extent_ = note-extent (commonx, X_AXIS);
}
- int p = Staff_symbol_referencer::get_rounded_position (dp.dot_);
+ int p = Staff_symbol_referencer::pure_get_rounded_position (dp.dot_);
/* icky, since this should go via a Staff_symbol_referencer
offset callback but adding a dot overwrites Y-offset. */
@@ -191,7 +198,7 @@ Dot_column::calc_positioning_done (SCM smob)
/*
Junkme?
*/
- Staff_symbol_referencer::set_position (i-second.dot_, i-first);
+ Staff_symbol_referencer::pure_set_position (i-second.dot_,
i-first);
}
me-translate_axis (cfg.x_offset () - me-relative_coordinate (commonx,
X_AXIS),
Index: lily/include/staff-symbol-referencer.hh
diff --git a/lily/include/staff-symbol-referencer.hh
b/lily/include/staff-symbol-referencer.hh
index
a3dd915c9ab85a96aed0986850f2091fdfbda3bb..179bae828f6905339b16ffa2757e67b9fb87fa12
100644
--- a/lily/include/staff-symbol-referencer.hh
+++ b/lily/include/staff-symbol-referencer.hh
@@ -33,6 +33,7 @@ public:
DECLARE_GROB_INTERFACE ();
static bool ugly_hack (Grob *);
static void set_position (Grob *, Real);
+ static void pure_set_position (Grob *, Real);
DECLARE_SCHEME_CALLBACK (callback, (SCM element));
/**
@@ -46,12 +47,19 @@ public:
static bool on_staff_line (Grob *, int);
static int line_count (Grob *);
static Real get_position (Grob *);
+ static Real pure_get_position (Grob *);
static Real staff_radius (Grob *);
static int get_rounded_position (Grob *);
+ static int pure_get_rounded_position (Grob *);
static Interval extent_in_staff (Grob *);
+
+private:
+ static void internal_set_position (Grob *, Real, bool);
+ static Real internal_get_position (Grob *, bool);
};
int compare_position (Grob *const , Grob *const );
bool position_less (Grob *const , Grob *const );
+bool pure_position_less (Grob *const ,
