[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by add

From: Paul Morris
Subject: Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by address@hidden)
Date: Tue, 2 Apr 2019 09:52:32 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

Hi Carl,

On 4/2/19 12:14 AM, address@hidden wrote:
input/regression/ chExceptions = #(append
(chordmode->exception-entry chordVar markupVar) chExceptions)
Hmm, chExceptions is used in its own definition here?  Does that work? This is
not making sense to me.

chExceptions is previously defined to be a couple of new chords
prepended to ignatzekExceptions, so it can be used this way.

Ah, right, okay.
scm/chord-entry.scm:238: 'chord-semantics chord-semantics))
Hmm, so there's a single 'chord-semantics property under a 'ChordSemanticsEvent ?  Seems like we could avoid that extra step of nesting and just have the subproperties of 'chord-semantics under 'ChordSemanticsEvent ?  And that would
be more like NoteEvent and other events. Or maybe I'm missing something?

We could do that, but it would pollute the global namespace with all the
that are part of 'chord-semantics and are only used for chord semantics.
 By putting them
in a single property, we avoid polluting the namespace.  Similar to the
way we do fret-diagram-details.

Okay, I see.
scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch original-inv-pitch))))
Would be more clear and consistent if original-inv-pitch were renamed to
original-inv-entry or similar.

Why?  Not to be argumentative, but I wonder what about this name is more
clear and consistent, in your opinion?

Should have said more at the time.  Here's what I think I was thinking.

Having 'pitch' in the name conveys that it is a LilyPond 'pitch' data structure, but that isn't the case.  Here you have to call `entry-pitch` on it to get a pitch out of it to pass to `ly:pitch-octave`.

`entry-pitch` works on `chord-entry` type things:

;; Return pitch of a chord-entry
(define (entry-pitch chord-entry)
  (car chord-entry))

So it seems to be more like a type of 'entry' structure, and avoiding using 'pitch' in the name (like with original-inv-entry or similar) makes it easier to understand what it is and what you can and can't do with it.
scm/chord-entry.scm:399: ((= alteration FLAT) (set! quality 'dim)))))
Instead of all of these (set! quality ...) why not just let the result of this cond be assigned to 'quality' and add an 'else' 'maj at the end? Basically:
(quality (cond ((= alteration SHARP) 'aug) ...))

I think it's because for some intervals, you set a zero alteration as
perfect, while
for others, the zero alteration is 'maj.  So it doesn't drop easily into
an else, I think.

Ah, right, so we still need to take step-number into account. But there's still refactoring that can be done along the lines I suggested, as there are basically two cases, the major/minor and the perfect.  Here's an untested refactor:

 (define (get-quality step-number alteration)
    ;; major/minor
    ;; note from Paul: I added 7 and 14 which weren't in the original
    ((member step-number '(2 9 3 10 6 13 7 14))
     (case alteration
      ((SHARP) 'aug)
      ((0) 'maj)
      ((FLAT) 'min)
      ((DOUBLE-FLAT) 'dim)))
    ;; perfect
    ;; TODO: will 11 have the same qualities as 4?
    ((member step-number '(1 8 4 11 5 12))
     (case alteration
      ((SHARP) 'aug)
      ((0) 'perfect)
      ((FLAT) 'dim)))
    ;; default fallback
    (else 'maj)))

 (define (make-chord-entry-from-pitch pitch)
   (let* ((step-number (pitch-step pitch))
          (alteration (ly:pitch-alteration pitch))
          (quality (get-quality step-number alteration)))
     (make-chord-entry pitch (make-chord-step step-number quality))))
scm/chord-entry.scm:411: (if (= step-number 7) (set! quality 'min))
Defining quality using cond or case would be a more idiomatic Scheme-ish way to
do it, avoiding the mutation of set!.

I totally agree with you; I'll try to work this out for a future patch.
For now, I want
to just get it into review.

Okay, makes sense.  Thanks for working on this.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]