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

inconsistent value with really low contrast in fontLookupAPCA() #113

Open
jonathanpoelen opened this issue Nov 19, 2023 · 4 comments
Open
Labels
question Further information is requested

Comments

@jonathanpoelen
Copy link

Describe the bug

In fontLookupAPCA() from apca-w3, a contrast less than 5 returns 500, 600, 700, 800 and 900 instead of 999

To Reproduce

for (let i = 0; i < 6; ++i)
  console.log(fontLookupAPCA(i).join(', '));
0.00, 999, 999, 999, 999, 999, 999, 999, 999, 999
1.00, 999, 999, 999, 999, 500, 600, 700, 800, 900
                          ~~~  ~~~  ~~~  ~~~  ~~~
2.00, 999, 999, 999, 999, 500, 600, 700, 800, 900
                          ~~~  ~~~  ~~~  ~~~  ~~~
3.00, 999, 999, 999, 999, 500, 600, 700, 800, 900
                          ~~~  ~~~  ~~~  ~~~  ~~~
4.00, 999, 999, 999, 999, 500, 600, 700, 800, 900
                          ~~~  ~~~  ~~~  ~~~  ~~~
5.00, 999, 999, 999, 999, 999, 999, 999, 999, 999

Expected behavior

All values should be 999.

Additional context

The bug is here: https://github.com/Myndex/apca-w3/blob/master/src/apca-w3.js#L571 which selects index 0 corresponding to a header.

  const factor = 0.2; // 1/5 as LUT is in increments of 5
  const index = (contrast == 0) ?
                 1 : (contrast * factor) | 0 ; // LUT row... n|0 is bw floor

The formula should probably look like

  const index = Math.max(1, (contrast * factor) | 0); // LUT row... n|0 is bw floor

Or else everything is out of sync and it should be

  const index = ((contrast * factor) | 0) + 1; // LUT row... n|0 is bw floor

This does not happen using APCAcontrast since the minimum value returned is greater than 7.3 and less than -7.3.

@jonathanpoelen jonathanpoelen added the bug Something isn't working label Nov 19, 2023
@Myndex Myndex added question Further information is requested and removed bug Something isn't working labels Nov 19, 2023
@Myndex
Copy link
Owner

Myndex commented Nov 19, 2023

Contrasts under $L^c ±10$ are invalid, and fonts cut off at $L^c ±30$.

I'm not sure what you mean when you say "expected value is 999" ?

I think what is going on is there is no $L^c\ 5$ row in the LUT, so the $L^c\ 0$ row is being returned... but values under $L^c ±10$ are to be disregarded in the current -w3 version.

In order to use values under $L^c ±10$ there is a code extention that must be installed.

@jonathanpoelen
Copy link
Author

My aim was to plot the values -108 to 106 using the result of fontLookupAPCA(). Except that in the -5 to 5 range, the curve doesn't make any sense when it could easily be flat:

0.00, 999, 999, 999, 999, 999, 999, 999, 999, 999
1.00, 999, 999, 999, 999, 999, 999, 999, 999, 999
2.00, 999, 999, 999, 999, 999, 999, 999, 999, 999
3.00, 999, 999, 999, 999, 999, 999, 999, 999, 999
4.00, 999, 999, 999, 999, 999, 999, 999, 999, 999
5.00, 999, 999, 999, 999, 999, 999, 999, 999, 999

@Myndex
Copy link
Owner

Myndex commented Nov 20, 2023

Hi @jonathanpoelen

I see, though for apca-w3, there are no valid values under Lc ±7.

But if you are just taking the iterated output of the fontLookupAPCA() function, I understand the issue.

Fix A

The interim fix is either to set the first element of the

const fontMatrixAscend = [
    ['Lc',100,200,300,400,500,600,700,800,900],
    [....

To

const fontMatrixAscend = [
    ['Lc',999,999,999,999,999,999,999,999,999],
    [....

Fix B

Or add a 5 line to all the related arrays.

const fontMatrixAscend = [
    ['Lc',100,200,300,400,500,600,700,800,900],
    [0,999,999,999,999,999,999,999,999,999],
    [5,999,999,999,999,999,999,999,999,999],  //  Insert this line
    [10,999,999,999,999,999,999,999,999...
;

const fontDeltaAscend = [
    ['∆Lc',100,200,300,400,500,600,700,800,900],
    [0,0,0,0,0,0,0,0,0,0],
    [5,0,0,0,0,0,0,0,0,0],  //  Insert this line
    [10,0,0,0,0,0,0,0,0,0],
;

  const contrastArrayAscend = ['lc',0,5,10,15,20,25,30,35,40,45,50,55,60,65,70,75,80,85,90,95,100,105,110,115,120,125];
//                                    ^ insert the 5

Thank you for reporting, it will be corrected in the next update. Please let me know if I can be of further assistance.

@Myndex
Copy link
Owner

Myndex commented Nov 20, 2023

Also just FYI, the lookups will be replaced with a pure algorithmic approach in the not too distant future. The LUTs are for developmental purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants