Re: First attempt at new accidentals in git

[Han-Wen Nienhuys]

2007/11/11, Rune Zedeler <address@hidden>:
> I have put the first attempt at moving accidentals to scheme in dev/rune
> branch.
> (Please check that VERSION is 2.11.32.rz3 - I have had some git-problems)
>
> * Split pitch into pitchclass and pitch - pitchclass not containing
> octave information.
> Han-Wen please comment on code. I had some problems with getting guile
> to accept  hierarchical smobs.
>
what's

    midt i at ændre pitch og tilføje pitchclass

first round of review; focus on pitch representation:


> --- a/lily/context-def.cc
> +++ b/lily/context-def.cc
> @@ -223,13 +223,16 @@ Context_def::path_to_acceptable_context (SCM type_sym, 
> Output_def *odef) const
>      {
>        Context_def *g = accepteds[i];
>
> -      vector<Context_def*> result
> -     = g->path_to_acceptable_context (type_sym, odef);
> -      if (result.size () && result.size () < best_depth)
> +      if (g!=this)

style: spaces around !=

I think this was discussed on the list before that this only handles
simple minded cycles? If yes, please add a comment.


> --- a/lily/include/key-entry.hh
> +++ b/lily/include/key-entry.hh
> @@ -13,31 +13,34 @@
>  #include "smobs.hh"
>  #include "moment.hh"
>  #include "rational.hh"
> +#include "pitch.hh"
> +
> +/*
> +FIXME!!!
> +Does it work with simple smobs when it contains a reference to a pitchclass_?
> +*/

Yes, however, you must make sure that the Pitchclass* does not come
from a SCM, ie.

  pitchclass_ = unsmob_pitchclass (scm_obj)  // wrong, pitchclass_
  // may be deleted if scm_obj becomes unreachable

  pitchclass_ = new Pitchclass (*unsmob_pitchclass (scm_obj))  // wrong

at the end in ~Key_entry() you need to delete pitchclass_. Also, don't
forget copy ctor / = operator(), if applicable. If not, please make
copy ctor private.

>  /** A "tonal" pitch. This is a pitch used in diatonal western music
> @@ -22,71 +23,53 @@
>      alteration).
>
>  */
> -class Pitch
> +class Pitch : public Pitchclass
>  {
>  private:
>    int octave_;
>    Pitch negated () const;
>    string to_string () const;
> +  SCM virtual_smobbed_copy () { // AAARGH!
> +    return smobbed_copy ();
> +  }

>    DECLARE_SIMPLE_SMOBS (Pitch);

This looks bad. You have to remove Pitch as a smobtype, and then
create an unsmob_pitch, which does

  dynamic_cast<Pitch*> (unsmob_pitchclass (obj))

Then, double check that all relevant functions distinguish between
unsmob_pitch()  and unsmob_pitchclass() correctly.

You should also modify the print routine for the Pitch(class) smob so that it
will print

 #<Pitch c''>

vs

 #<Pitchclass c>

depending on the type.

> @@ -0,0 +1,79 @@
> +#ifndef MUSICAL_PITCHCLASS_HH

PITCHCLASS_HH

> +
> +INSTANTIATE_COMPARE (Pitchclass, Pitchclass::compare);

that puzzles me: how can you compare pitchclasses, which are
essentially cyclic?

>  Key_entry::Key_entry (int n, Rational a)
>  Key_entry::Key_entry (int n, Rational a, int o, int b, Moment mp)
>  Key_entry::Key_entry ()

I dislike this. I prefer a default ctor, and then use set_XX methods
to set appropriate members. This is a lot of repetitive code.

> +Key_entry::~Key_entry ()
> +{
> +  // TODO: what here?
> +  //cout << "Killing entry" << endl;
> +
> +}

2 choices:

1.  use an SCM for keeping the Pitchclass reference; mark it in
Key_entry::mark(). Deletion happens automatically. You can't modify it
(the data may be shared.)

2. use a C++ pointer for keeping the Pitchclass. It has to be copied
and deleted in the C++ way.



>  {
> -  return notename_ + octave_ * scale_->step_tones_.size ();
> +  return notename_ + octave_ * (int)scale_->step_tones_.size ();

prefer int(xxx)

> +Pitchclass *
> +unsmob_pitch_or_pitchclass (SCM s, int number)
> +{
> +  Pitchclass *pc = unsmob_pitch (s);
> +  if (pc==NULL)

  if (!pc)


> * Moved accidental rules to scheme
> Now the user can write his own accidental rules in the .ly if he is not
> satisfied with the existing ones.
>
> If one for instance should wish to add a cautionary accidental at the
> start of each measure (hmm), then one may do like this:
>
> %%% BEGIN
> \version "2.11.32"
>
> #(define (start-of-measure-acc-rule context pitch barnum measurepos)
>    (cons #f (equal? measurepos (ly:make-moment 0 1))))
>
> mus = {
>    \key des \major
>    \transpose c c' {
>      g16 g g g' g' d' d' d' g' g' f f gis gis g g' ges1
>    }
> }
> <<
>    \new Staff {
>      \set Staff.autoCautionaries = #(list 'Staff start-of-measure-acc-rule)
>      \relative c' { c4 d e f g gis a ais b2 c }
>    }
>  >>
> %%% END
>
> * Added accidental-rules 'neo-modern and 'neo-modern-cautionary - adding
> even more accidentals to the modern accidentals. Notes different from
> the key signature now always gets an accidental if they do not directly
> follow a note of the same pitch.
>
> * Added accidental-rule 'dodecaphonic - thereby obsoleting
> http://lsr.dsi.unimi.it/LSR/Item?id=314 :-)
>
> TODO:
>
> * convert-ly? I don't think this is nessesary. The change is
> downwards-compatible as long as the .ly do not mess around with
> 'autoAccidentals and 'autoCautionaries directly. Perhaps we could add a
> simple warning if these properties are accessed.
>
> * Make better use of pitchclass. Currently it is only used in the
> key-signature.
>
> * Some fixme's still in the code. Primarily regarding garbagecollection.
>   Currently it probably leaks like mad.
>
> * Bugtesting, bugtesting, bugtesting.
>
> -Rune
>
>
> _______________________________________________
> lilypond-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/lilypond-devel
>


-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen