Fix incorrect lineJoin and pathfactor in get_drawings()#4955
Fix incorrect lineJoin and pathfactor in get_drawings()#4955sakamotch wants to merge 4 commits intopymupdf:mainfrom
Conversation
Bug 1: lineJoin was multiplied by pathfactor. stroke->linejoin is an enum (0=Miter, 1=Round, 2=Bevel) and should not be scaled. Note that lineCap is already correctly handled as plain integers without pathfactor multiplication. Bug 2: pathfactor calculation only handled uniform scaling (|a|==|d|) and 90-degree rotation (|b|==|c|). For non-uniform scaling, pathfactor fell back to 1, making width incorrect. Use sqrt(a²+b²) to handle arbitrary transforms. Fixed in: - src/extra.i (rebased build) - src_classic/helper-devices.i (classic build) - src/__init__.py (Python fallback)
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
src/__init__.py
Outdated
| scale = math.sqrt(ctm.a ** 2 + ctm.b ** 2) | ||
| if scale < 1e-9: | ||
| scale = math.sqrt(ctm.c ** 2 + ctm.d ** 2) | ||
| if scale < 1e-9: | ||
| scale = 1.0 | ||
| dev.pathfactor = scale |
There was a problem hiding this comment.
Thanks for creating this PR. I have a couple of questions:
How come we convert very small scales (<1e-9) to 1 instead of zero or 1e-9? Do we divide by the scale elsewhere?
It seems a little strange to replaces one partial expression (which uses .a and .d, or .b and .c, or none of them) with a different partial expression (which uses .a and .b, or .c and .d, or none of them).
Could we instead start with the determinant as a scalar representing the matrix? The determinant is how a matrix scales areas, which suggests we use sqrt(abs(determinant)) as the matrix scale, i.e. math.sqrt(abs(ctm.a*ctm.d-ctm.b*ctm.c)).
There was a problem hiding this comment.
Thanks for the suggestion! I'm now testing the determinant-based approach with my PDFs. Will follow up with the results.
There was a problem hiding this comment.
Verified with my PDFs, everything looks good!
The determinant-based approach correctly handles uniform scaling with arbitrary rotation angles, not just 0° and 90° like the original code:
For a uniform scale s with any rotation angle θ (where a = s·cosθ, b = s·sinθ, c = -s·sinθ, d = s·cosθ):
sqrt(|a·d - b·c|) = sqrt(s²·cos²θ + s²·sin²θ) = s✓
For non-uniform scaling, it gives the geometric mean of the two scale factors, which is a reasonable approximation.
For degenerate matrices (zero determinant), pathfactor becomes 0, meaning width=0. Since strokes are invisible in this case, no fallback to 1 is needed.
Does this look good to you?
| DICT_SETITEMSTR_DROP(dev_pathdict, "lineJoin", Py_BuildValue("f", (float)stroke->linejoin)); | ||
| if (!PyDict_GetItemString(dev_pathdict, "closePath")) { | ||
| DICT_SETITEMSTR_DROP(dev_pathdict, "closePath", JM_BOOL(0)); | ||
| } |
There was a problem hiding this comment.
src_classic/ is no longer used and is only retained in git as a reference (we should probably remove it) and probably does not build these days.
So it's probably best to remove this part of the patch.
Use sqrt(abs(a*d - b*c)) for pathfactor calculation as suggested in review. The determinant correctly represents how the matrix scales areas, so its square root gives a meaningful scalar scale. Remove src_classic/helper-devices.i changes since src_classic/ is no longer used.
2 participants
Fixes #4954
Changes
Bug 1:
lineJoinscaled bypathfactorstroke->linejoinis an enum (0=Miter, 1=Round, 2=Bevel) but was multiplied bypathfactor. Changed to emit the raw integer value, consistent with howlineCapis already handled.Bug 2:
pathfactorfalls back to 1 for non-uniform scalingThe original calculation only covered uniform scaling (
|a|==|d|) and 90° rotation (|b|==|c|). Changed to usesqrt(a² + b²)which handles arbitrary transforms.Note: For non-uniform scaling, stroke width is direction-dependent in general. This fix approximates it using the length of the transformed unit vector.
Files changed
src/extra.i— rebased buildsrc_classic/helper-devices.i— classic buildsrc/__init__.py— Python fallback