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

space around bars #767

Closed
eroux opened this issue Jan 8, 2016 · 37 comments
Closed

space around bars #767

eroux opened this issue Jan 8, 2016 · 37 comments
Assignees
Milestone

Comments

@eroux
Copy link
Contributor

eroux commented Jan 8, 2016

The space around bars in Gregorio are really too wide, but the algorithm should also be improved, and be like the one with normal syllables (only much more complex).

Also, I believe there is a bug somewhere because the space on the left of bars with text is really wider than the space on the right, so there might be two spaces, or a missing % somewhere...

@eroux
Copy link
Contributor Author

eroux commented Jan 9, 2016

A few examples from Solesmes' books, some interesting edge cases to handle:

exemple2

exemple3

exemple4

exemple1

@rpspringuel
Copy link
Contributor

The ones where the * appears under the notes strikes me as being more like ra *(efef) (,) rather than ra(efef) *(,) (to use the third as an example). Making the latter print something like is shown would require identifying the text of the bar syllable as an asterisk.

The last example should be accomplishable in the following manner:

  1. After printing a bar set \gre@lastisbartrue.
  2. After a line break set \gre@lastisbarfalse.
  3. At the beginning of a normal syllable, \ifgre@lastisbar and begindifference is negative (i.e. text starts before notes) then kern backwards the smaller of spacearoundbar + barwidth or -begindifference.
  4. At end of normal syllable set \gre@lastisbarfalse

@henryso
Copy link
Contributor

henryso commented Jan 22, 2016

To me, it looks as if the * is centered in the space between the syllables, which requires something a bit more clever than ra *(efef) (,), no?

@eroux
Copy link
Contributor Author

eroux commented Jan 23, 2016

Indeed, it's quite complex, the few notes I have (in my head) are:

  • when no text is associated, spacing of text between previous and next syllable should have the same constraints as if there was no bar
  • when there is text but no notes nor bars (it should be a BarSyllable in this case, I don't know how it is now), space between notes of the previous syllable and notes of the next syllable should have the same constraint as if there was not text, and text should be centered between syllables, with the same constraints as normal text (minimal space with previous and next syllable)
  • where there is a bar and text:
    • center the bar between the notes of surrounding syllables, with a minimal space
    • center the text between text of surrounding syllables, with a minimal space
    • add an additional contraint: maximal space between center of bar and center of associated text (so that they're not shifted too much)

that's about it... the algorithm itself is not complex, but the implementation won't be simple I'm afraid...

@rpspringuel
Copy link
Contributor

when no text is associated, spacing of text between previous and next syllable should have the same constraints as if there was no bar

This might run into problems if begindifference is positive. I think it would be better if the bar was positioned based on the notes and ignore the lyrics of both the previous and next syllable.

Actually, I think what you list for the third option would actually cover all three conditions, so long as an empty bar or text is understood to be perfectly aligned with the non-empty element.

I would implement this as an option, as there may be some people who want the old behavior.

@eroux
Copy link
Contributor Author

eroux commented Jan 24, 2016

The third case would indeed more or less cover it all, maybe it would be a good start, and adjustments could be made later if necessary. I'm not totally sure about the option, the code is already quite complex, this new feature will make it at least twice more complex, not having an option could make things a little bit simpler.

Also, when the bar is the last thing of a line, of course it doesn't make sense to "center" the bar or the text between previous and next syllable, so this case should be handled differently (in a discretionary certainly)...

@eroux
Copy link
Contributor Author

eroux commented Jan 24, 2016

Would you like to give it a try? I'm currently stuck on stem length...

@rpspringuel
Copy link
Contributor

I'll give it a shot.

@rpspringuel
Copy link
Contributor

See https://github.com/rpspringuel/gregorio/tree/feature/bar_spacing

I started by simply adding the necessary infrastructure to make the new spacing optional as that's the easiest change.

I've started thinking about how to implement the new spacing algorithm, but don't have anything workable yet. Looking at the time, I realize that I have to spend the rest of today doing my homework, even if tomorrow is likely to be a snow day due to the sheer amount of snow we got in the DC area yesterday (somewhere between 18 and 24 inches, depending on who you believe). I'll try knocking up a first pass at the actual spacing algorithm tomorrow, especially if it does turn out to be a snow day.

@eroux
Copy link
Contributor Author

eroux commented Jan 25, 2016

Ok good! Take your time, that's quite a difficult task I think...

@rpspringuel
Copy link
Contributor

Please look at 3810a44 and gregorio/gregorio-test@be8a060. The first is my first pass at implementation, but as you can tell from the second it's not completely successful (especially when the text needs to come before the bar). I have to stop working for today, and so can't continue to try and diagnose the problem, but I could use another set of eyes to review the code and spot any obvious mistakes.

@rpspringuel rpspringuel assigned rpspringuel and unassigned eroux Jan 29, 2016
@eroux
Copy link
Contributor Author

eroux commented Jan 30, 2016

I think the correct link is rpspringuel/gregorio-test@be8a060 ? This really looks promising! Tere are still many things to tune, but I think you're on really good tracks here!

Another use case that would be very important to improve (I would almost say "fix" as current output is quite bad):

g(g) (::) 5. Start(g)

I think it's the best way the pattern found in some pieces. This way the 5. remains with the Start even when line breaks after the bar, but when it does not, the bar should be somwhere above 5. It's also important to input it this way because the space between 5. and Start should be a normal space, it shouldn't be a stretched like a space around bar. Could you make something like that in your tests?

I'll review it more carefully when the clivis stem length will be done (today I hope)!

@rpspringuel
Copy link
Contributor

Yes, that is the correct link.

@rpspringuel
Copy link
Contributor

It looks to me like the old system already did what that image shows. Are you sure you've got the right image?

@eroux
Copy link
Contributor Author

eroux commented Jan 30, 2016

Sorry, that's possible, I admit i didn't test, i just wanted to make sure it was possible in the new system if it wasn't in the old one, but if it already was that's good! Sorry for the noise! I should have time tomorrow to review what you did.

@eroux
Copy link
Contributor Author

eroux commented Feb 2, 2016

I've tried your branch, but I see a few problems:

  1. first the "old" spacing doesn't have the same result as the spacing in develop:

In the following example:

name:bars-spaces;
%%
{1}11111(g) 2{2}2(,) 33333{3}(gf)
{1}11111(g) 2{2}2(::) 33333{3}(gf)
{1}11111(g) 2{2}2() 33333{3}(gf)
{1}11(g) (::) 33{3}(gf Z)

11{1}(g) 2{2}2(,) 33333{3}(gf)
11{1}(g) 2{2}2(::) 33333{3}(gf)
11{1}(g) 2{2}2() 33333{3}(gf)
11{1}(g) (::) 33333{3}(gf Z)

11{1}(g) 2{2}2(,) {3}(gf)
11{1}(g) 2{2}2(::) {3}(gf)
11{1}(g) 2{2}2() {3}(gf)
11{1}(g) (::) {3}(gf Z)

{1}11111(g) 2{2}2(,) {3}(gf)
{1}11111(g) 2{2}2(::) {3}(gf)
{1}11111(g) 2{2}2() {3}(gf)
{1}11111(g) (::) {3}(gf Z)

used in

\documentclass[11pt]{article}
\usepackage{fontspec}
\usepackage{graphicx}
\usepackage{geometry}
\geometry{a4paper}
\usepackage{gregoriotex}
\usepackage{fullpage}
\usepackage{libertine}
\begin{document}
\gresetclivisalignment{always}
\gresetinitiallines{0}

\kern -1cm

old spacing:

\gregorioscore[a]{bars-spacings}
\end{document}

I get the following in the develop branch:

old

and the following in your branch (adding the new spacings too):

oldnew

as you can see, in the fist syllable, there is not space between 1s and 2s in your branch, while there is in develop.

  1. the new spacings have a few problems too:
  2. there is way too much space between 1s and 3s in the last part of the first, second and fourth line
  3. bar is not centered in the last example of the 3rd line (it was quite ok in the old ones)
  4. way too much space between 2s and 3s of the first and second parts of the fourth line
  5. generally, there is too much space between text... it seems much more than the regular inter-text spacing...

Anyway, thanks a lot for your work! It would be very helpful to get a beta2 when #803 will be merged, I was thinking to merge this too and let the old spacings by default for now, asking testers to test the new ones, what do you think? Of course, this supposes that point 1. of this comment is fixed...

@rpspringuel
Copy link
Contributor

I just pushed a new commit which should fix the changes to the old spacing. I don't have time to test right now, but I thought I'd make it available.

@eroux
Copy link
Contributor Author

eroux commented Feb 2, 2016

Seems to solve the issue with old spacings indeed, thanks a lot! I think we can merge your branch into develop before releasing beta2 then, even if you're not completely done with all the details.

@rpspringuel
Copy link
Contributor

I have one idea I want to play with before submitting a PR, but I should be able to have something for tomorrow.

✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝✝
Br. Samuel, OSB
(R. Padraic Springuel)
PAX ☧ ΧΡΙΣΤΟΣ

On Feb 2, 2016, at 9:40 AM, Elie Roux notifications@github.com wrote:

Seems to solve the issue with old spacings indeed, thanks a lot! I think we can merge your branch into develop before releasing beta2 then, even if you're not completely done with all the details.


Reply to this email directly or view it on GitHub.

@rpspringuel
Copy link
Contributor

I've posted what I wanted to try and am more satisfied with it than what I was getting before, though there is still some problem with the positive previousenddifference and positive nextbegindifference case.

Even if I can't figure out the problem, I'll write up the documentation that goes with the changes and submit a PR tomorrow.

@rpspringuel rpspringuel modified the milestone: 4.1 Feb 5, 2016
@rarty
Copy link

rarty commented Feb 8, 2016

I was testing this (4.1.0-beta2-develop-cb7c9fa-3219) and for scores with translation text beneath the lyrics, activating the new bar spacing causes all text associated with/under bars to shift down to the same height as the translation line:

\gresetbarspacing{new}

\gabcsnippet{(c3) TU(ig) es(ij) Pe(i)trus,(i) *(,) et(i) su(i)per(g) hanc(hg) pe(e)tram(e) <i>etc.</i>(::)}

\gabcsnippet{(c3) TU[Thou art Peter, and upon this rock](ig) es(ij) Pe(i)trus,(i) *(,) et(i) su(i)per(g) hanc(hg) pe(e)tram[/](e) <i>etc.</i>(::)}

\gresetbarspacing{old}
\gabcsnippet{(c3) TU[Thou art Peter, and upon this rock](ig) es(ij) Pe(i)trus,(i) *(,) et(i) su(i)per(g) hanc(hg) pe(e)tram[/](e) <i>etc.</i>(::)}

texshop_image

@eroux
Copy link
Contributor Author

eroux commented Feb 13, 2016

An additional change to make on the C side: syllables without notes should be considered as BarSyllables, see this thread. I think this is a relatively small change... @henryso do you think you could do it? You're the one who knows the C code best

@eroux
Copy link
Contributor Author

eroux commented Feb 13, 2016

@rpspringuel, apart from the small bugs that should be fixed in rc1, I only got positive feedback on the new algorithm, so I think it should be the default one in rc1. Thanks a lot, you did a great job!

@rpspringuel
Copy link
Contributor

I've been studying the C code to see if I could figure out how to make the change myself, and I've come to the conclusion that there are two possible strategies:

  1. Modify the notes-determination algorithms so that it recognizes an empty syllable as a type of bar. Unfortunately, while I could figure out how to add another bar type by basically imitating the code connected to a division minima, I don't understand how those algorithms work well enough to see how one would go about recognizing a lack of any character, rather than some specific character.
  2. Modify the algorithm that decides whether to write \GreBarSyllable or \GreSyllable so that an empty syllable triggers the first rather than the second. Right now the condition that makes this decision is ((syllable->elements)[0]->type == GRE_BAR). Adding an "or the syllable contains no notes" to this condition would therefore accomplish that, but I don't know how to translate that into the code. Would it be something like || (syllable->elements)[0] == ""?

With a few pointers I might be willing to give one of these strategies a go, but I can't promise that I'll be able to get any results, C is not my strong suit.

Likewise it may be that neither of these strategies is the way to go, so feel free to tell me that and I'll leave this to someone with more expertise.

@henryso
Copy link
Contributor

henryso commented Feb 13, 2016

Off the cuff, I would change:

    } else {
        write_fixed_text_styles(f, syllable->text,
                syllable->next_syllable? syllable->next_syllable->text : NULL);
        syllable_type = "\\GreSyllable";
    }

to

    } else {
        write_fixed_text_styles(f, syllable->text,
                syllable->next_syllable? syllable->next_syllable->text : NULL);
        syllable_type = "\\GreBarSyllable";
    }

However, I have not looked into it deeply to see if this will break something else or not.

@henryso
Copy link
Contributor

henryso commented Feb 13, 2016

@rpspringuel Let me know if you are working on the C part for this so that I don't step on your toes.

@rpspringuel
Copy link
Contributor

I'm testing your off-the-cuff change, but a thought has occurred to me with this. Not only do we have to change whether \GreSyllable to \GreBarSyllable in the syllable which has an empty notes, but we also have to change the look-ahead argument in the previous \GreSyllable command (#7). Right now an empty notes sets that to 0 (one-note glyph or more than two notes glyph except porrectus : here we must put the aligncenter in the middle of the first note) when it needs to be 11 (divisio minima, minor and maior) (10, virgula, and 12, divisio finalis, would also work since for the purposes of this spacing calculation we just need to know that the next item is a bar, not what kind it is).

That's going to get much harder and while I could probably eventually figure it out, I'm not going to be able to get it ready in time for tomorrow (the day we want to release rc1). It's probably better if someone with more knowledge of the C makes this change if it's going to happen quickly.

@henryso
Copy link
Contributor

henryso commented Feb 13, 2016

The function you want to look at for that is gregoriotex_syllable_first_type. At the return 0 at the end of the function, you need to change it to something like:

    if (syllable->elements[0]) {
         return 0;
    }
    return <the appropriate value for a no-note syllable>;

@eroux
Copy link
Contributor Author

eroux commented Feb 13, 2016

There are many open issues (though most of them are small), I think it's best is to make sure everything is ok for rc1, we can delay it of a day or two if necessary

@rpspringuel
Copy link
Contributor

@henryso: Thanks. I'll try that.

@rpspringuel
Copy link
Contributor

Testing is looking good so far, but I've noticed that the change in syllable type breaks scores which use the old spacing system if they had one of these syllables with no notes. Is that going to be a problem for anyone?

@henryso
Copy link
Contributor

henryso commented Feb 14, 2016

I don't know if people will have an issue with this sort of thing, but if we think it might, then perhaps we should add a new type of syllable (like \GreNoNoteSyllable), emitted in this case, whose behavior can be controlled (aliased to \GreSyllable or \GreBarSyllable) from TeX?

@rpspringuel
Copy link
Contributor

gregorio-project/gregorio-test@16c162e shows the 4 tests which are affected by this change in behavior. If people think it will be a problem, I can easily implement a solution like @henryso suggests.

@rpspringuel
Copy link
Contributor

Shoot. I forgot about the look-ahead argument again. That will create a problem with @henryso's solution. \GreNoNoteSyllable couldn't be a simple alias, but would also have to correct for the previous syllalble's look-ahead argument being wrong. It's probably still doable, but would be a bit more complicated.

@eroux
Copy link
Contributor Author

eroux commented Feb 14, 2016

For now, let's just turn the new algorithm by default and make all no-notes syllables \GreBarSyllable, if we have negative feedback, we'll just change something in rc2

@rpspringuel
Copy link
Contributor

This may only apply to my branch, but I want to note that when a bar syllable with a translation text assigned to it occurs at the end of a line and is followed by a line which does not have any translation text, then the lyric text for the bar syllable is not raised up above the translation text (note, I've tried varying these conditions and this is the only circumstance I can find where this bug occurs). This bug can be seen in the results of the bar-spacing-new test in db97a16. My suspicion is that textlower (the amount of the raise) is getting reset to zero a bit too soon, but I have not yet dug into the code to determine where that might be happening.

@eroux
Copy link
Contributor Author

eroux commented Feb 16, 2016

Thanks, can you provide an example? (and open a separate issue)

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

No branches or pull requests

4 participants