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

Name font calibration distances #1526

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

rpspringuel
Copy link
Contributor

There were several "magic distances" still in the code. All of these distances are related to calibrating things to the fonts and so should not be user editable. However, having those numbers scattered through the codebase is harder to maintain (if you change one, you have to figure out which of the others also needs to change, by how much, and where they all are), so based on relationships between the numbers, I've reduced them to 6 named dimensions which are then used as needed in the various calculations.

Fixes #1521.

There were several "magic distances" still in the code.  All of these distances are related to calibrating things to the fonts and so should not be user editable.  However, having those numbers scattered through the codebase is harder to maintain (if you change one, you have to figure out which of the others also needs to change, by how much, and where they all are), so based on relationships between the numbers, I've reduced them to 6 named dimensions which are then used as needed in the various calculations.
@rpspringuel rpspringuel requested review from eroux and henryso February 12, 2021 19:07
@rpspringuel
Copy link
Contributor Author

One question on this remains:

Right now we have noteadditionalspacelinestext which is the amount of space added per level of really low note and is approximately ((\gre@dimen@interstafflinedistancebase + \gre@dimen@stafflinethicknessbase) / 2 ) (the vertical distance between notes a half-step apart). Should we replace this distance with the formula or is there a reason why when adding space between the notes and the lyrics, one would want to add something other than the distance actually required by the low note position?

@rpspringuel rpspringuel linked an issue Feb 12, 2021 that may be closed by this pull request
@rpspringuel
Copy link
Contributor Author

It should be noted that currently in the code ((\gre@dimen@interstafflinedistancebase + \gre@dimen@stafflinethicknessbase) / 2 ) is used for high notes above the staff. For consistencies sake, if noteadditionalspacelinestext it to be retained, then we should do something similar for the above staff notes.

Copy link
Contributor

@henryso henryso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to trust that your math is right. If the tests pass, then it probably is, or at least is close enough.

@rpspringuel
Copy link
Contributor Author

What about the issue with noteadditionalspacelinestext? Is it more reasonable to have the amount of space added (both above and below the staff) be based how much outside the staff the high/low notes are, or allow the user to customize the amount of space added per step outside the staff?

@henryso
Copy link
Contributor

henryso commented Feb 14, 2021

If it works, then go for it. I know there's some weirdness in spaces above and below the staff.

The additional distance added between the score and the lyrics for lower than "normal" notes (as defined by `noteadditionalspacelinestextthreshold`) is now calculated automatically based on the exact distance below the "normal" position that the low notes have been placed.  This mirrors what was already being done for notes higher than the "normal" position above the staff.
The distance is marked as obsolete (skipping deprecated status) because it is no longer functional and we normally require deprecated code to be functional.
@rpspringuel
Copy link
Contributor Author

I've pushed the changes to eliminate noteadditionalspacelinestext in favor of the automatic calculation. Tests with extremely low notes are altered slightly, as the default value for that distance was not exactly what the calculation would have predicted. If everything looks good, then this is ready for merge.

@henryso
Copy link
Contributor

henryso commented Feb 15, 2021

Is there a way to set things up so that the spacing of those low notes are not changed? I suspect that some people (like me, for instance) will have scores already typeset and want to retain the old spacing, even if it's wonky.

@rpspringuel
Copy link
Contributor Author

I could probably toggle the behavior with a conditional. Let me look at that after Mass.

Sent with GitHawk

By making use of the `\ifgre@allowdeprecated` conditional, we can allow the user to preserve the old spacing behavior regarding low notes.
@rpspringuel
Copy link
Contributor Author

Okay, I think I've got that worked out. Please look it over and let me know if you see any problems.

@rpspringuel
Copy link
Contributor Author

Note that the associated test PR has changed due to some of the monkey business I did to extract the old test results for the backwards compatibility tests. That process was much harder than it really should have been.

@rpspringuel rpspringuel linked an issue Feb 15, 2021 that may be closed by this pull request
@henryso
Copy link
Contributor

henryso commented Feb 15, 2021

I don't think this is deprecation. I see this as an alternative that doesn't really go away, like the old vs new style oriscus orientation algorithms.

@rpspringuel
Copy link
Contributor Author

Hmmm... That’s going to require a slightly different approach.

Now, currently the additional space added for high notes is calculated while the additional space for low notes is manually set. If we’re going to preserve the ability to manually set the space for low notes, should we add the ability to do so for high notes? If so, should the user be able to independently decide whether to use the manual or automatic setting for low and high notes (requiring two switches with two positions each), or does it suffice to have there be a single manual vs automatic setting?

Sent with GitHawk

@henryso
Copy link
Contributor

henryso commented Feb 16, 2021

I think that there should be a "legacy" switch only for the lower notes as the upper notes have been working as they've been working thus far and no one has been asking for a way to make the high note spacing work like the low note spacing. We'd only need this for the people who have things typeset and laid out and for whom such a change would cause a big mess and a need to re-layout the work.

@rpspringuel
Copy link
Contributor Author

Okay. I can work on that tomorrow.

Sent with GitHawk

@rpspringuel
Copy link
Contributor Author

Converted. I'll be offline on Ash Wednesday so if you spot anything I'll deal with it on Thursday.

@henryso
Copy link
Contributor

henryso commented Feb 17, 2021

Looks OK to me.

@rpspringuel rpspringuel merged commit 4a583d3 into gregorio-project:ctan Feb 18, 2021
@rpspringuel rpspringuel deleted the font-calibration branch February 18, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Low episema overlapping text Magic spaces
2 participants