Re: Colored box behind a single note
Werner LEMBERG writes: >>> If someone is going to rewrite the algorithm, the order of events >>> should be obeyed. >> >> For simultaneous events, LilyPond has no order. > > Oh. This means that > > c'\startGroup\stopGroup > > and > > c'\stopGroup\startGroup > > can't be reliably distinguished? Not completely. In particular, some engravers might get to see the \startGroup before other engravers see the \stopGroup. Also things like \startGroup and \stopGroup are quite likely to be placed in a separate music variable that is being combined using parallel music. So while there is a typical order of events, it's not a guaranteed one. >> That's why one needs to have one phase for recording events and >> another for doing something based on them. > > Hmm. I got the impression that `listen_note_grouping' is this > recording-events phase. I'd have to take a closer look. So far, I've mostly did code review by reflex, just knowing what kind of trouble LilyPond is prone to cause. >> Maybe the infamous issue 34 could be tackled as part of that >> mechanism? Not sure about that. > > It would be nice if we find a solution for that. You bet. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Colored box behind a single note
>> If someone is going to rewrite the algorithm, the order of events >> should be obeyed. > > For simultaneous events, LilyPond has no order. Oh. This means that c'\startGroup\stopGroup and c'\stopGroup\startGroup can't be reliably distinguished? > That's why one needs to have one phase for recording events and > another for doing something based on them. Hmm. I got the impression that `listen_note_grouping' is this recording-events phase. > Maybe the infamous issue 34 could be tackled as part of that > mechanism? Not sure about that. It would be nice if we find a solution for that. Werner ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Colored box behind a single note
Werner LEMBERG writes:
>>>if (d == STOP)
>>> {
>>>pop_count_++;
>>> - if (pop_count_ > bracket_stack_.size ())
>>> +
>>> + // Since N `L' events create N HorizontalBracket grobs we need (at
>>> + // most) N `R' events to finish them. However, at this particular
>>> + // moment, a single-moment horizontal bracket might be created also;
>>> + // this takes another `L' event (which might not have caused a new
>>> + // element on `bracket_stack' yet) and its corresponding `R' event.
>>> + if (pop_count_ > bracket_stack_.size () + 1)
>>> ev->origin ()->warning (_ ("do not have that many brackets"));
>>> }
>>
>> "which might not have caused a new element"? That sounds like this
>> may or may not suppress an actually warranted warning.
>>
>> Correct?
>
> Right, thanks for noticing. I've fixed this – to a certain extent,
> see comments in the attached new version of the patch.
>
> If someone is going to rewrite the algorithm, the order of events
> should be obeyed.
For simultaneous events, LilyPond has no order. That's why one needs to
have one phase for recording events and another for doing something
based on them.
There is a very tricky exception in order to distinguish
[Some override] \grace { ...
from
\grace { [Some override] ...
since \grace itself overrides a number of properties (mostly sizes) and
the order needs to be heeded. The grace iterator creates special events
for that purpose.
Maybe the infamous issue 34 could be tackled as part of that mechanism?
Not sure about that.
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Colored box behind a single note
>>if (d == STOP)
>> {
>>pop_count_++;
>> - if (pop_count_ > bracket_stack_.size ())
>> +
>> + // Since N `L' events create N HorizontalBracket grobs we need (at
>> + // most) N `R' events to finish them. However, at this particular
>> + // moment, a single-moment horizontal bracket might be created also;
>> + // this takes another `L' event (which might not have caused a new
>> + // element on `bracket_stack' yet) and its corresponding `R' event.
>> + if (pop_count_ > bracket_stack_.size () + 1)
>> ev->origin ()->warning (_ ("do not have that many brackets"));
>> }
>
> "which might not have caused a new element"? That sounds like this
> may or may not suppress an actually warranted warning.
>
> Correct?
Right, thanks for noticing. I've fixed this – to a certain extent,
see comments in the attached new version of the patch.
If someone is going to rewrite the algorithm, the order of events
should be obeyed. To allow overlapping horizontal brackets, hopefully
the same mechanism as used for overlapping slurs can be used.
Werner
diff --git a/lily/horizontal-bracket-engraver.cc b/lily/horizontal-bracket-engraver.cc
index 608af35965..4b18cc3ad7 100644
--- a/lily/horizontal-bracket-engraver.cc
+++ b/lily/horizontal-bracket-engraver.cc
@@ -28,6 +28,39 @@
#include "translator.icc"
+// In the following (simplified) description of the algorithm, we abbreviate
+// a horizontal bracket's `START' and `STOP' span-direction with `L' and
+// `R', respectively.
+//
+// For a given moment do the following.
+//
+// (1) Count the number of `L' and `R' events; they are also pushed onto a
+// stack so that they can be referenced later on (method
+// `listen_note_grouping').
+//
+// (2) Create a HorizontalBracket spanner for every `L' event, link it as
+// necessary, and push it onto `bracket_stack' (method `process_music').
+//
+// (3) Set one boundary for every element of `bracket_stack' (method
+// `acknowledge_note_column').
+//
+// (4) Check whether there is a single `R' event within a bunch of `L' events
+// (or a single `L' event within a bunch of `R' events) and set the
+// other boundary with the same values to create a single-moment
+// horizontal bracket for this case (method `acknowledge_note_column').
+//
+// (5) At a moment with a series of `R' events, `bracket_stack' normally
+// contains elements created by `L' events from earlier moments (plus
+// one possible single-moment horizontal bracket). Pop an element off
+// `bracket_stack' for every `R' event and ignore excessive `R' events
+// (method `stop_translation_timestep').
+//
+// The above algorithm allows for nested horizontal brackets.
+//
+// TODO: Allow overlapping horizontal brackets (issue #5240); in particular,
+// respect the order of `L' and `R' events for a given moment. This
+// would need a complete rewrite of the algorithm, however.
+
class Horizontal_bracket_engraver : public Engraver
{
public:
@@ -56,10 +89,32 @@ Horizontal_bracket_engraver::listen_note_grouping (Stream_event *ev)
{
Direction d = to_dir (ev->get_property ("span-direction"));
+ // Allow one single-moment bracket. This means that we can have
+ //
+ // LLL...LLR
+ //
+ // or
+ //
+ // LRRR...RR .
+ //
+ // Note that the order of events doesn't matter; however, to avoid a false
+ // warning the single `L' event in a series of `R' events should not come
+ // last; similarly, the single `R' event in a series of `L' events should
+ // not come first. In other words, both `RRRL' and `RLLL' are OK but make
+ // LilyPond complain without reason. I think it is not worth fixing this,
+ // given that the algorithm should be rewritten anyway.
+
if (d == STOP)
{
pop_count_++;
- if (pop_count_ > bracket_stack_.size ())
+
+ // Since N `L' events create N HorizontalBracket grobs we need (at
+ // most) N `R' events to finish them. However, at this particular
+ // moment, a single-moment horizontal bracket might be created also;
+ // this takes another `L' event and its corresponding `R' event.
+ int extra = (pop_count_ && push_count_) ? 1 : 0;
+
+ if (pop_count_ > bracket_stack_.size () + extra)
ev->origin ()->warning (_ ("do not have that many brackets"));
}
else
@@ -68,22 +123,41 @@ Horizontal_bracket_engraver::listen_note_grouping (Stream_event *ev)
events_.push_back (ev);
}
- if (pop_count_ && push_count_)
+ // The handled number of events are
+ //
+ // LLL...LLR => push_count_ >= 1 && pop_count_ == 1
+ //
+ // or
+ //
+ // LRR...RRR => pop_count_ >= 1 && push_count_ == 1 .
+ //
+ if (pop_count_ >= 2 && push_count_ >= 2)
ev->origin ()->warning (_ ("conflicting note group events"));
}
void
Horizontal_bracket_engraver::acknowledge_note_column (Grob_info gi)
{
+ bool process_single_moment_bracket = pop_count_
Re: Colored box behind a single note
Werner LEMBERG writes:
>>> diff --git a/lily/horizontal-bracket-engraver.cc
>>> b/lily/horizontal-bracket-eng
>if (d == STOP)
> {
>pop_count_++;
> - if (pop_count_ > bracket_stack_.size ())
> +
> + // Since N `L' events create N HorizontalBracket grobs we need (at
> + // most) N `R' events to finish them. However, at this particular
> + // moment, a single-moment horizontal bracket might be created also;
> + // this takes another `L' event (which might not have caused a new
> + // element on `bracket_stack' yet) and its corresponding `R' event.
> + if (pop_count_ > bracket_stack_.size () + 1)
> ev->origin ()->warning (_ ("do not have that many brackets"));
> }
"which might not have caused a new element"? That sounds like this may
or may not suppress an actually warranted warning.
Correct?
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Colored box behind a single note
Werner LEMBERG writes: > OK, more comments added to the source code. > > I wish that other parts of lilypond are having similarly detailed > comments... Well, that's exactly the reason to be adding them according to Immanuel Kant's seminal writings on coding standards. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Colored box behind a single note
>> diff --git a/lily/horizontal-bracket-engraver.cc
>> b/lily/horizontal-bracket-engraver.cc
>> index 608af35965..a42268a357 100644
>> --- a/lily/horizontal-bracket-engraver.cc
>> +++ b/lily/horizontal-bracket-engraver.cc
>> @@ -56,10 +56,23 @@ Horizontal_bracket_engraver::listen_note_grouping
>> (Stream_event *ev)
>> {
>>Direction d = to_dir (ev->get_property ("span-direction"));
>>
>> + // Allow one single-moment bracket. Abbreviating a horizontal bracket's
>> + // `START' span-direction with `L' and `STOP' with `R', this means that we
>> + // can have
>> + //
>> + // LLL...LLR
>> + //
>> + // or
>> + //
>> + // LRRR...RR
>> + //
>> + // events attached to a single moment (we don't take care of the order of
>> + // `L' and `R' events).
>> +
>>if (d == STOP)
>> {
>>pop_count_++;
>> - if (pop_count_ > bracket_stack_.size ())
>> + if (pop_count_ > bracket_stack_.size () + 1)
>> ev->origin ()->warning (_ ("do not have that many brackets"));
>> }
>>else
>> @@ -68,22 +81,33 @@ Horizontal_bracket_engraver::listen_note_grouping
>> (Stream_event *ev)
>>events_.push_back (ev);
>> }
>>
>> - if (pop_count_ && push_count_)
>> + if (pop_count_ >= 2 && push_count_ >= 2)
>> ev->origin ()->warning (_ ("conflicting note group events"));
>> }
>
> Those number changes do not appear like reflecting some design but
> rather like poking the code until the wanted construct happens not
> to cause warnings or errors. Why increase push/pop counts by 2 when
> admitting one more level? Why allow popping exactly 1 more than
> pushing?
OK, more comments added to the source code.
I wish that other parts of lilypond are having similarly detailed
comments...
Werner
diff --git a/lily/horizontal-bracket-engraver.cc b/lily/horizontal-bracket-engraver.cc
index 608af35965..0889ae10af 100644
--- a/lily/horizontal-bracket-engraver.cc
+++ b/lily/horizontal-bracket-engraver.cc
@@ -28,6 +28,35 @@
#include "translator.icc"
+// In the following (simplified) description of the algorithm, we abbreviate
+// a horizontal bracket's `START' and `STOP' span-direction with `L' and
+// `R', respectively.
+//
+// For a given moment,
+//
+// (1) count the number of `L' and `R' events; they are also pushed onto a
+// stack so that they can be referenced later on (method
+// `listen_note_grouping'),
+//
+// (2) create a HorizontalBracket spanner for every `L' event, link it as
+// necessary, and push it onto `bracket_stack' (method `process_music'),
+//
+// (3) set one boundary for every element of `bracket_stack' (method
+// `acknowledge_note_column'),
+//
+// (4) check whether there is a single `R' event within a bunch of `L' events
+// (or a single `L' event within a bunch of `R' events) and set the
+// other boundary with the same values to create a single-moment
+// horizontal bracket for this case (method `acknowledge_note_column'),
+//
+// (5) pop an element off `bracket_stack' for every `R' event; ignore
+// excessive `L' and `R' events (method `stop_translation_timestep').
+//
+// The above algorithm allows for nested horizontal brackets.
+//
+// TODO: Allow overlapping horizontal brackets (issue #5240). This would
+// need a complete rewrite of the algorithm, however.
+
class Horizontal_bracket_engraver : public Engraver
{
public:
@@ -56,10 +85,27 @@ Horizontal_bracket_engraver::listen_note_grouping (Stream_event *ev)
{
Direction d = to_dir (ev->get_property ("span-direction"));
+ // Allow one single-moment bracket. This means that we can have
+ //
+ // LLL...LLR
+ //
+ // or
+ //
+ // LRRR...RR .
+ //
+ // Note that the order of events doesn't matter; in other words,
+ // `LLLRLLL...LLL' is valid, too.
+
if (d == STOP)
{
pop_count_++;
- if (pop_count_ > bracket_stack_.size ())
+
+ // Since N `L' events create N HorizontalBracket grobs we need (at
+ // most) N `R' events to finish them. However, at this particular
+ // moment, a single-moment horizontal bracket might be created also;
+ // this takes another `L' event (which might not have caused a new
+ // element on `bracket_stack' yet) and its corresponding `R' event.
+ if (pop_count_ > bracket_stack_.size () + 1)
ev->origin ()->warning (_ ("do not have that many brackets"));
}
else
@@ -68,22 +114,41 @@ Horizontal_bracket_engraver::listen_note_grouping (Stream_event *ev)
events_.push_back (ev);
}
- if (pop_count_ && push_count_)
+ // The handled number of events are
+ //
+ // LLL...LLR => push_count_ >= 1 && pop_count_ == 1
+ //
+ // or
+ //
+ // RRR...RRL => pop_count_ >= 1 && push_count_ == 1 .
+ //
+ if (pop_count_ >= 2 && push_count_ >= 2)
ev->origin ()->warning (_ ("conflicting note group events"));
}
void
Horizontal_bracket_engraver::acknowledge_note_column (Grob_info gi)
{
+ bool process_single_moment_br
Re: Colored box behind a single note
Werner LEMBERG writes:
> [Moving the discussion to lilypond-devel.]
>
>>> A loop?
>
> Attached a new version of the patch, now using a loop.
>
>
> Werner
>
> diff --git a/lily/horizontal-bracket-engraver.cc
> b/lily/horizontal-bracket-engraver.cc
> index 608af35965..a42268a357 100644
> --- a/lily/horizontal-bracket-engraver.cc
> +++ b/lily/horizontal-bracket-engraver.cc
> @@ -56,10 +56,23 @@ Horizontal_bracket_engraver::listen_note_grouping
> (Stream_event *ev)
> {
>Direction d = to_dir (ev->get_property ("span-direction"));
>
> + // Allow one single-moment bracket. Abbreviating a horizontal bracket's
> + // `START' span-direction with `L' and `STOP' with `R', this means that we
> + // can have
> + //
> + // LLL...LLR
> + //
> + // or
> + //
> + // LRRR...RR
> + //
> + // events attached to a single moment (we don't take care of the order of
> + // `L' and `R' events).
> +
>if (d == STOP)
> {
>pop_count_++;
> - if (pop_count_ > bracket_stack_.size ())
> + if (pop_count_ > bracket_stack_.size () + 1)
> ev->origin ()->warning (_ ("do not have that many brackets"));
> }
>else
> @@ -68,22 +81,33 @@ Horizontal_bracket_engraver::listen_note_grouping
> (Stream_event *ev)
>events_.push_back (ev);
> }
>
> - if (pop_count_ && push_count_)
> + if (pop_count_ >= 2 && push_count_ >= 2)
> ev->origin ()->warning (_ ("conflicting note group events"));
> }
Those number changes do not appear like reflecting some design but
rather like poking the code until the wanted construct happens not to
cause warnings or errors. Why increase push/pop counts by 2 when
admitting one more level? Why allow popping exactly 1 more than
pushing?
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Colored box behind a single note
[Moving the discussion to lilypond-devel.]
>> A loop?
Attached a new version of the patch, now using a loop.
Werner
diff --git a/lily/horizontal-bracket-engraver.cc b/lily/horizontal-bracket-engraver.cc
index 608af35965..a42268a357 100644
--- a/lily/horizontal-bracket-engraver.cc
+++ b/lily/horizontal-bracket-engraver.cc
@@ -56,10 +56,23 @@ Horizontal_bracket_engraver::listen_note_grouping (Stream_event *ev)
{
Direction d = to_dir (ev->get_property ("span-direction"));
+ // Allow one single-moment bracket. Abbreviating a horizontal bracket's
+ // `START' span-direction with `L' and `STOP' with `R', this means that we
+ // can have
+ //
+ // LLL...LLR
+ //
+ // or
+ //
+ // LRRR...RR
+ //
+ // events attached to a single moment (we don't take care of the order of
+ // `L' and `R' events).
+
if (d == STOP)
{
pop_count_++;
- if (pop_count_ > bracket_stack_.size ())
+ if (pop_count_ > bracket_stack_.size () + 1)
ev->origin ()->warning (_ ("do not have that many brackets"));
}
else
@@ -68,22 +81,33 @@ Horizontal_bracket_engraver::listen_note_grouping (Stream_event *ev)
events_.push_back (ev);
}
- if (pop_count_ && push_count_)
+ if (pop_count_ >= 2 && push_count_ >= 2)
ev->origin ()->warning (_ ("conflicting note group events"));
}
void
Horizontal_bracket_engraver::acknowledge_note_column (Grob_info gi)
{
+ bool process_single_moment_bracket = pop_count_ && push_count_;
+
for (vsize i = 0; i < bracket_stack_.size (); i++)
{
- Side_position_interface::add_support (bracket_stack_[i], gi.grob ());
- Pointer_group_interface::add_grob (bracket_stack_[i],
- ly_symbol2scm ("columns"), gi.grob ());
- add_bound_item (bracket_stack_[i],
- gi.grob ());
- add_bound_item (text_stack_[i],
- gi.grob ());
+ // For a single-moment horizontal bracket (which is the most recently
+ // pushed element on the stack), use the current note column for both
+ // the left and right bound of the bracket.
+ int count = (process_single_moment_bracket
+ && i + 1 == bracket_stack_.size ()) ? 2 : 1;
+
+ for (int j = 0; j < count; j++)
+{
+ Side_position_interface::add_support (bracket_stack_[i],
+gi.grob ());
+ Pointer_group_interface::add_grob (bracket_stack_[i],
+ ly_symbol2scm ("columns"),
+ gi.grob ());
+ add_bound_item (bracket_stack_[i], gi.grob ());
+ add_bound_item (text_stack_[i], gi.grob ());
+}
}
}
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
