gregorio icon indicating copy to clipboard operation
gregorio copied to clipboard

`spacebeneathtext` causes syllable hyphen to drop down into translation line.

Open matthewgonzalez opened this issue 7 years ago • 18 comments

This bug has been plaguing me on a project I am working on that relies heavily on the translation feature.

Gregorio Version: 4.2.0

The problem: At the end of a line, the hyphen drops down into the translation text (see screenshot) Culprit: I've narrowed this down to some issue with setting spacebeneathtext What should happen: The hyphen should display with its respective line and syllable, even when adjusting spacebeneathtext

I've attached a minimum example. Pardon the large margins; they were set this way purposely to force the hyphen situation.

screen shot 2017-02-25 at 4 00 33 pm

translation-hyphen-bug.zip

matthewgonzalez avatar Feb 25 '17 22:02 matthewgonzalez

I don't have time to track down which algorithm is responsible for inserting that hyphen at the moment, but based on the described symptoms, it must be failing to add spacebeneathtext to the height of the hyphen.

rpspringuel avatar Feb 26 '17 18:02 rpspringuel

This seems to be an issue with the hyphens which are added via gregoriotex.lua (to my knowledge, there are two other types of hyphens which are added in gregoriotex-syllable.tex).

The code in question is the following (sidenote, adddash is mispelled on the last line!):

if adddash==true then
          local dashnode, hyphnode = getdashnnode()
          dashnode.shift = currentshift
          hyphnode.font = currentfont
          insert_after(line.head, lastseennode, dashnode)
          addash=false
end

https://github.com/gregorio-project/gregorio/blob/e72026e4a2770d307b119b90f8e6dfbf0d812bdd/tex/gregoriotex.lua#L548-L554

The proper currentshift isn't being applied here due to the following conditional (I'm not sure why the conditional is there in the first place):

if currentshift == 0 then
              currentshift = n.shift
end

https://github.com/gregorio-project/gregorio/blob/e72026e4a2770d307b119b90f8e6dfbf0d812bdd/tex/gregoriotex.lua#L514-L516

If this conditional is removed, the hyphen assumes the correct shift adjustments and doesn't get pushed down.

I point these out to help you properly fix this. I'm not super familiar with your codebase.

matthewgonzalez avatar Mar 18 '17 15:03 matthewgonzalez

Thanks for the tips. I'll look into this later tonight and test those changes against the test repository.

rpspringuel avatar Mar 18 '17 21:03 rpspringuel

I am unable to duplicate the issue with the latest develop branch because Domino won't split between the o and the m. Do you have another example of this problem?

rpspringuel avatar Mar 19 '17 00:03 rpspringuel

I would adjust the left and right margins until the issue is forced. I notice that I often have to delete the .aux/.gaux files to get the syllable to break "correctly" after margin adjustments. I'm not sure what caused this particular hyphen to be thrown rather than the others.

matthewgonzalez avatar Mar 19 '17 00:03 matthewgonzalez

Deleting the aux files did it. The bug appears to be highly sensitive to the line width and previous runs. Once the line width is dialed in, then a fresh typesetting process will produce it, but if you tweak the line width into it, then things don't work out so well. Now that I can produce the bug, I'll look at the changes.

rpspringuel avatar Mar 19 '17 03:03 rpspringuel

Removing the conditional doesn't work for me. @eroux, this code originates from a change you introduced back in 2008 (https://github.com/gregorio-project/gregorio/commit/be1bfce0a11ae95c3c449ea40b764d1d7fac2716). Can you explain what the code is doing? If I'm reading things correctly, currentshift has to do with horizontal positioning of the hyphen, not vertical positioning. However, given this I don't see where the vertical position of the hyphen is supposed to be taken into account. @henryso, this might also have something to do with the dynamic line heights code, but I'm not sure so I'd appreciate you taking a look at that.

rpspringuel avatar Mar 20 '17 00:03 rpspringuel

Did you remove the conditional and leave thecurrentshift = n.shift ?

matthewgonzalez avatar Mar 20 '17 00:03 matthewgonzalez

Tried that and simply removing it altogether (i.e. allow currentshift to remain `0) neither had any effect on the hyphen when dealing with a clean compilation.

rpspringuel avatar Mar 20 '17 02:03 rpspringuel

Hmm... I just double checked using a different version of the test file (the one I didn't copy to the test repository) and now I am seeing the fix. I'm going to have to look at this in more detail tomorrow.

rpspringuel avatar Mar 20 '17 02:03 rpspringuel

the commit you referred to adds hyphens at end of lines when they are missing. For instance if you have Dominus in your score and if in the middle of a line there is no hyphen, then if a line breaks in the middle of the word, there would be no hyphen at the end of the line. This code adds this missing hyphen in Lua. If there was a bug in this code from 8 years ago I think it would have been found... For me the bug comes from a change in 5.0...

eroux avatar Mar 20 '17 07:03 eroux

If there was a bug in this code from 8 years ago I think it would have been found...

I tend to agree, though the bug would have been introduced probably in the 4.0, 4.1, or 4.2 series since the OP found it in 4.2.

Looking through the CHANGELOG, I think there are two candidates for the cause: variable line heights, and using traverse_id to get to the appropriate syllable. I'm afraid I don't know enough about these to see any potential problems.

On the testing front, I've determined that the problem is that I've uploaded everything correctly to the test repository, and that the @matthewgonzalez proposed change does change the test, but that the change isn't detected by the testing script, even with PDF_DENSITY upped to 1200. I've uploaded the test (tex-output/translation-hyphen-test) to the hyphen_height branch in the test repository. Would others please look at it to see if the change is detectable on their system.

rpspringuel avatar Mar 20 '17 15:03 rpspringuel

Some additional information, using \gresetlineheightexpansion{uniform} does not eliminate the bug, so I'm less inclined to think it's and issue with the variable line heights.

rpspringuel avatar Mar 20 '17 15:03 rpspringuel

I just tested with 4.1.4, which is from before traverse_id was introduced, and was able to reproduce the bug. I'm at a loss for when the bug might have been introduced then. I'll look into backing through the repository and doing some more testing on another day. I have other things I need to devote my afternoon to.

rpspringuel avatar Mar 20 '17 16:03 rpspringuel

So I finished those other things and did some more testing. The bug cannot be reproduced with v3.0.3 (using TeXLive 2014), which is the last version just before the line heights were introduced. On the other hand, it does appear in v4.0.0 (using TeXLive 2014 or 2015). I thus have to circle back to the idea that the bug is caused by the automatic line height adjustments.

rpspringuel avatar Mar 20 '17 18:03 rpspringuel

I've run the bisect and the results are that the first bad commit must be one of the following:

4a4e8ca082c2043a3244efd837ae5d84b591a5be b40817d0a13c23b52797de18059bdd055fb20e8d f5ef6542be100a937fed85df64f810b905dd2203 5c5821a81e06c884f3895dde755a6673dc6eae6f

Each of these commits has problems that forced me to skip them (either they wouldn't compile or there were typos in a TeX file that prevented the test document from compiling).

The problem is that I don't see in any of them code changes that would affect the hyphen. Maybe I made a mistake in the bisect process? Is there anyone who has TeXLive 2014 available that can check my work? The first steps are:

git bisect start
git bisect good v3.0.3
git bisect bad v4.0.0

If not I'll see if I can't set aside some time to try again.

rpspringuel avatar May 05 '17 18:05 rpspringuel

I don't have 2014 at the ready.

henryso avatar May 05 '17 20:05 henryso

I just completed a new bisect using one of my Linux VMs and it tells me that the first bad commit is 5c5821a81e06c884f3895dde755a6673dc6eae6f

Don't know what the problem is yet, but at least I've been able to figure out when the bug was introduced.

rpspringuel avatar Jun 10 '19 00:06 rpspringuel