Skip to content

fix(SVG importer): make origin the center of SVG viewBox - #3758

Open
rodolforg wants to merge 1 commit into
synfig:masterfrom
rodolforg:fix-svg-origin
Open

rodolforg wants to merge 1 commit into
synfig:masterfrom
rodolforg:fix-svg-origin

Conversation

@rodolforg

Copy link
Copy Markdown
Contributor

No description provided.

@rodolforg rodolforg added the SVG label Jun 6, 2026
@codacy-production

codacy-production Bot commented Jun 6, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 9 complexity · 0 duplication

Metric Results
Complexity 9
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for parsing the SVG viewBox attribute into a synfig::Rect member variable (view_box_), replacing the previous manual ox and oy offset calculations. However, several critical issues were identified in the review: a trailing comma in the constructor initializer list will cause a compilation error, there is a unit mismatch when initializing view_box_ depending on whether viewBox is specified, and the coordinate transformations in coor2vect and the canvas view-box output do not correctly center the coordinates or account for the minx and miny offsets of the viewBox.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread synfig-core/src/modules/mod_svg/svg_parser.cpp
Comment on lines +255 to +260
if (approximate_zero(view_box_.get_width())) {
view_box_.maxx = view_box_.minx + width / kux;
}
if (approximate_zero(view_box_.get_height())) {
view_box_.maxy = view_box_.miny + height / kux; // kuy
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There is a critical unit mismatch and coordinate system misalignment in how view_box_ is handled:

  1. Unit Mismatch: If viewBox is specified in the SVG, view_box_ is stored in SVG user units (pixels). If viewBox is not specified, you are dividing by kux here, storing it in Synfig units. This makes view_box_ have inconsistent units.
  2. Coordinate Misalignment: In coor2vect, you are centering coordinates around 0,0 using view_box_.get_width() / 2. However, the Synfig canvas view-box is set to [view_box_.minx, view_box_.miny, view_box_.maxx, view_box_.maxy] (which is not centered around 0,0 if minx and miny are 0). This causes all imported elements to be placed completely outside the visible area of the canvas.

To fix this, view_box_ should always be kept in SVG user units (pixels). Then, the Synfig canvas view-box should be centered around 0,0 with a width and height scaled by kux.

		if (approximate_zero(view_box_.get_width())) {
			view_box_.maxx = view_box_.minx + width;
		}
		if (approximate_zero(view_box_.get_height())) {
			view_box_.maxy = view_box_.miny + height; // kuy
		}

Comment thread synfig-core/src/modules/mod_svg/svg_parser.cpp Outdated
Comment thread synfig-core/src/modules/mod_svg/svg_parser.cpp Outdated
@rodolforg

Copy link
Copy Markdown
Contributor Author

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026. For more details on the timeline and next steps, please review the Help Documentation.

Amen 🙏

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants