Skip to content

fix: group abs_transform should only need to apply transform once#1037

Draft
fundon wants to merge 4 commits intolinebender:mainfrom
fundon:fix_group_abs_transform_applies_transform_twice
Draft

fix: group abs_transform should only need to apply transform once#1037
fundon wants to merge 4 commits intolinebender:mainfrom
fundon:fix_group_abs_transform_applies_transform_twice

Conversation

@fundon
Copy link
Copy Markdown

@fundon fundon commented Mar 19, 2026

If there is transform on the use element, the abs_transform on the group will be applied transform twice.

This will not cause problems in resvg rendering, but if it is parsed through usvg, the abs_transform obtained is incorrect, which will cause the custom rendering to be incorrect.

The correct scale should be 0.5, not 0.25.

let old_abs_transform = parent.abs_transform;
parent.abs_transform = parent.abs_transform.pre_concat(transform);

let abs_transform = parent.abs_transform.pre_concat(transform);

  • Original SVG

navigation-icon_full

  • Before
Screenshot 2026-03-19 at 5 30 43 PM
  • After
Screenshot 2026-03-19 at 5 31 53 PM

@fundon fundon force-pushed the fix_group_abs_transform_applies_transform_twice branch from 78fd3dc to 1df4c36 Compare March 19, 2026 09:38
@fundon fundon force-pushed the fix_group_abs_transform_applies_transform_twice branch from 4f8261a to ccbbc68 Compare March 19, 2026 09:45
@fundon fundon force-pushed the fix_group_abs_transform_applies_transform_twice branch from ccbbc68 to 10b2658 Compare March 19, 2026 10:09
@LaurenzV
Copy link
Copy Markdown
Collaborator

This change was explicitly introduced in 09338a1 to solve #722. So I'm not entirely sure whether this is the right fix. It does seem to me like the absolute transform of group2 should be translate(20, 20), since the transform is on the node itself? Or am I missing something?

@LaurenzV
Copy link
Copy Markdown
Collaborator

I agree that a scale of 0.25 does seem wrong here, but I'm not sure if the fix you applied is the right one.

@fundon
Copy link
Copy Markdown
Author

fundon commented Mar 19, 2026

Let me check the context. Thank you.

@RazrFalcon
Copy link
Copy Markdown
Collaborator

If it does not affect resvg then why some tests have changed?

@fundon fundon marked this pull request as draft March 23, 2026 11:26
@fundon
Copy link
Copy Markdown
Author

fundon commented Mar 23, 2026

If it does not affect resvg then why some tests have changed?

I have no clue. Is it an error caused by the latest oxipng?

@LaurenzV
Copy link
Copy Markdown
Collaborator

Can you try restoring the old snapshots and see whether tests still pass?

@fundon
Copy link
Copy Markdown
Author

fundon commented Mar 23, 2026

Can you try restoring the old snapshots and see whether tests still pass?

Tried. These use cases all have several pixel differences.

@LaurenzV
Copy link
Copy Markdown
Collaborator

Ah ok. Well, I haven't gotten to reviewing this yet, but if it's just a very small diff it's probably fine and due to some floating-point stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants