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

\font-feature #1002

Merged
merged 11 commits into from
Mar 2, 2021
Merged

\font-feature #1002

merged 11 commits into from
Mar 2, 2021

Conversation

ctrlcctrlv
Copy link
Member

Closes #992.

0bbb1a3 adds SU.dumpString ;
d809267 fixes an unreported bug I noticed related to #994 ;
541404b is the bulk of the work and adds \font-feature itself;
24dc59a adds tests for \font-feature.

This is my first PR. Hopefully first of many. Let's see how it goes. 🎲

@ctrlcctrlv
Copy link
Member Author

OK, I don't know how to fix the failing test. SILE does not output OpenType features in a defined order. This doesn't matter normally: +dlig;+hlig and +hlig;+dlig are equivalent.

osboxes@osboxes:~/Workspace/sile/tests$ sile -b debug font-features.sil 
This is SILE v0.10.9.r36-gf16af44
<font-features.sil>
[1] 
osboxes@osboxes:~/Workspace/sile/tests$ diff font-features.debug font-features.expected 
39c39
< Set font 	Gentium Plus;24;400;;normal;+hlig;+dlig;LTR
---
> Set font 	Gentium Plus;24;400;;normal;+dlig;+hlig;LTR
61c61
< Set font 	Gentium Plus;24;400;;normal;+hlig;+dlig;;LTR
---
> Set font 	Gentium Plus;24;400;;normal;+dlig;+hlig;;LTR
osboxes@osboxes:~/Workspace/sile/tests$ sile -b debug font-features.sil 
This is SILE v0.10.9.r36-gf16af44
<font-features.sil>
[1] 
osboxes@osboxes:~/Workspace/sile/tests$ diff font-features.debug font-features.expected 
39c39
< Set font 	Gentium Plus;24;400;;normal;+hlig;+dlig;LTR
---
> Set font 	Gentium Plus;24;400;;normal;+dlig;+hlig;LTR
61c61
< Set font 	Gentium Plus;24;400;;normal;+hlig;+dlig;;LTR
---
> Set font 	Gentium Plus;24;400;;normal;+dlig;+hlig;;LTR
79c79
< Set font 	Gentium Plus;10;400;;normal;+cv81;+cv75;LTR
---
> Set font 	Gentium Plus;10;400;;normal;+cv75;+cv81;LTR
107c107
< Set font 	Gentium Plus;18;400;;normal;+cv81;+cv75;;LTR
---
> Set font 	Gentium Plus;18;400;;normal;+cv75;+cv81;;LTR
110c110
< Set font 	Gentium Plus;10;400;;normal;+cv81;+cv75;-cv75;+cv81;LTR
---
> Set font 	Gentium Plus;10;400;;normal;+cv75;+cv81;-cv75;+cv81;LTR
117c117
< Set font 	Gentium Plus;10;400;;normal;+cv81;+cv75;LTR
---
> Set font 	Gentium Plus;10;400;;normal;+cv75;+cv81;LTR
120c120
< Set font 	Gentium Plus;10;400;;normal;+cv81;+cv75;+cv75;-cv81;LTR
---
> Set font 	Gentium Plus;10;400;;normal;+cv75;+cv81;+cv75;-cv81;LTR
140c140
< Set font 	Gentium Plus;18;400;;normal;+cv81;+cv75;;LTR
---
> Set font 	Gentium Plus;18;400;;normal;+cv75;+cv81;;LTR
143c143
< Set font 	Gentium Plus;10;400;;normal;+cv81;+cv75;-cv75;-cv81;LTR
---
> Set font 	Gentium Plus;10;400;;normal;+cv75;+cv81;-cv75;-cv81;LTR

Fixing the failing linter stuff is easy and I'm pushing that now.

@alerque
Copy link
Member

alerque commented Jul 31, 2020

The debug output backend should sort the OT features so that doesn't happen.

It might take a little bit to give this all a shakedown. I see some things that probably can be simplified a bit, but I want to actually put it through some paces before commenting on those and I'm knee-deep in JIT issues. In the mean time a couple low hanging fruit...

core/utilities.lua Outdated Show resolved Hide resolved
packages/features.lua Outdated Show resolved Hide resolved
tests/font-features.sil Show resolved Hide resolved
packages/features.lua Outdated Show resolved Hide resolved
@ctrlcctrlv ctrlcctrlv requested a review from alerque August 6, 2020 23:38
@alerque
Copy link
Member

alerque commented Aug 7, 2020

The Lua 5.1 builds are failing by chance or because of implementation differences that affect the non-deterministic table output. We need to sort the feature list output by the debugger so this doesn't happen and tests don't semi-randomly break.

tests/font-features.expected Outdated Show resolved Hide resolved
@ctrlcctrlv
Copy link
Member Author

@alerque I don't know enough about SILE internals to fix this myself. core/debug-output.lua calls out to SILE.font._key (core/font.lua) which just uses options.features; meaning, I think, the very parser needs to change to always give options in the same order. Is this right?

@ctrlcctrlv
Copy link
Member Author

Another solution might be, in core/debug-output.lua's setFont, break the array each time + becomes -, and then sort the sub-arrays alphabetically and glue them back together.

So:

1 +cv80;+cv75;-cv80;+dlig;-cv80;-cv75

2 {+cv75, +cv80}, {-cv80}, {+dlig}, {-cv75, -cv80}

3 +cv75;+cv80;-cv80;+dlig;-cv75;-cv80

Or, if we want to be really clever, we can read left-to-right and "cancel out" a plus and a minus, as:

1 +cv80;+cv75;-cv80;+dlig;-cv80;-cv75

2 +dlig;-cv80

3 -cv80;+dlig # alphabetized by second byte

Which do you prefer, my first solution (change all options orders), or one of these?

@alerque
Copy link
Member

alerque commented Aug 11, 2020

I squashed together some of the commits that did something and then undid the same thing, then simplified and cleaned up a bit. The Lua ordering problem isn't fixed yet and I'm honestly not sure what the best approach is, but this should be a little bit closer to being mergable when we get that bit ready.

@alerque alerque added this to the v0.10.10 milestone Aug 11, 2020
@alerque alerque added the enhancement Software improvement or feature request label Aug 11, 2020
@alerque
Copy link
Member

alerque commented Aug 11, 2020

I had a look at the trouble sorting the debug output and I'm pretty sure that is just a symptom of a deeper problem. It's not just the debug output that is wrong, it's potentially the actual output too. The debug backend is just dumping the same unsorted array of features that the real font selector is using. The debug output shouldn't have all the duplicated keys or an ordering problem — it shouldn't have that data in the first place. What needs fixing is the actual data ending up in the feature option array, not just its debug representation.

@alerque alerque modified the milestones: v0.10.10, v0.10.11 Aug 14, 2020
@alerque alerque modified the milestones: v0.10.11, v0.10.12 Sep 25, 2020
@alerque alerque modified the milestones: v0.10.12, v0.10.13 Oct 10, 2020
@alerque alerque modified the milestones: v0.10.13, v0.10.x Nov 30, 2020
@alerque alerque removed this from the v0.10.14 milestone Feb 3, 2021
@alerque alerque added this to the v0.10.x milestone Feb 3, 2021
@alerque
Copy link
Member

alerque commented Feb 24, 2021

Another solution might be, in core/debug-output.lua's setFont, break the array each time + becomes -, and then sort the sub-arrays alphabetically and glue them back together.

The SILE setting is a string, it shouldn't be troubled with sorting a table as part of the debug output, the problem is where the setting is getting set as a table, not where it's being debugged.

Or, if we want to be really clever, we can read left-to-right and "cancel out" a plus and a minus, as

I don't think we can do that, these might not actually cancel. It's possible for +dlig-dlig to be different than an empty value (for a default-on feature an empty string leaves it on, turning it on explicitly and then off has the effect of turning it off).

EDIT I fixed this for you. You already were doing this 'clever' version internally, it's only when you converted the table to a string that you didn't sort the keys. This wasn't part of the debug functionality at all, it's was where you were setting the SILE setting.

@alerque
Copy link
Member

alerque commented Feb 24, 2021

Hey @ctrlcctrlv would you be available to give this branch a quick spin again? I overhauled the Lua code a bit (okay a lot). Behind the scenes the way the functions iterate and save things is almost completely different. The output is close to the same, but not quite. Notably it now de-duplicates new options being set with those already in the font features setting, not just those in the current option list. Nesting commands and duplicating some features now does not result in a cluttered list in the settings. Also it goes ahead and saves negatives in case they are pertinent, not totally canceling out positives by not mentioning the feature at all.

If this still does what you expect I think we can merge since the tests are now deterministic.

@alerque alerque force-pushed the issue992 branch 2 times, most recently from 1c37541 to f547f30 Compare February 24, 2021 20:00
@alerque
Copy link
Member

alerque commented Mar 2, 2021

In the interest of this being in the next release, I'm going to assume the answer is yes. Since there aren't any breaking changes either way only a new package the worst case scenario is that the function isn't satisfactory and needs to be patched. I expect the next release cycle to be relatively short because I'm pushing to get math stuff included, but I really want to shake out some changes to the release cycle first before math lands so that important release has a better chance of being a smooth rollout.

@alerque alerque merged commit f2d15d8 into sile-typesetter:master Mar 2, 2021
@alerque alerque deleted the issue992 branch March 2, 2021 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Software improvement or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

\add-font-feature; \remove-font-feature. Why not \font-feature?
3 participants