Fix precision rounding issues in LineWrapper

[hollandjake]

What kind of change does this PR introduce?

Similar to #1583 this fixes the rounding errors occurring during text width calculations

Checklist:

• Unit Tests
• Documentation N/A
• Update CHANGELOG.md
• Ready to be merged

Closes #1611

[hollandjake]

All these issues with rounding does raise the question as to whether we should be compliant with the spec and use 32bit numbers everywhere. but thats one for the maintainers to decide

see Annex C.2

[hollandjake]

Moved back to draft as in even more obscure cases im still noticing line wrapping when it shouldnt. I will come back with a more comprehensive PR that will rework this.

feel free to close in the mean time

[blikblum]

In changelog there's an entry saying that the rounding issues are fixed. Is is accurate?

[hollandjake]

@blikblum they were partially fixed. fixed on the Y axis, there is however issues on the X axis which this PR aims to fix. however the solution is not as trivial and raises issues regarding how we handle rounding

[hollandjake]

Now fixed on the X axis too (note this modifies wordWidth, although this should be only used internally anyway) it forces numbers to be rounded down (into 32bit float) before being compared with other numbers ensuring random numerical precision doesn't introduce weird behaviour

[hollandjake]

All these issues with rounding does raise the question as to whether we should be compliant with the spec and use 32bit numbers everywhere. but thats one for the maintainers to decide

see Annex C.2

In response to this, we have now forced the text calculations to be float32 rather than float64, the difference with extremely small numbers will not be noticeable on actual documents, however the text flow will be much more expected

[blikblum]

Is this function called with roundUp = true?

[hollandjake]

by default it will roundDown

[blikblum]

I think we could simplify and only implement roundDown. If needed later implement roundUp

[hollandjake]

only reason i left both was that if we want to do float64 > float32 it would need to be rounded up

[blikblum]

Should not be rounded only when storing the number to the pdf file?

wordWidth is called in many intermediate steps.

[hollandjake]

we need to ensure that during the line_wrapper computations we are keeping a float32 number so we could either wrap all instances of wordWidth with the rounder, or just do it here

[hollandjake]

or alternatively we wrap every comparison operation with PDFNumber to ensure the rounding never affects it

[blikblum]

or alternatively we wrap every comparison operation with PDFNumber to ensure the rounding never affects it

Lets take the current approach, adding tests to ensure correctness. Later we can try to minimize number of roundings

[blikblum]

note this modifies wordWidth, although this should be only used internally anyway

pdfmake uses a lot of pdfkit internals

@liborm85 any thoughts on this?

[blikblum]

Whats the output running with master?

It add ellipsis?

[hollandjake]

oh you meant that as a test, you had commented on #1613 😅 , ill correct now and ill post what it does on master

[blikblum]

You can keep this also

[hollandjake]

if we remove the PDFNumber wrapper in the wordWidth method (AKA current master)

[hollandjake]

Failing due to miniscule pixel differences between OS versions (I see the same thing on macos with vector-spec-js-vector-complex-svg-1-snap). Can someone else generate the images for me

[blikblum]

Failing due to miniscule pixel differences between OS versions (I see the same thing on macos with vector-spec-js-vector-complex-svg-1-snap). Can someone else generate the images for me

It occurred in my env also. Maybe there's a way to configure the pixel difference tolerance

[hollandjake]

Failing due to miniscule pixel differences between OS versions (I see the same thing on macos with vector-spec-js-vector-complex-svg-1-snap). Can someone else generate the images for me

It occurred in my env also. Maybe there's a way to configure the pixel difference tolerance

we can set it based on num pixels or percentage, @blikblum which would you prefer and what value

[blikblum]

we can set it based on num pixels or percentage, @blikblum which would you prefer and what value

The minimum to pass and specific to this test.

[liborm85]

note this modifies wordWidth, although this should be only used internally anyway

pdfmake uses a lot of pdfkit internals

@liborm85 any thoughts on this?

That's fine, pdfmake does not use this LineWrapper.

[blikblum]

@hollandjake

I created a unit test for bounded text. Visual tests should be used only when strictly necessary

I added a new matcher toContainText

Also removed the round up code path since is not being used currently. Please review if its correct

For the record if deems necessary to use round up, here is the original code

const fArray = new Float32Array(1);
const uArray = new Uint32Array(fArray.buffer);

export function PDFNumber(n, roundUp = false) {
  // PDF numbers are strictly 32bit
  // so convert this number to a 32bit number
  // @see ISO 32000-1 Annex C.2 (real numbers)
  const rounded = Math.fround(n);
  if (roundUp) {
    if (rounded >= n) return rounded;
  } else if (rounded <= n) return rounded;

  // Will have to perform 32bit float truncation
  fArray[0] = n;

  // Get the 32-bit representation as integer and shift bits
  if (!(roundUp ^ (n > 0))) {
    uArray[0] += 1;
  } else {
    uArray[0] -= 1;
  }

  // Return the float value
  return fArray[0];
}

 test.each([
    [0],
    [0.04999999701976776], //float32 rounded down
    [0.05],
    [0.05000000074505806], //float32 rounded up
    [1],
    [-1],
    [-5.05],
    [5.05],
  ])('PDFNumber(%f, true) -> %f', (n) => {
    expect(PDFNumber(n, true)).toBeGreaterThanOrEqual(n);
  });

[hollandjake]

@blikblum lgtm