Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression with addMetaNote that effects time signatures, fermatas and tempo #59

Open
cianoc opened this issue Jul 4, 2021 · 9 comments · Fixed by #68
Open

Regression with addMetaNote that effects time signatures, fermatas and tempo #59

cianoc opened this issue Jul 4, 2021 · 9 comments · Fixed by #68
Assignees
Labels

Comments

@cianoc
Copy link
Member

cianoc commented Jul 4, 2021

I first noticed this with the bartok example where it's all in common time. Here's an example:

{-# LANGUAGE OverloadedStrings #-}

import Music.Prelude

main = defaultMain music

music = let 
    meta = title "Test 123"
        . composer "Some guy"
        . timeSignature (time 3 4)

    in meta $ pseq[c, d, e, f, g]
@cianoc
Copy link
Member Author

cianoc commented Jul 4, 2021

%%% WARNING: GENERATED FILE. DO NOT EDIT. %%%

\version "2.20.0"

\paper {
    #(define fonts
       (make-pango-font-tree "Times New Roman"
                             "Arial"
                             "Andale Mono"
                             (/ staff-height pt 20)))


    % First page vs. subsequent pages
    indent          = 0.3\in
    short-indent    = 0\in
    % horizontal-shift

    % paper-width
    % left-margin
    % right-margin

    % Horizontal space taken up by music
    line-width      = 210\mm - 2.0 * 0.4\in

    page-breaking = #ly:one-page-breaking
}

\header {
  title         = "Test 123"
  composer      = "Some guy"
  tagline       = ""  % removed
}

\layout {
}

#(set-global-staff-size 10)

<<
    \new StaffGroup <<
                    >>
    \new StaffGroup <<
                    >>
    \new StaffGroup <<
                    >>
    <<
        \new Staff {   \set Staff.instrumentName = "Piano I" \set Staff.shortInstrumentName = "Piano I" \clef treble {   \time 4/4 c'1-\mf
                                                                                                                     } d'1 e'1 f'1 g'1
                   }
    >>
    \new StaffGroup <<
                    >>
    \new StaffGroup <<
                    >>
>>

@cianoc
Copy link
Member Author

cianoc commented Jul 4, 2021

t

@cianoc
Copy link
Member Author

cianoc commented Jul 16, 2021

So I'm happy to try and take a go at this - but I'm probably going to need some pointers as where to look.

@cianoc
Copy link
Member Author

cianoc commented Aug 5, 2021

Tempo is also being ignored. This bug also seems to occur with musicxml export, though the musicxml file currently crashes musescore for unknown reasons :(

@hanshoglund
Copy link
Member

hanshoglund commented Aug 5, 2021

This looks like a regression. I would go about it like this:

  • Check if the problem extends to StandardNotation (the pipeline is Music -> StandardNotation -> [Xml/Ly]).
  • Depending on the outcome, continue to "bisect" the pipeline to find the exact spot where the information disappears. You can use MonadLog for debug printing, or rely on tests an REPL to test the pure functions.
  • Finally I would add a golden test (e.g. using tasty-golden or hspec-golden) to prevent the issue from reappearing.

Let me know if you need any further pointers!

@cianoc cianoc changed the title Time Signatures are ignored when generating lilypond Regression with addMetaNote that effects time signatures, fermatas and tempo Aug 5, 2021
@cianoc cianoc self-assigned this Aug 5, 2021
@cianoc
Copy link
Member Author

cianoc commented Aug 5, 2021

Ah thanks! I've fixed the time signature bug (caused by a regression due to the Option datatype being deprecated - which is a little weird, but a trivial fix). But now I've discovered a wider issue with addMetaNote that I'm looking into.

cianoc pushed a commit to cianoc/music-suite that referenced this issue Sep 23, 2021
@cianoc cianoc linked a pull request Sep 23, 2021 that will close this issue
hanshoglund pushed a commit that referenced this issue Sep 28, 2021
hanshoglund pushed a commit that referenced this issue Sep 28, 2021
@hanshoglund
Copy link
Member

Reopening as fermatas and tempo needs to be resolved too.

@hanshoglund hanshoglund reopened this Sep 28, 2021
@cianoc
Copy link
Member Author

cianoc commented Oct 5, 2021

So I've looked into this some more and for the code where tempo would be generated you have "TODO add tempo". So I'm guessing this got removed in a refactoring. I couldn't see anything for fermata.

Is there any reason for tempo to be tied to a barline like it used to be? Obviously it makes it easier to code, but it does feel a little limiting. Particularly as fermatas will be independent of the barline and I have to work out how to code that anyway.

@hanshoglund
Copy link
Member

I looked through the code and it seems these features exist in the Lilypond AST (Music.Data.Lilypond). There was a bug in the Lilypond pretty-printer which I have fixed in #70/#71.

It looks like the code to render these from StandardNotation.Work where never added to the toLy function. I think this is straightforward to do, following the approach in toXml.

I don't think there is any non-technical reason reason to connect tempo/fermata to barlines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants