On Mon, Jan 24, 2011 at 9:40 PM, Neil Puttock wrote:
> + common[a] = common_refpoint_of_array (covered_grobs, me, Axis (a));
>
> I suspect this line needs changing, otherwise it junks the calculated
> refpoint for the stems (which means we no longer get the correct
> refpoint for cross-staff
On 24 January 2011 14:28, Han-Wen Nienhuys wrote:
> See attached (it applies on top of issue3928041_55001.diff)
+ common[a] = common_refpoint_of_array (covered_grobs, me, Axis (a));
I suspect this line needs changing, otherwise it junks the calculated
refpoint for the stems (which means we
On Mon, Jan 24, 2011 at 11:22 AM, Han-Wen Nienhuys wrote:
> Let me whip up a prototype for you to see how it works.
See attached (it applies on top of issue3928041_55001.diff)
It is imperfect in several ways,
- see TODOs in the code.
- 4th beat 1st line: should go below
- 5th beat 1st line: sho
On Mon, Jan 24, 2011 at 10:49 AM, Mike Solomon wrote:
> I had just reverted it to the old behavior (I think...).
>
> The question is: when we have a collision like the third example, how much do
> we shorten the stems before it becomes ridiculous? We could shorten the
> stems to avoid the colli
I had just reverted it to the old behavior (I think...).
The question is: when we have a collision like the third example, how much do
we shorten the stems before it becomes ridiculous? We could shorten the stems
to avoid the collision there, but if the note were a perfect 5th lower, it'd
lead
On Mon, Jan 24, 2011 at 9:42 AM, Mike Solomon wrote:
> Fixed, although I have no idea what the "desired output" is in this case (I
> have a feeling it's "Hey...you asked for it...").
IMO, in the last case, the stems should be shortened so there is no
collision; why is there a collision now?
--
Mike, I tried Chopin preludes Opus 28 No. 1 (where I thought there might
be trouble because beams need to cross stems there) and No. 2 (the
classic example that benefits from this fix).
These, and the large score + parts I use for informal checking, look
good.
Hope you can keep weaving it in to th
On Mon, Jan 24, 2011 at 2:46 AM, Han-Wen Nienhuys wrote:
>> I hope that my clearer code will assuage your fears that this affects
>> anything other than certain limited scenarios - in the regtests, it
>
> that is not so much my fear. I am concerned with putting in code that
> we know is flawed i
On Mon, Jan 24, 2011 at 12:58 AM, wrote:
>> acknowledge manual beams only instead, then there's no need for this
>
> hack
>
> OK - but as far as I know, there is no way to have DECLARE_ACKNOWLEDGER
> (manual_beam) . What would be the appropriate way to do this?
Look at the cause of the beam gr
http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1014
lily/beam.cc:1014: pos[RIGHT] += offset;
also, these offsets disregard carefully computed beam quantizations.
Even though these are f
http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:05:39, hanwenn wrote:
it looks like this only handles the 1st coll
Sorry, I got the author incorrect.
Thanks for looking into this, Mike!
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Hi Carl,
thanks for diving into this!
I love the idea of handling beam collisions, but I think the design of
the backend code is flawed. See my comments.
http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/3928041/diff/30001/
New patch set uploaded.
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
I made a lot of changes to get things looking more lilypond-y.
I also now have it better handling scenarios where the subdivisions are
wacky. Check out:
\relative c' { << { b32 [ c,16 c'8 d,64 e'8. c,32 e'' ] } \\ { f8 [ c32
d'32 bes,,64 c'64 c64 c'128 ] } >> }
The low D is snug as a bug in a
I noticed another nitpick.
http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly
File input/regression/beam-collision.ly (right):
http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly#newcode1
input/regression/beam-collision.ly:1: \ver
Looks good, but there's still some excessive beam translation in
slur-scoring.ly.
http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision.ly
File input/regression/beam-collision.ly (right):
http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collision
LGTM.
Carl
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Regtest comparison looks fine, builds the docs from scratch. Let's push
it in 24 hours if there's no complaints.
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypo
New patch set uploaded.
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Original Message
> Only unnecessary lengthening now is some stems on
> cue notes, but it really sticks out.
Well, as they say, "pictures or it didn't happen" The cue from the
trumpets looks desperate for attention.
> I could only reduce it to four
> required ingredients: manu
I only re-checked that one score, after the patch to overcome the
autobeam confusion. Only unnecessary lengthening now is some stems on
cue notes, but it really sticks out. I could only reduce it to four
required ingredients: manual beams, 32nd notes, dots, and CueVoice.
\music = \relative c'' {
New patches pushed.
Thanks,
Carl
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Done - attached is a fresh patch set. The first three are what's already on
Rietveld, and the 4th is Carl's formatting & copyright changes.
Cheers,
MS
0004-Changes-suggested-by-Carl.patch
Description: Binary data
0003-Intermediary-37-patch.patch
Description: Binary data
0002-Fixes-formatti
LGTM.
Don't forget to fix your copyright on beam-collision-engraver.
One set of braces to be removed.
Thanks,
Carl
http://codereview.appspot.com/3928041/diff/17001/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):
http://codereview.appspot.com/3928041/diff/17001
Latest patch set uploaded with full side-by-side-diffs.
Thanks,
Carl
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Hey Keith,
I believe the new patch set fixes that problem.
Cheers,
MS
On Jan 8, 2011, at 9:01 PM, Keith OHara wrote:
> Mike,
> I like it on small scores.
>
> I let your patch compile a symphony and got several unneeded stem
> lengthenings per page. I started carving out a reasonable-sized .l
Mike,
I like it on small scores.
I let your patch compile a symphony and got several unneeded stem
lengthenings per page. I started carving out a reasonable-sized .ly to
send for you to test, but that might actually take longer than you
recognizing the problem from the output. Three images at
On 1/8/11 12:48 PM, "Mike Solomon" wrote:
> All is fixed. Attached is the original patch plus one with Carl's suggested
> formatting changes.
>
> I ran all of the regtests and nothing breaks.
>
> I'd like one other person to test these patches out w/ the attached .ly
> document to confirm. Th
On 1/8/11 12:52 PM, "Neil Puttock" wrote:
> On 8 January 2011 19:48, Mike Solomon wrote:
>
>> I ran all of the regtests and nothing breaks.
>
> This is not good enough.
>
> You *must* do a proper regression test comparison.
As defined in the CG, this means:
Run
make test-baseline
bef
On 1/8/11 12:51 PM, "David Kastrup" wrote:
> Mike Solomon writes:
>
>> All is fixed. Attached is the original patch plus one with Carl's
>> suggested formatting changes.
>>
>> I ran all of the regtests and nothing breaks.
>>
>> I'd like one other person to test these patches out w/ the at
On 1/8/11 12:48 PM, "Mike Solomon" wrote:
> All is fixed. Attached is the original patch plus one with Carl's suggested
> formatting changes.
>
Latest patch is posted on Rietveld, as
http://codereview.appspot.com/3928041/
Thanks,
Carl
___
li
On 8 January 2011 19:48, Mike Solomon wrote:
> I ran all of the regtests and nothing breaks.
This is not good enough.
You *must* do a proper regression test comparison.
Withough doing make check, I can already see it will fail since you
haven't documented covering-note-heads in beam.cc and
def
Mike Solomon writes:
> All is fixed. Attached is the original patch plus one with Carl's
> suggested formatting changes.
>
> I ran all of the regtests and nothing breaks.
>
> I'd like one other person to test these patches out w/ the attached
> .ly document to confirm. Then, if there are no oth
All is fixed. Attached is the original patch plus one with Carl's suggested
formatting changes.
I ran all of the regtests and nothing breaks.
I'd like one other person to test these patches out w/ the attached .ly
document to confirm. Then, if there are no other other comments, please push
t
Carl,
Thanks for the comments!
I've integrated all but one of them - when you say "Also add to TabStaff," what
do you mean? I was under the impression that TabStaff would inherit this
engraver because it inherits from Staff.
Cheers,
MS
On Jan 8, 2011, at 1:38 PM, carl.d.soren...@gmail.com wro
Carl,
Thanks for the comments!
I've integrated all but one of them - when you say "Also add to TabStaff," what
do you mean? I was under the impression that TabStaff would inherit this
engraver because it inherits from Staff.
Cheers,
MS
On Jan 8, 2011, at 1:38 PM, carl.d.soren...@gmail.com wro
Reviewers: MikeSol,
Message:
Looks great, Mike!
You have some code indentation issues that don't match our style. Other
than that, I think it's good to go.
Of course, we do need a regression test as well.
Thanks,
Carl
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engrav
38 matches
Mail list logo
