Skip to content

Fix incorrect lineJoin and pathfactor in get_drawings()#4955

Merged
julian-smith-artifex-com merged 1 commit intopymupdf:mainfrom
sakamotch:fix/linejoin-pathfactor-bug
Apr 16, 2026
Merged

Fix incorrect lineJoin and pathfactor in get_drawings()#4955
julian-smith-artifex-com merged 1 commit intopymupdf:mainfrom
sakamotch:fix/linejoin-pathfactor-bug

Conversation

@sakamotch
Copy link
Copy Markdown
Contributor

Fixes #4954

Changes

Bug 1: lineJoin scaled by pathfactor

stroke->linejoin is an enum (0=Miter, 1=Round, 2=Bevel) but was multiplied by pathfactor. Changed to emit the raw integer value, consistent with how lineCap is already handled.

// Before
Py_BuildValue("f", dev->pathfactor * stroke->linejoin)

// After
Py_BuildValue("i", stroke->linejoin)

Bug 2: pathfactor falls back to 1 for non-uniform scaling

The original calculation only covered uniform scaling (|a|==|d|) and 90° rotation (|b|==|c|). Changed to use sqrt(a² + b²) which handles arbitrary transforms.

// Before
dev->pathfactor = 1;
if (ctm.a != 0 && fz_abs(ctm.a) == fz_abs(ctm.d))
    dev->pathfactor = fz_abs(ctm.a);
else if (ctm.b != 0 && fz_abs(ctm.b) == fz_abs(ctm.c))
    dev->pathfactor = fz_abs(ctm.b);

// After
float scale = sqrtf(ctm.a * ctm.a + ctm.b * ctm.b);
if (scale < 1e-9f)
    scale = sqrtf(ctm.c * ctm.c + ctm.d * ctm.d);
if (scale < 1e-9f)
    scale = 1.0f;
dev->pathfactor = scale;

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 build
  • src_classic/helper-devices.i — classic build
  • src/__init__.py — Python fallback

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@sakamotch
Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Comment thread src/__init__.py Outdated
Comment thread src_classic/helper-devices.i
@julian-smith-artifex-com
Copy link
Copy Markdown
Collaborator

Many thanks for updating things.

Could your squash your 4 commits into 1 commit and rebase on top of current main?

Then i think i'd be ok to merge this PR, after checking with @JorjMcKie.

@sakamotch sakamotch force-pushed the fix/linejoin-pathfactor-bug branch from 3cf53a9 to 659e86b Compare April 9, 2026 11:36
@sakamotch
Copy link
Copy Markdown
Contributor Author

Thanks for the review.
Squashed into one commit and rebased onto current main.

@sakamotch
Copy link
Copy Markdown
Contributor Author

I checked the CI failure more carefully. The determinant-based pathfactor I introduced here is the problem.

I'll change it to:

float sx = sqrtf(ctm.a * ctm.a + ctm.b * ctm.b);
float sy = sqrtf(ctm.c * ctm.c + ctm.d * ctm.d);
float dot = ctm.a * ctm.c + ctm.b * ctm.d;

if (sx == sy && dot == 0.0f)
    dev->pathfactor = sx;
else
    dev->pathfactor = 1.0f;

This extends the old logic from 0/90-degree cases to arbitrary rotation angles, while keeping the same strict fallback behavior.

I’m keeping == intentionally, since the previous implementation already used exact float comparisons.

I tested this locally and tests/test_annots.py::test_redact4 now passes, and tests/test_drawings.py also passes with this change.

@sakamotch
Copy link
Copy Markdown
Contributor Author

@julian-smith-artifex-com If this revised approach looks good, I can update the PR with a new squashed commit.

@julian-smith-artifex-com
Copy link
Copy Markdown
Collaborator

test_redact4 is currently failing due to a regression in mupdf in the latest master branch. So i don't think you should try to get the test to pass here.

Tests are passing with this PR and git branch 1.27.x, so i think the PR is good.

@julian-smith-artifex-com
Copy link
Copy Markdown
Collaborator

[rerunning tests with mupdf master as failure was from mupdf, not thie PR.]

@julian-smith-artifex-com
Copy link
Copy Markdown
Collaborator

I hope you don't mind, but i've taken the liberty of rebasing the PR tree on top of latest main.

Hopefully it will still pass tests, and we can merge the PR.

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 some transform patterns
correctly. Use sqrt(abs(a*d - b*c)) so uniform scaling with arbitrary
rotation is handled correctly; for non-uniform scaling this yields the
geometric mean of the scale factors.

Fixed in:
- src/extra.i (rebased build)
- src/__init__.py (Python fallback)
@sakamotch sakamotch force-pushed the fix/linejoin-pathfactor-bug branch from ed72ff4 to d547cdf Compare April 16, 2026 05:12
@sakamotch
Copy link
Copy Markdown
Contributor Author

Thanks again for taking care of the earlier rebase.

It looks like main moved forward again, so I rebased the PR branch onto the latest main. Hope that's okay, and hopefully it should be mergeable now.

@sakamotch
Copy link
Copy Markdown
Contributor Author

@julian-smith-artifex-com Checks passed, looks mergeable now!

@julian-smith-artifex-com julian-smith-artifex-com merged commit 0f65399 into pymupdf:main Apr 16, 2026
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_drawings() returns incorrect lineJoin and width

3 participants