Changing subproperties with grob-set-property (working on bug #40)

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
While tweaking the tablature example for the documentation,
I remembered my first attempts in optimizing the glissando
behavior. Now I tried to take accidentals into account and
therefore computed a new X coordinate for the right bound.

The syntax

(ly:grob-set-property! grob '(bound-details right X) right-X)

won't work, however. How can I put the new value into the list?
Or is my approach aiming in the wrong direction?

(Attached is the full file)

Thanks in advance

Marc

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Marc Hohl schrieb:
> [...]
> (Attached is the full file)
>
Arrgh! Now it is...

\version "2.13.6"

\paper {
   indent = 0
   ragged-right = ##f
}

#(define (glissando::calc-extra-dy grob)
   (let* ((original (ly:grob-original grob))
          (left-bound (ly:spanner-bound original LEFT))
          (right-bound (ly:spanner-bound original RIGHT))
          (left-pitch (ly:event-property (event-cause left-bound) 'pitch))
          (right-pitch (ly:event-property (event-cause right-bound) 'pitch)))

     (if (and (= (ly:pitch-octave left-pitch) (ly:pitch-octave right-pitch))
              (= (ly:pitch-notename left-pitch) (ly:pitch-notename right-pitch)))
         (- (ly:pitch-alteration right-pitch) (ly:pitch-alteration left-pitch))
         0 )))

#(define (glissando::calc-accidental-X grob)
    (let* ((original (ly:grob-original grob))
           (right-bound (ly:spanner-bound original RIGHT))
           (right-bound-info (ly:line-spanner::calc-right-bound-info grob))
           (right-X (assoc-get 'X right-bound-info 0))
           (accidental (ly:grob-object right-bound 'accidental-grob)))

           (if (ly:grob? accidental)
               (set! right-X (- right-X 30)))

           (ly:grob-set-property! grob '(bound-details right X) right-X)
           (ly:line-spanner::print grob)))


noten = \relative c {
   c4 \glissando cis
   c4   \glissando cis
   c4 \glissando ces
   c4 \glissando ces
   c4 \glissando d \glissando e \glissando f
   c2 \glissando c
    c4 \glissando dis e \glissando ges
}

\score {
    \new Staff { \clef "G_8"
                 \noten \break
                 \override Glissando #'extra-dy = #glissando::calc-extra-dy
                 \override  Glissando #'stencil = #glissando::calc-accidental-X
                 \noten }
}

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
In reply to this post by Marc Hohl
2009/10/31 Marc Hohl <[hidden email]>:

> The syntax
>
> (ly:grob-set-property! grob '(bound-details right X) right-X)
>
> won't work, however. How can I put the new value into the list?

You'll have to do some C++ hacking.  There are two options here (see
the code in grob-scheme.cc):

1) amend ly_grob_set_property_x () to allow lists

2) create a new exported function ly_grob_set_nested_property_x ()

I favour the latter, since it's cleaner to implement: if you amend
ly:grob-set-property! you'll have to use a hybrid type check, since
the property path could be either a symbol or list.

> Or is my approach aiming in the wrong direction?

I think it's fine for a tweak, but a better fix would involve working
directly on the code in line-spanner.cc.

              (set! right-X (- right-X 30)))

You could get a compensatory value automatically by using
ly:grob-extent on the accidental.

Whatever you decide to do, allowing nested properties in
ly:grob-set-property! would be a useful enhancement.

Regards,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:

> 2009/10/31 Marc Hohl <[hidden email]>:
>
>  
>> The syntax
>>
>> (ly:grob-set-property! grob '(bound-details right X) right-X)
>>
>> won't work, however. How can I put the new value into the list?
>>    
>
> You'll have to do some C++ hacking.  There are two options here (see
> the code in grob-scheme.cc):
>
> 1) amend ly_grob_set_property_x () to allow lists
>
> 2) create a new exported function ly_grob_set_nested_property_x ()
>
> I favour the latter, since it's cleaner to implement: if you amend
> ly:grob-set-property! you'll have to use a hybrid type check, since
> the property path could be either a symbol or list.
>  
Ok, I will have a look at the code. I am sure that questions will arise,
because c++ is new for me, but nevertheless ...
>  
>> Or is my approach aiming in the wrong direction?
>>    
>
> I think it's fine for a tweak, but a better fix would involve working
> directly on the code in line-spanner.cc.
>  
Hm, probably.
>               (set! right-X (- right-X 30)))
>
> You could get a compensatory value automatically by using
> ly:grob-extent on the accidental.
>  
Yes, the value was simply a quick test to see whether
my approach will work as I expected, but I didn't work
on the grob-extent, because ly:grob-set-property! failed.

> Whatever you decide to do, allowing nested properties in
> ly:grob-set-property! would be a useful enhancement.
>  
So I concentrate on this, try to solve it and afterwards come back to
the initial
problem. If I follow proposal 2, there will be a
ly:grob-set-nested-property!
function, am I right?

Thanks for your reply!

Marc

> Regards,
> Neil
>
> ---
>
> ----
> Join the Frogs!
>
>
>  


---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Marc Hohl schrieb:

> Neil Puttock schrieb:
>> [...]
>>
>> 2) create a new exported function ly_grob_set_nested_property_x ()
>>
>> I favour the latter, since it's cleaner to implement: if you amend
>> ly:grob-set-property! you'll have to use a hybrid type check, since
>> the property path could be either a symbol or list.
>>  
> Ok, I will have a look at the code. I am sure that questions will arise,
> because c++ is new for me, but nevertheless ...
Staring at the code, I get a faint impression that it would be more
difficult than I expected.
But there is a file lily/nested-property.cc. Are there some functions
that would do the job or
at least something similar to it?

At the moment, I think I understood the meaning of the arguments to
LY_DEFINE:
First comes the c++ name of the function, then the name used in scheme,
then the number of required arguments, the number of optional arguments,
then a zero (?? at least in every function I looked at), then a list of
arguments (...)
and the docstring.

So the header of the definition could be

LY_DEFINE (ly_grob_set_nested_property_x, "ly:grob-set-nested-property!",
       3, 0, 0, (SCM grob, SCM symlist, SCM val),
       "Set nested property @var{symlist} in grob @var{grob} to value
@var{val}.")
{
  Grob *sc = unsmob_grob (grob);
 
  LY_ASSERT_SMOB (Grob, grob, 1);
  LY_ASSERT_TYPE (ly_is_list, symlist, 2);

But that's more or less a guess...

Marc

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/1 Marc Hohl <[hidden email]>:

> Staring at the code, I get a faint impression that it would be more
> difficult than I expected.
> But there is a file lily/nested-property.cc. Are there some functions that
> would do the job or
> at least something similar to it?

There's only one function in the file which will set a nested
property.  If you're still unsure, have a look at
new-dynamic-engraver.cc, which uses this function to set
'bound-details.

>
> At the moment, I think I understood the meaning of the arguments to
> LY_DEFINE:
> First comes the c++ name of the function, then the name used in scheme,
> then the number of required arguments, the number of optional arguments,
> then a zero (?? at least in every function I looked at), then a list of
> arguments (...)
> and the docstring.

You're on the right track. :)

The third number option sets a `rest' argument (see ly_format () for
an example).

> So the header of the definition could be
>
> LY_DEFINE (ly_grob_set_nested_property_x, "ly:grob-set-nested-property!",
>      3, 0, 0, (SCM grob, SCM symlist, SCM val),
>      "Set nested property @var{symlist} in grob @var{grob} to value
> @var{val}.")
> {
>  Grob *sc = unsmob_grob (grob);
>
>  LY_ASSERT_SMOB (Grob, grob, 1);
>  LY_ASSERT_TYPE (ly_is_list, symlist, 2);
>
> But that's more or less a guess...

Looks OK, though it's preferable to use ly_cheap_is_list for
typechecking symlist, since has a lower overhead.

Regards,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:

> 2009/11/1 Marc Hohl <[hidden email]>:
>
>  
>> Staring at the code, I get a faint impression that it would be more
>> difficult than I expected.
>> But there is a file lily/nested-property.cc. Are there some functions that
>> would do the job or
>> at least something similar to it?
>>    
>
> There's only one function in the file which will set a nested
> property.  If you're still unsure, have a look at
> new-dynamic-engraver.cc, which uses this function to set
> 'bound-details.
>  
Ah, thanks.

>  
>> At the moment, I think I understood the meaning of the arguments to
>> LY_DEFINE:
>> First comes the c++ name of the function, then the name used in scheme,
>> then the number of required arguments, the number of optional arguments,
>> then a zero (?? at least in every function I looked at), then a list of
>> arguments (...)
>> and the docstring.
>>    
>
> You're on the right track. :)
>
> The third number option sets a `rest' argument (see ly_format () for
> an example).
>  
Hm, I am not sure whether I understand that completely, but it seems
not to be important in this case.

>  
>> So the header of the definition could be
>>
>> LY_DEFINE (ly_grob_set_nested_property_x, "ly:grob-set-nested-property!",
>>      3, 0, 0, (SCM grob, SCM symlist, SCM val),
>>      "Set nested property @var{symlist} in grob @var{grob} to value
>> @var{val}.")
>> {
>>  Grob *sc = unsmob_grob (grob);
>>
>>  LY_ASSERT_SMOB (Grob, grob, 1);
>>  LY_ASSERT_TYPE (ly_is_list, symlist, 2);
>>
>> But that's more or less a guess...
>>    
>
> Looks OK, though it's preferable to use ly_cheap_is_list for
> typechecking symlist, since has a lower overhead.
>  
Ok, then:

LY_DEFINE (ly_grob_set_nested_property_x, "ly:grob-set-nested-property!",
     3, 0, 0, (SCM grob, SCM symlist, SCM val),
     "Set nested property @var{symlist} in grob @var{grob} to value
@var{val}.")
{
 Grob *sc = unsmob_grob (grob);

 LY_ASSERT_SMOB (Grob, grob, 1);
 LY_ASSERT_TYPE (ly_cheap_is_list, symlist, 2);

 if (!ly_is_procedure (val)
     && !type_check_assignment (symlist, val, ly_symbol2scm
("backend-type?")))
   error ("typecheck failed");

 set_nested_property (sc, scm_list_n (symlist, SCM_UNDEFINED), val);
 return SCM_UNSPECIFIED;
}

(Hm, it still feels like the time beginning with scheme where I stirred
in the fog and put several pieces together in the hope they will work
somehow...).

Marc
 




---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/2 Marc Hohl <[hidden email]>:

> Ok, then:
>
> LY_DEFINE (ly_grob_set_nested_property_x, "ly:grob-set-nested-property!",
>    3, 0, 0, (SCM grob, SCM symlist, SCM val),
>    "Set nested property @var{symlist} in grob @var{grob} to value
> @var{val}.")
> {
> Grob *sc = unsmob_grob (grob);
>
> LY_ASSERT_SMOB (Grob, grob, 1);
> LY_ASSERT_TYPE (ly_cheap_is_list, symlist, 2);

Hmm, now I've had more time to think about this, I'm not sure this is
ideal, since there's no check that the elements of the list are
symbols.  It's probably safer to use SCM_ASSERT_TYPE to brew a custom
assertion for "list of symbols":

bool type_ok = ly_cheap_is_list (symlist);

  if (type_ok)
    for (SCM s = symlist; scm_is_pair (s); s = scm_cdr (s))
      type_ok &= ly_is_symbol (scm_car (s));

  SCM_ASSERT_TYPE (type_ok, symlist, SCM_ARG2, __FUNCTION__, "list of symbols");

>
> if (!ly_is_procedure (val)
>    && !type_check_assignment (symlist, val, ly_symbol2scm
> ("backend-type?")))
>  error ("typecheck failed");

I'm afraid you can't do this on a nested property, since
type_check_assignment () expects a symbol as the first arg.  Though
you could extract the main symbol (e.g., 'bound-details) using scm_car
(), the type check would still fail due to a mismatch between the
property type (list?) and the val (which would be a number in the case
of your snippet above).

> set_nested_property (sc, scm_list_n (symlist, SCM_UNDEFINED), val);

Since symlist is already a SCM list, you don't need to use scm_list_n here:

set_nested_property (sc, symlist, val);

> (Hm, it still feels like the time beginning with scheme where I stirred
> in the fog and put several pieces together in the hope they will work
> somehow...).

Don't worry, it'll soon make perfect sense, though if you're starting
to get cold feet, you could try implementing this in Scheme instead.

Regards,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:

> 2009/11/2 Marc Hohl <[hidden email]>:
>
>  
>> Ok, then:
>>
>> LY_DEFINE (ly_grob_set_nested_property_x, "ly:grob-set-nested-property!",
>>    3, 0, 0, (SCM grob, SCM symlist, SCM val),
>>    "Set nested property @var{symlist} in grob @var{grob} to value
>> @var{val}.")
>> {
>> Grob *sc = unsmob_grob (grob);
>>
>> LY_ASSERT_SMOB (Grob, grob, 1);
>> LY_ASSERT_TYPE (ly_cheap_is_list, symlist, 2);
>>    
>
> Hmm, now I've had more time to think about this, I'm not sure this is
> ideal, since there's no check that the elements of the list are
> symbols.  It's probably safer to use SCM_ASSERT_TYPE to brew a custom
> assertion for "list of symbols":
>
> bool type_ok = ly_cheap_is_list (symlist);
>
>   if (type_ok)
>     for (SCM s = symlist; scm_is_pair (s); s = scm_cdr (s))
>       type_ok &= ly_is_symbol (scm_car (s));
>
>   SCM_ASSERT_TYPE (type_ok, symlist, SCM_ARG2, __FUNCTION__, "list of symbols");
>
>  
Ok.

>> if (!ly_is_procedure (val)
>>    && !type_check_assignment (symlist, val, ly_symbol2scm
>> ("backend-type?")))
>>  error ("typecheck failed");
>>    
>
> I'm afraid you can't do this on a nested property, since
> type_check_assignment () expects a symbol as the first arg.  Though
> you could extract the main symbol (e.g., 'bound-details) using scm_car
> (), the type check would still fail due to a mismatch between the
> property type (list?) and the val (which would be a number in the case
> of your snippet above).
>
>  
I just copied this part from the definition of ly:grob-set-property!
and pasted it (as I never stop to mention: this is all very unfamiliar
to me,
so it just looked ok). Is there an alternative for this check, or should
I simply
drop it completely?
>> set_nested_property (sc, scm_list_n (symlist, SCM_UNDEFINED), val);
>>    
>
> Since symlist is already a SCM list, you don't need to use scm_list_n here:
>
> set_nested_property (sc, symlist, val);
>
>  
Ah, of course. I was so overwhelmed by the fact that I found out where
scm_list_n is
defined and what it does (in new-dynamic.engraver.cc is a call for
scm_list_3, so grepping
through the code showed that there exists an _n variant, and so on),
that I simply overlooked this fact.

>> (Hm, it still feels like the time beginning with scheme where I stirred
>> in the fog and put several pieces together in the hope they will work
>> somehow...).
>>    
>
> Don't worry, it'll soon make perfect sense, though if you're starting
> to get cold feet, you could try implementing this in Scheme instead.
>
>  
I am not sure whether this would be a better solution; I am learning
slowly, but I think that sooner or later I have to dive into the c++ code,
so if you don't loose your patience with me, I'll follow this task to
whatever end
(I should stop watching LOTR, it tends to bring too much pathos into my
mails ;-)

Marc

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/3 Marc Hohl <[hidden email]>:

> I just copied this part from the definition of ly:grob-set-property!
> and pasted it (as I never stop to mention: this is all very unfamiliar to
> me,
> so it just looked ok).

I hope you're compiling between changes though, otherwise you won't
know whether it's working.

> Is there an alternative for this check, or should I
> simply
> drop it completely?

You'll have to remove it.

I don't think it matters too much, since you've already ensured the
property path is a list.

> I am not sure whether this would be a better solution; I am learning
> slowly, but I think that sooner or later I have to dive into the c++ code,
> so if you don't loose your patience with me, I'll follow this task to
> whatever end

OK, though you still might consider it a useful exercise to try a
Scheme implementation too. :)

Regards,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:

> 2009/11/3 Marc Hohl <[hidden email]>:
>
>  
>> I just copied this part from the definition of ly:grob-set-property!
>> and pasted it (as I never stop to mention: this is all very unfamiliar to
>> me,
>> so it just looked ok).
>>    
>
> I hope you're compiling between changes though, otherwise you won't
> know whether it's working.
>  
To be honest: I didn't until now, mainly because I had to work on the new
website for my college of music where I work, and I did some stuff for
tablature.

But now I did, and it works instantly. Wow. Should this go into a patch?

Now I can come back to my earlier problem (the right end of the
glissando line)
and use ly:grob-set-nested-property! as an intermediate workaround
for the glissando. If this works (and it does with a fixed value for X),
then I'll proceed with changing (or at least trying to change)
line-spanner.cc.
But for the latter, I'll go and fetch me some book for learning c++.

Thanks for your support!

Marc

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/4 Marc Hohl <[hidden email]>:

> To be honest: I didn't until now, mainly because I had to work on the new
> website for my college of music where I work, and I did some stuff for
> tablature.

No problem; there's no need to hurry something like this, especially
if you're feeling your way around unfamiliar code.

> But now I did, and it works instantly. Wow. Should this go into a patch?

Of course. :)

> Thanks for your support!

You're welcome.

Regards,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:

> 2009/11/4 Marc Hohl <[hidden email]>:
>
>  
>> To be honest: I didn't until now, mainly because I had to work on the new
>> website for my college of music where I work, and I did some stuff for
>> tablature.
>>    
>
> No problem; there's no need to hurry something like this, especially
> if you're feeling your way around unfamiliar code.
>
>  
>> But now I did, and it works instantly. Wow. Should this go into a patch?
>>    
>
> Of course. :)
>  
Here it is.

Marc

Enhancement-Providing-ly-grob-set-nested-property.patch.gz (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/5 Marc Hohl <[hidden email]>:

> Here it is.

LGTM, just missing a regression test.

Cheers,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:
> 2009/11/5 Marc Hohl <[hidden email]>:
>
>  
>> Here it is.
>>    
>
> LGTM, just missing a regression test.
>  
Thanks! Ok, I try to find a suitable example with nested properties...

Greetings

Marc

> Cheers,
> Neil
>
> ---
>
> ----
> Join the Frogs!
>
>
>  


---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Marc Hohl schrieb:
> Neil Puttock schrieb:
>> 2009/11/5 Marc Hohl <[hidden email]>:
>>
>>  
>>> Here it is.
>>>    
>>
>> LGTM, just missing a regression test.
>>  
Here is a new patch, regression test included.

Ok to apply?

Marc

Enhancement-Providing-ly-grob-set-nested-property.patch.gz (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/6 Marc Hohl <[hidden email]>:

> Ok to apply?

Can you simplify the regression test?  It should be similar in length
and scope to a bug report snippet.

I suggest you remove most of the notes, and simplify the callback so
it only tests the new functionality: if you use 'after-line-breaking,
you can remove all the extra code, thus placing the focus on the
exported function only.

Thanks,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:
> 2009/11/6 Marc Hohl <[hidden email]>:
>
>  
>> Ok to apply?
>>    
>
> Can you simplify the regression test?  It should be similar in length
> and scope to a bug report snippet.
>  
Yes, of course. The additional notes provided a better spacing, but I
cut it down in length.
> I suggest you remove most of the notes, and simplify the callback so
> it only tests the new functionality: if you use 'after-line-breaking,
> you can remove all the extra code, thus placing the focus on the
> exported function only.
>  
I hope I understood you correctly - I was only able to remove the
ly:line-spanner::print callback,
i.e. one line of code, but I think it is clearer now.

Greetings,

Marc

> Thanks,
> Neil
>
> ---
>
> ----
> Join the Frogs!
>
>
>  


Enhancement-Providing-ly-grob-set-nested-property.patch.gz (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Neil Puttock
2009/11/9 Marc Hohl <[hidden email]>:

> Yes, of course. The additional notes provided a better spacing, but I cut it
> down in length.

If you use semibreves, the glissando will be long enough, and one
\glissando will suffice to test the property setting.

> I hope I understood you correctly - I was only able to remove the
> ly:line-spanner::print callback,
> i.e. one line of code, but I think it is clearer now.

These lines get in the way of testing the nested property:

+  (let* ((original (ly:grob-original grob))
+         (right-bound (ly:spanner-bound original RIGHT))
+         (right-pitch (ly:event-property (event-cause right-bound) 'pitch)))
+
+        (if (= (ly:pitch-semitones right-pitch) 11) ;; middle b

It's obvious if the test fails, so you should be able to boil it down
to the barest minimum:

\relative c' {
  \override Glissando #'after-line-breaking =
    #(lambda (grob)
       (ly:grob-set-nested-property! grob '(bound-details right Y) 3))
  c1 \glissando
  d1
}

Regards,
Neil

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Changing subproperties with grob-set-property (working on bug #40)

Marc Hohl
Neil Puttock schrieb:
> [...]
> These lines get in the way of testing the nested property:
> [...]
Sorry for not seeing the obvious; now I understand what you meant.

Here's the corrected patch.

Marc


Enhancement-Providing-ly-grob-set-nested-property.patch.gz (1K) Download Attachment
12