Menu
▾
▴
1 Attachments
#984 Noteheads don't merge when one of the voices contains a second
Accepted
nobody
None
Defect
2015-09-28
2010-01-19
Anonymous
No
Originally created by: *anonymous
Originally created by: v.villenave
% I don't think this has been reported before. Looks like a side-effect of
% the way issue 618 was fixed (it's also somehow similar to issue 723).
\version "2.13.11" % Ideally, in the following example the two % middle Gs should be merged similarly. \new PianoStaff << \new Staff = "up" \relative c'' { \stemUp g'8 \change Staff = "down" g, % this one does merge \change Staff = "up" g' \change Staff = "down" g, % this one doesn't. } \new Staff = "down" \relative c' { \stemDown r8 <g a g'> r <g f' g> } >>
Related
Issues: #1713
Issues: #618
Issues: #723
Originally posted by: v.villenave
FTR, here's the (buggy) output with 2.11.44 (before issue 618 was fixed).
Labels: -Type-Enhancement -Priority-Low -engraving-nitpick Type-Defect Priority-Medium
Owner: ---
Related
Issues:
#618Originally posted by: k-ohara5...@oco.net
There is a TODO comment in note-collision.cc check_meshing_chords() indicating a plan to resolve this, by filtering out the heads on the unusual side of the stem from the test for mesh-ability.
Originally posted by: v.villenave
I'm putting a €80 bounty on this one. It prevents me from publishing my latest work.
Labels: Bounty
Originally posted by: dak@gnu.org
I am having a go at it. I need to get more accustomed to engravers anyway.
Owner: dak@gnu.org
Cc: v.villenave
Originally posted by: mts...@gmail.com
Allows seconds to merge
http://codereview.appspot.com/5729057
Labels: Patch-new
Originally posted by: mts...@gmail.com
I sent David an e-mail about this, as he and I had a Newton-Leibnitz problem on this issue (w/in seconds of the bounty, I started looking into the feasibility of a solution and during that time David posted his response. While doing my preliminary tests, I wound up solving the problem). David was cool with me posting the patch if it wasn't kludgy, and I think it is an appropriate solution.
I don't want the bounty from Valentin, but I've been lazy about getting a good OuMuPo site up and running, so he can use the €80 worth of time to do some web-programming :)
Originally posted by: dak@gnu.org
Patchy the autobot says: LGTM. collision-head-chords (second example could conceivably also be merged) and collision-seconds arguably improved. But please check the Rietveld review.
Labels: Patch-review
Originally posted by: dak@gnu.org
In previous comment, "second example" actually refers to the group in the second measure (which is quite far from being the second example). This is an interesting corner case. Since I am still tempted to do an algorithmic patch replacing the stream of exceptions in the current implementation, it would be nice to get a quotation from a reference. If this figure were merged, one would basically have one visual stem running from top to bottom, with flags at top and bottom. The notehead configuration can be considered compatible, but would that kind of straight stem be acceptable?
Originally posted by: k-ohara5...@oco.net
The "second example" in the second measure of ‘collision-head-chords.ly’ is
<< <c d> \\ <c d> >>
so our choices are to separate the chords completely or merge completely so that the stems are set on the same line.
The code uses the term stems-'touch' for the situation of stems-on-the-same-line, distinct from 'merge' when the stems are on either side of a column of merged heads. The 'touch' case takes a different path through the code than the 'merge' case.
I suggest that merging a chord of seconds with another chord of seconds (making the stems 'touch') is outside the scope of this issue.
Labels: -Patch-review Patch-needs_work
Originally posted by: dak@gnu.org
Hi Keith,
Mike has expressed his willingness to let me take this issue over and "do it my way". My personal take is to conceptually take both stems and move them across each other horizontally, recording all constellations that "make sense".
This might even mean merging noteheads on the "secondary" stem. If you take a look at the stem code, you'll notice that the reflection is not actually symmetric. So even while vertical and horizontal positions are measured in different units and not considered at the same time, the horizontal positions are needed accurately, and the pairing of horizontal and vertical position should be maintained.
That is a part of Mike's patch that makes perfect sense to me. Now the old code and Mike's addition are as a combination just too complex for me to feel good about them, in particular since the preexisting part makes assumptions about how seconds are arranged. That's not good since it means that the stem code is not free to be changed without repercussions on the stem merger's assumptions.
So I'll try coming up with something new tomorrow, with the overall goal of reducing rather than increasing the amount of code lines, and hopefully with fewer exceptions.
Originally posted by: v.villenave
Greetings everybody,
where do we stand with this issue? David, if you're investigating it (or planning to), then perhaps it'd make sense to mark it as "Started" and remove Mike's Patch-needs_work label -- or is Mike's proposal still relevant in some way?
Originally posted by: dak@gnu.org
Sorry, yes I am working on it. There have been a few interruptions and distractions in the last few days, and the preexisting code actually compares only the extremal noteheads (which is not quite enough). But I hope to finish by Monday.
Status: Started
Originally posted by: k-ohara5...@oco.net
This might be easier than you think.
Mike's patch actually worked just fine; it was merely obfuscated. (A bit of its code /appeared/ to make the error that David mentions, but that code had no effect.)
Here is a cleaned version using that idea <http://codereview.appspot.com/5823046>
The case where both voices have identical seconds
<< <c d> \\ <c d> >>is not merged by default, because that could be mistaken for<<d \\ c>>. One can \override NoteColumn #'force-hshift = #0.95 on the lower voice and #0.0 on the upper voice to merge them by hand.Labels: -Patch-needs_work Patch-new
Last edit: Trevor Daniels 2015-09-28
Originally posted by: dak@gnu.org
It might be harder than you think. The current code is a total mess that for most decisions only consults the properties of the extremal note heads. This only works where all noteheads on a stem are actually identical. Which is the most common case, but definitely the only one. The loop over the noteheads will not be correct in several cases where there are multiple noteheads, not all of them mergeable.
I am taking so long not because I am just adding a "missing part": that one is actually pretty easy. But because the whole thing needs rewriting. I am not interested in getting a higher number of cases right by accident. So that needs some work, some more stuff needs to get pulled into the loop, and I need to consult the _corresponding_ noteheads for information about merging. Which means that I have to sort not just the notehead coordinates but rather the whole notehead info.
And so forth and so on. I don't like leaving code where I can just write down a number of regtests that will fail under it.
Originally posted by: dak@gnu.org
Tell you what: I'm still days away from getting my code finished (which is pretty much a rewrite of the whole function). The current code is a heap of assumptions and heuristics. Making Valentin's current bounty case (and a few more) work out better is reasonably simple in contrast, and that is what this issue and its bounty is about, and Valentin has a score to get published. You can't make this do the right thing in cases involving harmonics in non-trivial ways, and it won't ever work right with "string chords" where the individual notes have different durations, which is one of the reasons I have not further worked on patches in that area (I am actually surprised that there does not seem to be an issue for those; I remember Werner requesting them in the context of the Bach solo violin works).
So this needs to be done right, and in the mean time I have a conference talk about LilyPond coming up for which not one slide has been written yet and where I leave on Friday. I have to put this on hold till at least the next week. So go ahead with the quick fix if you want (and there is a need for it, anyway, and a bounty). It does not really interfere with my work on this function, and that will have to carry at least into next week.
Originally posted by: dak@gnu.org
Patchy the autobot says: LGTM. Of course, my opinion about Patchy here is somewhat akin to Picasso's "Computers are useless. They can only provide answers.": we get more cases right at the cost of more code based on flawed assumptions. The day has too few hours.
Labels: Patch-review
Originally posted by: k-ohara5...@oco.net
True, that the simple patch does not discover that it can merge stopped harmonics; we still need an override if we want to merge them. It handles the trivial cases better overall, though.
~~~~
:::TeX
\version "2.13.32" \relative f << {\textLengthOn
<d' a'="" b="" d="">2 q q q | <d e="" f="" g="" a=""> <d a'b=""> <a' b="">
\once\override NoteColumn #'force-hshift = #0
^\markup\tiny"override" | <d, a'\\harmonic="">
\once\override NoteColumn #'force-hshift = #0
<d a'\\harmonic="">^\markup\tiny"override" <d a'\\harmonic=""> <d a'="">
} \ {
<d a'="" c="" d=""> <d f="" a="" d=""> <e f="" f'=""> | <d, e="" f="" g=""> <d g="" a=""> <a' b="">
\once\override NoteColumn #'force-hshift = #0.95
| <d, a'\\harmonic="">
\once\override NoteColumn #'force-hshift = #0
<d a'\\harmonic=""> <d a'=""> <d a'\\harmonic="">
} >>
~~~~
Last edit: Trevor Daniels 2015-09-28
Originally posted by: ColinPKC...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-review Patch-countdown
Originally posted by: v.villenave
I'm delighted to see that this issue **could** lead to better code and better in-depth maintainability/reliability; David, if you're still up for this, the bounty is yours (Mike told me he didn't care).
So, if the current patch is to be accepted and merged, should "we" open a new issue (possibly tagged as Enhancement) re. David's upcoming work?
Labels: -Bounty
Owner: ---
Originally posted by: dak@gnu.org
I don't see the need for the red tape. It's the next thing I need to finish (currently I am on the Linux fair in Chemnitz and mostly involved with talking to people and preparing my talk tomorrow), and I have started coding it. It is annoyingly extensive work since the existing code is based on a lot of assumptions (which _are_ valid in the majority of cases, or we would have seen more problems by now), but even though they are mostly valid, they don't make sense. If the assumption is that you have just the same duration and notehead style on one stem, intermingling looking at just one notehead, looking at the stem, looking at the notehead currently being spaced without rhyme or reason might seem all ok.
But it takes time to analyze what really makes sense when you stop assuming that everything will be the same anyway. Even though it will be in most cases.
I am afraid that I've drunk to much to make sense right now.
Originally posted by: ColinPKC...@gmail.com
Counted down to 201203128, please push.
Labels: -Patch-countdown Patch-push
Originally posted by: k-ohara5...@oco.net
The simple fix (comment 2,13) is committed as [r4bae96d93d05b96bbf79b722155b2b5795418278]
Labels: -Patch-push Fixed_2_15_34
Status: Fixed
Originally posted by: colingh...@gmail.com
I just started verifying this one but ran out of time. A couple of test files are attached, extracted from above. I'll finish this in the morning.
Originally posted by: k-ohara5...@oco.net
Probably this fix will not appear until version .35 (the commit was still in staging when version .34 was built.)
Labels: -Fixed_2_15_34 Fixed_2_15_35
Originally posted by: ma...@gregoriana.sk
(No comment was entered for this change.)
Status: Verified