grame-cncm / guidolib

Guido project - music score layout engine - music description language
http://guido.grame.fr
Mozilla Public License 2.0
152 stars 34 forks source link

Cross-Staff Beaming #154

Closed arshiacont closed 1 year ago

arshiacont commented 2 years ago

This is an excerpt from Debussy's Children's Corner

{[ \staff<1>  
   (* meas. 1 *)  \clef<"f4"> \key<0> \meter<"C", autoBarlines="off", autoMeasuresNum="system"> 
   \staff<2> \stemsUp  \beamBegin:1 \beamBegin:2  f-1/16 c0/16 d0/16 e0/16 \beamEnd:2 \beamBegin:2 
   \staff<1> \stemsDown f0/16 a0/16 c1/16 d1/16 \beamEnd:1 \beamEnd:2 
   \staff<2> \stemsUp \beamBegin:1 \beamBegin:2 e1/16 
   \staff<1> \stemsDown d1/16 c1/16 a0/16 \beamEnd:2 \beamBegin:2 f0/16 
   \staff<2> \stemsUp e0/16 d0/16 c0/16 \beamEnd:1 \beamEnd:2 ]
 , 
[ \staff<2> 
   (* meas. 1 *) \clef<"f4"> \key<0> \meter<"C", autoBarlines="off", autoMeasuresNum="system"> \beamsOff 
   \ten<position="below">( f-1/1)
 ]
  }

Leads to image

Whereas it should look like this:

image

Two issues:

arshiacont commented 2 years ago

@dfober any hints where to start chasing this one in the code?

dfober commented 2 years ago

beaming is computed in GRBeam.cpp, more precisely in GRBeam::tellPosition, the code is really hard to understand (very long methods). I've already add a bit more structure a couple of years ago, but it remains a nightmare.

arshiacont commented 2 years ago

Just for memo: The problem in this issue is related to the initp0 method called in GRBeam::tellPosition which calculates a yoffset, exactly this block: https://github.com/grame-cncm/guidolib/blob/6d1fb4474af0b95dde8668188db904c0baba2074/src/engine/graphic/GRBeam.cpp#L398

the yoffset in this context seems to be wrong.

arshiacont commented 2 years ago

Slightly more readable test:

{[ \staff<1> 
  \clef<"f4"> \staff<2> 
   \stemsUp \beamBegin:1 \beamBegin:2 f-1/16 c0/16 d0/16 e0/16 \beamEnd:2 \beamBegin:2 
   \staff<1> \stemsDown f0/16 a0/16 c1/16 d1/16 \beamEnd:1 \beamEnd:2 _/2 ]
 , 
[ \staff<2>  \clef<"f4"> \key<0> empty/1 ]
  }
image
dfober commented 2 years ago

it looks like the beam is drawn in the wrong context e.g. the coordinates are relative to the first staff and it's drawn with the second one.

arshiacont commented 2 years ago

Thank you @dfober .

It seems like the problematic Beam here is being decoded as smallerBeam in GRBeam. The TellPosition attempts to fix offsets at the end of its function call but they are evaded by this line while treating fSmallerBeam: https://github.com/grame-cncm/guidolib/blob/6d1fb4474af0b95dde8668188db904c0baba2074/src/engine/graphic/GRBeam.cpp#L1337

What are smallerBeam in this context?!

If I comment that return statement, I get this which is slightly better but still have way to go:

image

arshiacont commented 2 years ago

@dfober I managed to get the following result by doing two things:

  1. Removing the return statement mentioned above so that the offset of the beam is correctly set
  2. Forcing the right-most beam [for now manually] as a fSmallerBeam of the longest beam (the first beam is included already but not the second) in GRVoiceManager::organizeBeaming(...) .

Whenever you have some time, we can go over the solution since I'm not sure if I understand the intent of fSmallerBeam and to avoid creating regressions. Ping me over email or else.

Once this is done the long beam's slope/initial points can be fixed more easily given the nested beams.

image

arshiacont commented 2 years ago

I am half-way done!

A particular challenge would be this excerpt where the nested beams (aka fSmallerBeam) are at the system-level:

{[ \staff<1>  
   (* meas. 1 *)  \clef<"f4"> _/2 \staff<2> \stemsUp \beamBegin:1 
   \beamBegin:2 e1/16 \staff<1> \stemsDown d1/16 c1/16 a0/16 \beamEnd:2 
   \beamBegin:2 f0/16 \staff<2> \stemsUp e0/16 d0/16 c0/16 \beamEnd:2 
\beamEnd:1 ]
 , 
[ \staff<2> \clef<"f4">  _/1 ]}
dfober commented 2 years ago

I've checked the wrong context hypothesis. This is the result when the beam is drawn in the context of the first staff issue154 The different slopes show also that nested beams are treated separately. My conclusion at this point is that the whole mechanics would have to be revised to handle nested beams properly.

arshiacont commented 2 years ago

Interesting.. Is there a branch with your hypothesis? On my branch I had a quick look at the slope issue for Nested Beam. in the setBeam() function, the "parent" beam can ideally adopt child positions but my conclusion was that it could easily work on this example but we can come up with more complex compound beaming where it wouldn't... .

dfober commented 2 years ago

No, there is no specific branch. I checked by correcting the y coordinates manually for this specific score (by subtracting the 'y' position of the staff) i.e. GRBeam::OnDraw :

if (st->p[0].y > 380) { ay[0]-=400; ay[1]-=400; ay[2]-=400; ay[3]-=400; };
    // This does the drawing!
    hdc.Polygon(ax, ay, 4);

where 400 is the second staff position().y

arshiacont commented 2 years ago

The changes I made in this PR actually does this: check changes in GRBeam.cpp: removing the return statement will allow the tellPosition to reach the end where we fix offsets.

On top of the above: changes in GRVoiceManager fixes how beams are nested so that the second beam renders as being nested.

Do you remember which issue was being broken in the regression test?

dfober commented 1 year ago

Based on your modification, here is the best result I can get (for now and without breaking the tests) issue154 I find it ambiguous: the second part looks like feathered beams. However I can commit that if it suits you.

arshiacont commented 1 year ago

hmmmm I guess this is better than what we have. Let's commit this. I will then break this into several tickets as I can see what the problems are.

dfober commented 1 year ago

that's done (pushed). I'll go on with this issue.

arshiacont commented 1 year ago

@dfober Here is the unit test that gathers several beaming issues all-together as discussed a few days back:

{[ \staff<1>  
   (* meas. 1 *)  \clef<"f4"> \key<0> 
   \staff<2> \stemsUp  \beamBegin:1 \beamBegin:2  f-1/16 c0/16 d0/16 e0/16 \beamEnd:2 \beamBegin:2 
   \staff<1> \stemsDown f0/16 a0/16 c1/16 d1/16 \beamEnd:1 \beamEnd:2 
   \staff<2> \stemsUp \beamBegin:1 \beamBegin:2 e1/16 
   \staff<1> \stemsDown d1/16 c1/16 a0/16 \beamEnd:2 \beamBegin:2 f0/16 
   \staff<2> \stemsUp e0/16 d0/16 c0/16 \beamEnd:1 \beamEnd:2 ]
 , 
[ \staff<2> 
   (* meas. 1 *) \clef<"f4"> \key<0>
 ]
  }
dfober commented 1 year ago

fixed (could be improved)