moinejf / abc2svg

project moved to https://chiselapp.com/user/moinejf/repository/abc2svg
GNU Lesser General Public License v3.0
48 stars 6 forks source link

Wrong barnumber if variation starts within a measure #82

Closed bwl21 closed 6 years ago

bwl21 commented 6 years ago
X:3064
F:3064_github-issue-206-Wrong-barnumbers
T:github issue 206: Wrong barnubmers 
C:
S:
M:6/4
L:1/4
Q:1/4=120
K:C
%
%
%%score 1
%
V:1 clef=treble name="Sopran" snm="S"
cc | cc ]1 c2 :]2 d2 |] e3e3 |f4 |]

Yields

screenshot_1142

IMHO the barnumber 2 should be on the end of the variation. screenshot_1142

bwl21 commented 6 years ago

With 93f928dae204a2ca1e8f8c3a7f7e414f6ea78a3e

I get

screenshot_1147

which is IMHO not correct, as bar number is now "3" instead of "2"

bwl21 commented 6 years ago

tried with abc2svg-1.16.2-2-g1268b0f (2018-04-02)

I:contbarnb 1
I:measurenb 1
X:3064
F:3064_github-issue-206-Wrong-barnumbers
T:github issue 206: Wrong barnubmers 
C:
S:
M:6/4
L:1/4
Q:1/4=120
K:C
%
%
%%score 1
%
V:1 clef=treble name="Sopran" snm="S"
cc | cc ]1 c2 :]2 d2 |] e3e3 |f4 |]

still yields number 3 instead of 2

screenshot_1155

With I:contbarnb '0' ist give barnumber as 2

moinejf commented 6 years ago

This is because you didn't set correctly the measure bars. The music should be:

cc cc [1 c2 :|2 d2 |] e3e3 |f4 |]
bwl21 commented 6 years ago

Thanks, the measure bars were indeed not set correctly. The intention was (start with an upbeat, then measure 1 with a variant end, then measure 2 which begins after the variation, then a full measure, then a measure of four beats (matching the upbeat).

BB | cc cc ]1 c2 :|2 d2 |] e3e3 |f4 |]

and it should yield

screenshot_1156

Harpnots are as follows. You can see the intended flow through the arrows. Bar are visible as barlines above the note. Barnumbers are besides the notes with the bars.

screenshot_1158

why should ]1not work? I thought it denotes an invisible bar (but I don't remember where I got this information). Nevethelss, I had a look in bar_cnv(bartype) and see that it returns the given bartype if the given bartype is not caught by the case statement. Therefore ] works by accident. Maybe it should be reported as error if it is not supported.

Nevertheless, as I have to detect invisible bars for creating the harpnotes, I had ] hardcoded, which is obviously not correct. How are invisible bars reported in voice_element.bar_type ?

moinejf commented 6 years ago

About ]1 et al., you should have a look at the ABC standard and not at my code! abcm2ps and abc2svg are permissive. It is simpler to convert ]1 to [1 instead of checking if it enters in some standard scheme. What is sure is that some ABC programs would not accept such a syntax. The standard invisible bar is []. About the numbering problem, I will have a look tomorrow...

bwl21 commented 6 years ago

About ]1 et al., you should have a look at the ABC standard and not at my code!

right - even if your code follows the standard pretty exactly :-) Abc 2.2 says: "An invisible bar line may be notated by putting the bar line in brackets, e.g. [|] ... ".

Do I get it right that the voice_element.bar_type is populated with a reconciled value in parse.js near line 1297? Or is it even a combination of bar_cnv and the parser?

moinejf commented 6 years ago

bar_cnv() is used for drawing. Otherwise, the job in new_bar() is explained in the documentation http://moinejf.free.fr/abcm2ps-doc/features.xhtml section 4.8

bwl21 commented 6 years ago

It is really hard, sorry ...

I:contbarnb 1
I:measurenb 1
X:3064
I:MIDI program 25
F:3064_github-issue-206-Wrong-barnumbers
T:github issue 206: Wrong barnubmers 
C:
S:
M:6/4
L:1/4
Q:1/4=120
K:C
%
%
%%score 1
%
V:1 clef=treble name="Sopran" snm="S"
BB |:"^Bn:1" cccc  []1 c2 "^invisible" :|2  d2 |] "^Bn:2" e3e3 | "^Bn:3" f4 |]

now yields

screenshot_1164

The I have noted the expected barnumbers as "Bn:2" ... If I set contbarnb to 0 then I get the barnumber 2. But with contbarnb=1 it seems to count the invisible bar as well.

moinejf commented 6 years ago

Found. But your syntax is still not correct. See the sections 4.9 and 4.10 of the ABC standard 2.2: the variants start with '[' only.

bwl21 commented 6 years ago

now it works as I expected. Thanks.

Wrt to not correct syntax:

chapter 4.8 says: Abc parsers should be quite liberal in recognizing bar lines. In the wild, bar lines may have any shape, using a sequence of | (thin bar line), [ or ] (thick bar line), and : (dots), e.g. |[| or [|::: .

When I look into 4.9 and 4.10: these chapters say, that variations start with [1 or even ["var 1". This means that the beginning of a variation is entirely separate from a bar.

ABC also separates between first and second repeat and variant endings. It says that |[1 can be shortcut to |1. But |["var1" cannot be shortcut to |"var1" (for obvious reaons, "var1" could be the label of a variant or a chord).

In abc2svg there is a visual difference between |[2 and |["2" which is not yet fully reasonable for me.

screenshot_1165 vs. screenshot_1166

I would like to teach my users the "correct way", but this is hard, if "quite liberal syntaxes" work as well.

I really have problems with "quite liberal" approaches, they encourage shortcuts which eventually manifest themselves as extra way and extra complications.

bwl21 commented 6 years ago

I assume that the reason for the visual difference is in the distinction between second repeat and variation.

I don't think that we should try to fix this.

moinejf commented 6 years ago

This was a bug in bar parsing: adding new features complexifies the code and brings bugs. BTW, ["2" is a nonsense: the string syntax (["string") has been added to solve the "last time" variant.

bwl21 commented 6 years ago

sure it is nonsense but it was the easiest to play around. Nevertheless now (with c56430a) it is fine. Thanks.

It is true, that adding features has the risk to bring new bugs. This is the reason for my attempts to do regression testing. It may also happen that I sometimes regret to add particular features (or even bugfixes). Better live with know bugs than with unknown bugs. ;-)

bwl21 commented 6 years ago

Sorry, i have to reopen it ... while playing around (I call this monkey test) I observe some problems. I see that the "string" syntax only makes sense for the last variation. So I assume that a non-integer switches from "first/second repeat" to a variation within the repeat.

But the hang of case 3 should be fixed.

  1. with this testcase

    I:contbarnb 1 
    I:measurenb 1
    X:3064
    I:MIDI program 24
    F:3064_github-issue-206-Wrong-barnumbers
    T:github issue 206: Wrong barnubmers 
    C:
    S:
    M:6/4
    L:1/4
    Q:1/4=120
    K:C
    %
    %
    %%score 1
    %
    V:1 clef=treble name="Sopran" snm="S"
    BB |: cccc ["first" c2 "^invisible" :["last"  d2 |] "^Bn:2" e3e3 | "^Bn:3" f4 |]

    the position of bar number is wrong again:

    screenshot_1167

  2. If I replace "first" by "1" I get the correct result

    screenshot_1168

  3. If I remove "first" such that only [ remains, then abc2svg hangs.

bwl21 commented 6 years ago

[ no longer hangs with f34c3fb. thanks