|
|
Created:
13 years ago by MikeSol Modified:
12 years, 12 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAdds beam collision avoidance to auto beaming
Patch Set 1 : Adds beam collision avoidance to auto beaming #Patch Set 2 : Fixes sorting algorithm #
Total comments: 5
Patch Set 3 : Makes logic of loops in finalize more transparent #
Total comments: 10
Patch Set 4 : Incorporate's Neil's suggestions. #
Total comments: 2
MessagesTotal messages: 13
Actualizes Han Wen's idea for putting everything in finalize. Seems to work... Cheers, Mike
Sign in to reply to this message.
http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:55: if (covered_grobs_.size ()) I'm going to start testing this now, but it's sad to see: - an 8-level nested if/loop construct (!(?(!(?(!)?)!)?)!) - so many comments vanishing Hopefully somebody can suggest a refactoring which is easier to read?
Sign in to reply to this message.
On Mar 27, 2011, at 3:29 PM, percival.music.ca@gmail.com wrote: > > http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.cc > File lily/beam-collision-engraver.cc (right): > > http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... > lily/beam-collision-engraver.cc:55: if (covered_grobs_.size ()) > I'm going to start testing this now, but it's sad to see: > - an 8-level nested if/loop construct (!(?(!(?(!)?)!)?)!) I only count 7! But the indentation was off (fixed). > - so many comments vanishing > That's because most of the code vanished. I can add a comment if you want - where would it benefit most from a comment (meaning where is it difficult to understand from variable names & from following the loops?)? Cheers, MS
Sign in to reply to this message.
http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:55: if (covered_grobs_.size ()) to drop 1 indent level, do if (covered.empty()) return; http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:81: start = 0; what graham said. Maybe you can add a comment about how you're going about it? In particular, the logic around start looks odd. i'd write something like this: for (i over beams) { while (covered[start].rank[left] < beam[i].right) start++ for (j = start; covered[j] < beam.right; j++) { ..maybe add covered[j] to beam[i].. } }
Sign in to reply to this message.
On Mon, Mar 28, 2011 at 8:13 AM, <hanwenn@gmail.com> wrote: > > http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.cc > File lily/beam-collision-engraver.cc (right): > > http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... > lily/beam-collision-engraver.cc:55: if (covered_grobs_.size ()) > to drop 1 indent level, do > > if (covered.empty()) > return; > > http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... > lily/beam-collision-engraver.cc:81: start = 0; > what graham said. > > Maybe you can add a comment about how you're going about it? > > In particular, the logic around start looks odd. > > i'd write something like this: > > for (i over beams) { > while (covered[start].rank[left] < beam[i].right) make that while (covered[start].rank[right] < beam[i].rank[left] start++ > start++ > > for (j = start; covered[j] < beam.right; j++) { > ..maybe add covered[j] to beam[i].. > } > } > > http://codereview.appspot.com/4290069/ > -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
Fixed w/ Han-Wen's suggestions. http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:55: if (covered_grobs_.size ()) On 2011/03/28 11:13:35, hanwenn wrote: > to drop 1 indent level, do > > if (covered.empty()) > return; > Done. http://codereview.appspot.com/4290069/diff/5001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:81: start = 0; On 2011/03/28 11:13:35, hanwenn wrote: > what graham said. > > Maybe you can add a comment about how you're going about it? > > In particular, the logic around start looks odd. > > i'd write something like this: > > for (i over beams) { > while (covered[start].rank[left] < beam[i].right) > start++ > > for (j = start; covered[j] < beam.right; j++) { > ..maybe add covered[j] to beam[i].. > } > } Done.
Sign in to reply to this message.
http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:50: sort (covered_grobs_.begin (), covered_grobs_.end (), Grob::less); vector_sort (covered_grobs_, Grob::less); http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:51: sort (beams_.begin (), beams_.end (), Grob::less); vector_sort (beams_, Grob::less); http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:54: if (!covered_grobs_.size ()) You probably want this at the top. http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:59: // Start conisdering grobs at the first grob whose end falls at or after the beams beginning. considering beam's http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:66: Only consider grobs whose end falls at or after the beams beginning. beam's
Sign in to reply to this message.
New patch set uploaded. http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:50: sort (covered_grobs_.begin (), covered_grobs_.end (), Grob::less); On 2011/03/28 20:50:09, Neil Puttock wrote: > vector_sort (covered_grobs_, Grob::less); Done. http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:51: sort (beams_.begin (), beams_.end (), Grob::less); On 2011/03/28 20:50:09, Neil Puttock wrote: > vector_sort (beams_, Grob::less); Done. http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:54: if (!covered_grobs_.size ()) On 2011/03/28 20:50:09, Neil Puttock wrote: > You probably want this at the top. Done. http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:59: // Start conisdering grobs at the first grob whose end falls at or after the beams beginning. On 2011/03/28 20:50:09, Neil Puttock wrote: > considering > > beam's Done. http://codereview.appspot.com/4290069/diff/8001/lily/beam-collision-engraver.... lily/beam-collision-engraver.cc:66: Only consider grobs whose end falls at or after the beams beginning. On 2011/03/28 20:50:09, Neil Puttock wrote: > beam's Done.
Sign in to reply to this message.
Looks much more readable, thanks! http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver... lily/beam-collision-engraver.cc:59: // Start conisdering grobs at the first grob whose end falls at or after the beam's beginning. typo: conisdering
Sign in to reply to this message.
On Mar 28, 2011, at 6:39 PM, percival.music.ca@gmail.com wrote: > Looks much more readable, thanks! > > > http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver.cc > File lily/beam-collision-engraver.cc (right): > > http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver... > lily/beam-collision-engraver.cc:59: // Start conisdering grobs at the > first grob whose end falls at or after the beam's beginning. > typo: conisdering Co = with nis = niche (Dutch) dering = from dere, "to harm" in old English, which is a Dutch variant, thus the "nis" As the beam collision engraver prevents beams in the same niche from harming each other, I felt that the neologism was appropriate. But, for ease of reading, I'll change it to considering, although it no longer encapsulates the meaning I was trying to imbue into the word. Cheers, MS
Sign in to reply to this message.
On Mar 28, 2011, at 6:39 PM, percival.music.ca@gmail.com wrote: > Looks much more readable, thanks! > > > http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver.cc > File lily/beam-collision-engraver.cc (right): > > http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver... > lily/beam-collision-engraver.cc:59: // Start conisdering grobs at the > first grob whose end falls at or after the beam's beginning. > typo: conisdering > > http://codereview.appspot.com/4290069/ Can somebody please test this patch out on a few thorny keyboard pieces before I push it? It will make beam collision kick in on all beams, and I want to make sure that I don't run into the same issue I did w/ accidentals. I would do it myself, but I am not in possession of anything that could really test this to the max. Cheers, MS
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver.cc File lily/beam-collision-engraver.cc (right): http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver... lily/beam-collision-engraver.cc:64: for (vsize j = start; covered_grobs_[j]->spanned_rank_interval ()[LEFT] <= beams_[i]->spanned_rank_interval ()[RIGHT]; j++) you migth want to store beam[i]->spanned_rank_interval() in a local variable to improve readability.
Sign in to reply to this message.
On Mar 30, 2011, at 12:30 AM, hanwenn@gmail.com wrote: > lgtm > > > http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver.cc > File lily/beam-collision-engraver.cc (right): > > http://codereview.appspot.com/4290069/diff/11001/lily/beam-collision-engraver... > lily/beam-collision-engraver.cc:64: for (vsize j = start; > covered_grobs_[j]->spanned_rank_interval ()[LEFT] <= > beams_[i]->spanned_rank_interval ()[RIGHT]; j++) > you migth want to store beam[i]->spanned_rank_interval() in a local > variable to improve readability. Done and pushed. e4be20302c832985b7faac6fc0daf1f45f382391 Cheers, MS
Sign in to reply to this message.
|