Issue 348120043: Allow scripts to be defined either as glyphs or stencils. (Closed)
|
Created:
5 years, 2 months ago by Valentin Villenave Modified:
5 years, 1 month ago Reviewers:
dak, rap.ardiputra127 CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAllow scripts to be defined either as glyphs or stencils.
This feature has been discussed several times over the years;
this patch aims to lift the limitation that Script grobs are
only to be defined as Feta glyphs rather than glyphs from any
font (assuming their naming closely follows that of Feta), or
any Stencil definition. (See regtest for a demonstration;
proper doc to follow if this feature gets accepted.)
Patch Set 1 #
Total comments: 22
Patch Set 2 : Simplify. (but still use pair-or-stencil? for now) #
MessagesTotal messages: 11
https://codereview.appspot.com/348120043/diff/1/input/regression/script-stenc... File input/regression/script-stencil.ly (right): https://codereview.appspot.com/348120043/diff/1/input/regression/script-stenc... input/regression/script-stencil.ly:31: #(append! default-script-alist No. Don't change any system data structures destructively or the changes will persist across multiple files specified on the command line. https://codereview.appspot.com/348120043/diff/1/input/regression/script-stenc... input/regression/script-stencil.ly:43: #(assoc-set! Same here. https://codereview.appspot.com/348120043/diff/1/lily/include/lily-imports.hh File lily/include/lily-imports.hh (right): https://codereview.appspot.com/348120043/diff/1/lily/include/lily-imports.hh#... lily/include/lily-imports.hh:86: extern Variable ly_stencil_p; Unnecessary in C++. Use unsmob<Stencil> instead. https://codereview.appspot.com/348120043/diff/1/lily/lily-imports.cc File lily/lily-imports.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/lily-imports.cc#newcode80 lily/lily-imports.cc:80: Variable ly_stencil_p ("ly:stencil?"); See above. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc File lily/script-interface.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:32: #include "lily-imports.hh" lily-imports.h should no longer be needed. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:36: Script_interface::get_glyph_or_stencil (Grob *me, Direction d) There is no point in renaming this into get_glyph_or_stencil if it cannot return anything but a stencil. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:59: if (to_boolean (Lily::ly_stencil_p (s))) That's rubbish. Should be something like if (Stencil *stil = unsmob<Stencil> (s)) return *stil; Actually, should likely be "else if" since there is no point in checking again in case we ended with a programming_error above. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:144: return get_glyph_or_stencil (me, dir).smobbed_copy (); See above. https://codereview.appspot.com/348120043/diff/1/scm/c++.scm File scm/c++.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/c++.scm#newcode47 scm/c++.scm:47: (define-public (pair-or-stencil? x) Seems like "pair" is a stand-in for something more specific. While the predicate previously was just pair? likely for reasons of laziness, in combination with stencil? it seems like something more specific would be decidedly less awkward. https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.sc... scm/define-grob-properties.scm:1446: (script-stencil ,pair-or-stencil? "A pair @code{(@var{type} . @var{arg})} which It seems to me like it would make more sense to leave script-stencil as it is and instead put a callback into stencil that interprets it. Then whatever uses script-stencil now can instead get stencil (and thus read and interpret script-stencil via the callback). Then you can just override stencil if you want to specify a specific stencil. Am I overlooking something here?
Sign in to reply to this message.
Thanks David! You make valid points, I just have a couple of doubts below. https://codereview.appspot.com/348120043/diff/1/input/regression/script-stenc... File input/regression/script-stencil.ly (right): https://codereview.appspot.com/348120043/diff/1/input/regression/script-stenc... input/regression/script-stencil.ly:31: #(append! default-script-alist On 2019/02/22 16:19:53, dak wrote: > No. Don't change any system data structures destructively or the changes will > persist across multiple files specified on the command line. Done. https://codereview.appspot.com/348120043/diff/1/input/regression/script-stenc... input/regression/script-stencil.ly:43: #(assoc-set! On 2019/02/22 16:19:53, dak wrote: > Same here. Done. https://codereview.appspot.com/348120043/diff/1/lily/include/lily-imports.hh File lily/include/lily-imports.hh (right): https://codereview.appspot.com/348120043/diff/1/lily/include/lily-imports.hh#... lily/include/lily-imports.hh:86: extern Variable ly_stencil_p; On 2019/02/22 16:19:53, dak wrote: > Unnecessary in C++. Use unsmob<Stencil> instead. Done. https://codereview.appspot.com/348120043/diff/1/lily/lily-imports.cc File lily/lily-imports.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/lily-imports.cc#newcode80 lily/lily-imports.cc:80: Variable ly_stencil_p ("ly:stencil?"); On 2019/02/22 16:19:53, dak wrote: > See above. Done. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc File lily/script-interface.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:32: #include "lily-imports.hh" On 2019/02/22 16:19:53, dak wrote: > lily-imports.h should no longer be needed. Done. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:36: Script_interface::get_glyph_or_stencil (Grob *me, Direction d) On 2019/02/22 16:19:53, dak wrote: > There is no point in renaming this into get_glyph_or_stencil if it cannot return > anything but a stencil. The point was to avoid any risk of confusion with Grob::get_stencil (). If you find it unnecessary, I’ll remove it. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:59: if (to_boolean (Lily::ly_stencil_p (s))) On 2019/02/22 16:19:53, dak wrote: > Should be something like > if (Stencil *stil = unsmob<Stencil> (s)) > return *stil; Done. > Actually, should likely be "else if" since there is no point in checking again > in case we ended with a programming_error above. Well, that’s a problem because most stencil expressions do syntactically satisfy the scm_is_pair test above. So all I could come up with was to let the test unfold in case it’s a valid glyph lookup, and only then test for a possible Stencil smob. https://codereview.appspot.com/348120043/diff/1/scm/c++.scm File scm/c++.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/c++.scm#newcode47 scm/c++.scm:47: (define-public (pair-or-stencil? x) On 2019/02/22 16:19:53, dak wrote: > Seems like "pair" is a stand-in for something more specific. While the > predicate previously was just pair? likely for reasons of laziness, in > combination with stencil? it seems like something more specific would be > decidedly less awkward. There are several awkward things here. To wit, (feta . ("upglyph" . "downglyph")) where I’d much rather see "feta" as a quoted string. But I didn’t want to change that in order to make my patch as little disruptive as possible. https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.sc... scm/define-grob-properties.scm:1446: (script-stencil ,pair-or-stencil? "A pair @code{(@var{type} . @var{arg})} which On 2019/02/22 16:19:53, dak wrote: > It seems to me like it would make more sense to leave script-stencil as it is > and instead put a callback into stencil that interprets it. Are you referring to the Script.stencil property? That wouldn’t be of much use since this property affects all Script objects whereas script-stencil definitions allow to define many distinct articulations. So your callback would have to refer to an alist to look up appropriate glyphs/stencils anyway. > Then you can just override stencil if you > want to specify a specific stencil. That’s already what we’re doing right now, no change needed whatsoever. But it’s a rather clunky hack, and doesn’t integrate well with the alist definitions. (But I may be the one missing something here…)
Sign in to reply to this message.
https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc File lily/script-interface.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:36: Script_interface::get_glyph_or_stencil (Grob *me, Direction d) On 2019/02/22 20:41:08, Valentin Villenave wrote: > On 2019/02/22 16:19:53, dak wrote: > > There is no point in renaming this into get_glyph_or_stencil if it cannot > return > > anything but a stencil. > > The point was to avoid any risk of confusion with Grob::get_stencil (). If you > find it unnecessary, I’ll remove it. Where would the risk for confusion lie? https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newc... lily/script-interface.cc:59: if (to_boolean (Lily::ly_stencil_p (s))) On 2019/02/22 20:41:08, Valentin Villenave wrote: > On 2019/02/22 16:19:53, dak wrote: > > Should be something like > > if (Stencil *stil = unsmob<Stencil> (s)) > > return *stil; > > Done. > > > Actually, should likely be "else if" since there is no point in checking again > > in case we ended with a programming_error above. > > Well, that’s a problem because most stencil expressions do syntactically satisfy > the scm_is_pair test above. So all I could come up with was to let the test > unfold in case it’s a valid glyph lookup, and only then test for a possible > Stencil smob. You are confusing stencils with stencil expressions. Stencils satisfy ly:stencil? and you can extract their stencil expression (which usually is a pair) with ly:stencil-expr . https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.sc... scm/define-grob-properties.scm:1446: (script-stencil ,pair-or-stencil? "A pair @code{(@var{type} . @var{arg})} which On 2019/02/22 20:41:08, Valentin Villenave wrote: > On 2019/02/22 16:19:53, dak wrote: > > It seems to me like it would make more sense to leave script-stencil as it is > > and instead put a callback into stencil that interprets it. > > Are you referring to the Script.stencil property? That wouldn’t be of much use > since this property affects all Script objects whereas script-stencil > definitions allow to define many distinct articulations. So your callback would > have to refer to an alist to look up appropriate glyphs/stencils anyway. > > > Then you can just override stencil if you > > want to specify a specific stencil. > > That’s already what we’re doing right now, no change needed whatsoever. But > it’s a rather clunky hack, and doesn’t integrate well with the alist > definitions. (But I may be the one missing something here…) I don't understand where the difference in what you do with script-stencil as a property and what you would do with stencil as a property would lie.
Sign in to reply to this message.
Simplify. (but still use pair-or-stencil? for now)
Sign in to reply to this message.
On 2019/02/22 21:00:08, dak wrote:
> You are confusing stencils with stencil expressions. Stencils satisfy
> ly:stencil? and you can extract their stencil expression (which usually is a
> pair) with ly:stencil-expr .
Yes, but what we’re dealing with in this function is a stencil expression, isn’t
it? (At least, as long as it isn’t unsmobbed.)
If I do
if (scm_is_pair (s) {} else *unsmob<Stencil> (s)
the stencil expression in "s" will never reach the "else" subroutine.
> I don't understand where the difference in what you do with script-stencil as
a
> property and what you would do with stencil as a property would lie.
Forgive me for not understanding what you mean right away. Right now,
Script.stencil is defined as ly:script-interface::print which is where glyphs
are looked up (or, with this new patch, stencil expressions), then printed. Are
you suggesting that we scrap this interface and make Script.stencil point to a
Scheme callback instead?
V⋅.
Sign in to reply to this message.
On 2019/02/22 21:51:39, Valentin Villenave wrote:
> On 2019/02/22 21:00:08, dak wrote:
> > You are confusing stencils with stencil expressions. Stencils satisfy
> > ly:stencil? and you can extract their stencil expression (which usually is a
> > pair) with ly:stencil-expr .
>
> Yes, but what we’re dealing with in this function is a stencil expression,
isn’t
> it? (At least, as long as it isn’t unsmobbed.)
There is some fundamental confusion here. A stencil expression is a list. It
never satisfies the ly:stencil? predicate and always satisfies the pair?
predicate. Calling unsmob<Stencil> on its SCM representation will deliver a 0
pointer. It is not a smob. A Stencil is a smob. It will satisfy the
ly:stencil? predicate and never will satisfy the pair? predicate. Calling
unsmob<Stencil> on its SCM representation will deliver a valid Stencil * .
Calling ly:stencil-expr on it will deliver its stencil expression. A Stencil
type contains both a stencil expression and a bounding box. Those are basically
indendent components of a Stencil .
> If I do
>
> if (scm_is_pair (s) {} else *unsmob<Stencil> (s)
>
> the stencil expression in "s" will never reach the "else" subroutine.
Wrong.
> > I don't understand where the difference in what you do with script-stencil
as
> a
> > property and what you would do with stencil as a property would lie.
>
> Forgive me for not understanding what you mean right away. Right now,
> Script.stencil is defined as ly:script-interface::print which is where glyphs
> are looked up (or, with this new patch, stencil expressions), then printed.
Are
> you suggesting that we scrap this interface and make Script.stencil point to a
> Scheme callback instead?
ly:script-interface::print is a Scheme callback (it may or may not be
implemented in C++ but that's not really relevant). What does overriding
script-stencil buy you that overriding stencil would not equally well do? I
want to understand what this achieves in a better way than what we have already.
Sign in to reply to this message.
Sign in to reply to this message.
On 2019/02/22 22:11:58, dak wrote: > There is some fundamental confusion here. A stencil expression is a list. It > never satisfies the ly:stencil? predicate and always satisfies the pair? > predicate. Oh, I see. I mixed up "unsmob" and "eval": a stencil expression becomes a Stencil when it’s eval’d, not unsmobbed. > A Stencil > type contains both a stencil expression and a bounding box. Indeed: I wondered if I had to create a Box object with appropriate X and y extents, but I realized that the stencil expression does that on its own when evaluated. (At least it’s my understanding.) > ly:script-interface::print is a Scheme callback (it may or may not be > implemented in C++ but that's not really relevant). What does overriding > script-stencil buy you that overriding stencil would not equally well do? I > want to understand what this achieves in a better way than what we have already. I’m still struggling to understand; are you referring to the stencil property of the grob? (Which, since we don’t have ScriptStaccato or ScriptMarcato defined as different grobs, would affect all Script objects regardless of their individual specifics?) «What we have» is convenient, as it allows for several Script objects to be defined as an alist, each one with different properties (e.g. avoid-slur, script-prio etc.) and a specific Feta glyph (even allowing for different glyphs depending on direction). That glyph lookup is defined as the script-stencil property, which effectively replaces the Script grob’s stencil (via the script-interface::print callback). (Please correct me if I’m wrong.) As I was aiming for the less disruptive change possible, I suggested that this script-stencil property could also be defined as a stencil expression (in which case there’s no font glyph lookup anymore), but preserving every other part of the current logic. I initially thought about adding an additional property (something maybe called "script-stencil-formatter", that would allow users to _both_ use a specific glyph _and_ throw in a (lambda (stil) (ly:stencil-scale stil)) function, for example. Then I decided against it. The current mechanism relies on the script-stencil property, in a rather inflexible way: see for example the hardcoded "feta" reference in script-interface.cc; I’m merely trying to make it a bit more versatile by allowing users to define new Script objects (or redefine existing ones) as stencil expressions (not just Feta glyphs), _within_ the alist-based structure we already offer (thus keeping the ability to fine-tune all the other properties available individually for each articulation type)… And within that alist, the relevant property happens to be named script-stencil, not just stencil. So I thought that’s where I’d look. V.
Sign in to reply to this message.
v.villenave@gmail.com writes: > On 2019/02/22 22:11:58, dak wrote: >> There is some fundamental confusion here. A stencil expression is a > list. It >> never satisfies the ly:stencil? predicate and always satisfies the > pair? >> predicate. > > Oh, I see. I mixed up "unsmob" and "eval": a stencil expression becomes > a Stencil when it’s eval’d, not unsmobbed. No, it doesn't. There is ly:interpret-stencil-expression for tracing a stencil expression via a generic callback function but that's for interpreting stencils when drawing, not for creating stencils. For creating a stencil from a bounding box and a stencil expression there is ly:make-stencil . Also there are various callbacks for creating outlines from stencils. >> A Stencil >> type contains both a stencil expression and a bounding box. > > Indeed: I wondered if I had to create a Box object with appropriate X > and y extents, but I realized that the stencil expression does that on > its own when evaluated. (At least it’s my understanding.) No, you use ly:make-stencil to combine a stencil expression with a bounding box. >> ly:script-interface::print is a Scheme callback (it may or may not be >> implemented in C++ but that's not really relevant). What does >> overriding script-stencil buy you that overriding stencil would not >> equally well do? I want to understand what this achieves in a better >> way than what we have already. > > I’m still struggling to understand; are you referring to the stencil > property of the grob? Yes? > (Which, since we don’t have ScriptStaccato or ScriptMarcato defined as > different grobs, would affect all Script objects regardless of their > individual specifics?) Sure, but you are trying to override stencils, aren't you? Why do you even place an entry for "script-stencil" then instead of just placing an entry for "stencil"? > «What we have» is convenient, as it allows for several Script objects > to be defined as an alist, each one with different properties (e.g. > avoid-slur, script-prio etc.) and a specific Feta glyph (even allowing > for different glyphs depending on direction). That glyph lookup is > defined as the script-stencil property, which effectively replaces the > Script grob’s stencil (via the script-interface::print callback). > (Please correct me if I’m wrong.) But you are free to have stencil rather than script-stencil replaced, aren't you? > As I was aiming for the less disruptive change possible, I suggested > that this script-stencil property could also be defined as a stencil > expression (in which case there’s no font glyph lookup anymore), but > preserving every other part of the current logic. What is there that you cannot put into stencil instead? > The current mechanism relies on the script-stencil property, in a rather > inflexible way: So why do you use script-stencil rather than stencil? > see for example the hardcoded "feta" reference in script-interface.cc; > I’m merely trying to make it a bit more versatile by allowing users to > define new Script objects (or redefine existing ones) as stencil > expressions (not just Feta glyphs), _within_ the alist-based structure > we already offer (thus keeping the ability to fine-tune all the other > properties available individually for each articulation type)… And > within that alist, the relevant property happens to be named > script-stencil, not just stencil. That alist just contains a list of properties to override. It's your decision whether it overrides script-stencil or stencil. -- David Kastrup
Sign in to reply to this message.
On 2019/02/23 22:40:44, dak wrote: > Sure, but you are trying to override stencils, aren't you? Why do you > even place an entry for "script-stencil" then instead of just placing an > entry for "stencil"? Oh. I’ve been an idiot from the start: it simply never occurred to me that the script-alist definitions could include a straightforward stencil property (I thought script-stencil would take precedence). So the problem becomes a lot simpler now; users that want a new or modified articulation script (which there have been quite a few of over the years) may just do something like that: http://lsr.di.unimi.it/LSR/Snippet?id=1087 and be done with it. I’m still bothered by the way script-interface.cc is written, specifically the hardcoded "feta" reference which I tried to address; do you have any thoughts on that? (Other than that, it appears there’s very little to salvage from my patch :-) V.
Sign in to reply to this message.
On 2019/02/24 08:48:22, Valentin Villenave wrote: > I’m still bothered by the way script-interface.cc is written, specifically the > hardcoded "feta" reference which I tried to address; do you have any thoughts on > that? (Other than that, it appears there’s very little to salvage from my patch > :-) 'feta here is nothing other than a particular character lookup scheme to be used on the other details of the entry. If you have suggestions for other, possibly more general useful lookup schemes, they can be added into the get_stencil procedure. It would even be possible to use a 'stencil lookup scheme that would buy nothing over just using the stencil callback other than selection of different stencils for up/down. Since you could do that kind of decision equally well in a stencil callback, that would be quite redundant, however. The ability to just override the stencil callback renders every such addition redundant but since the character lookup scheme flag is there, one might as well use it to allow more than a single choice.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
