ChordPro / chordpro

Reference implementation of the ChordPro standard for musical lead sheets.
Other
324 stars 51 forks source link

--a2crd doesn't detect chords at the start of a line #89

Closed nomike closed 4 years ago

nomike commented 4 years ago

Describe the bug If a line only contains one chord and it's in column 0 or 1, it is not detected as a chord.

To Reproduce Steps to reproduce the behavior: Run chordpro --a2crdon this file:

This is a song for testing chord detection with the --a2crd funtion:

A         C           D           E  F
The first line of the song is pretty normal,
G
but as the second line has only one chord it's not detected as such.
         Am
However, if this single chord is not right at the beginning of the line, it works again.
 Bm
A single space is not enough though,
  Cm
there have to be at least two.

Expected result

# This is a song for testing chord detection with the --a2crd funtion:

[A]The first [C]line of the [D]song is pret[E]ty [F]normal,
[G]but as the second line has only one chord it's not detected as such.
However, [Am]if this single chord is not right at the beginning of the line, it works again.
A[Bm] single space is not enough though,
th[Cm]ere have to be at least two.

Actual result

# This is a song for testing chord detection with the --a2crd funtion:

[A]The first [C]line of the [D]song is pret[E]ty [F]normal,
G
but as the second line has only one chord it's not detected as such.
However, [Am]if this single chord is not right at the beginning of the line, it works again.
 Bm
A single space is not enough though,
th[Cm]ere have to be at least two.
nomike commented 4 years ago

I've looked in the code of sub classify and I don't have an idea to actually fix this. The chord above text format isn't very good unfortunately.

I wonder though, how websites like https://ultimate-guitar.com get this right though, because they seem do.

One possible way would be to leave the heuristic as it is, because it works 98% of the time and add an additional step if it assumes it's a lyrics line: Tokenize the line at whitespaces and then check for every token if it's a known chord. If it is, assume it's a chord line. If not, stick to the previous oppinion, that it's a lyrics line.

This will not catch all cases, but at least most of them. Of course there can be false positives, but a lyrics line rarely consists of only "C" or contains "Asus4" or something similar.

Of course, if custom chords are defined within the .chopro file itself, this won't help, but I think that's the Moment where the 80/20 rule kicks in.

nomike commented 4 years ago

I was starting to work on this, but a2crd is this very separated module and I'm not sure if it's a good idea to drag to much of the main ChordPro module in there (like config and stuff).

What do you think?

sciurius commented 4 years ago

I think the most interesting module to use is App::Music::ChordPro::Chords::Parser, which is very independent of everything else. It should give access to valid chord names.

sciurius commented 4 years ago

Here is a little test: t/500_a2crd.t

Try it with

prove -lv t/500-a2crd.t

or

perl -Ilib t/500_a2crd.t
nomike commented 4 years ago

I made a commit which fixes detection for single chord lines:

A
This works now

However, it still does not work if there is a single space before the chord. I'm working on that.

ChordPro commented 4 years ago

What is the latest status?

nomike commented 4 years ago

I've merged the latest changes from the dev branch in my fork and pushed my latest commits. Chord detection works much more smooth now. I also fixed the issue with a single space before the only chord in a line.

I just pushed a last commit for removing two debug print statements.

The new version is capable of parsing this

This is a song for testing chord detection with the --a2crd funtion:

A         C           D           E  F
The first line of the song is pretty normal,
G
but as the second line has only one chord it's not detected as such.
         Am
However, if this single chord is not right at the beginning of the line, it works again.
 Cm
A single space is not enough though,
  Dm
there have to be at least two.
Emsus4    Am  C
Fooo      Bar Baz
           C    F
 A  Cm D F Even though this starts like some chords it isn't as there is text afterwards.

Into this:

# This is a song for testing chord detection with the --a2crd funtion:

[A]The first [C]line of the [D]song is pret[E]ty [F]normal,
[G]but as the second line has only one chord it's not detected as such.
However, [Am]if this single chord is not right at the beginning of the line, it works again.
A[Cm] single space is not enough though,
th[Dm]ere have to be at least two.
[Emsus4]Fooo      [Am]Bar [C]Baz
 A  Cm D F [C]Even [F]though this starts like some chords it isn't as there is text afterwards.

So feel free to try it out and pull it into your dev branch if you like it.

sciurius commented 4 years ago

Fixed in 0.978.