Skip to content

Fix footprint transform#4280

Open
timon-schelling wants to merge 2 commits into
masterfrom
fix-footprint-transform
Open

Fix footprint transform#4280
timon-schelling wants to merge 2 commits into
masterfrom
fix-footprint-transform

Conversation

@timon-schelling

@timon-schelling timon-schelling commented Jun 24, 2026

Copy link
Copy Markdown
Member

The render node(s) pass along the logical instead of the physical scale as part of the footprint, leading to upstream nodes rendering at the wrong scale and producing textures of the wrong size.

wrong at 2.8x scale
image

correct at 2.8x scale
image

The first Commit is a minimal fix.
The second makes the create context node responsible for converting the render configs footprint to physical scale and then passing that along. Things that explicitly require logical scale like metadata and svg rendering convert back to logical.

Both approaches work, @TrueDoctor any opinions?
I'm slightly in favor of the second.

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

Copy link
Copy Markdown
Contributor

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 refactors the rendering pipeline to consistently manage physical and logical coordinate spaces. It scales the footprint transform by the render scale early in the pipeline and introduces logical transforms where needed across background rendering, caching, and pixel preview nodes. The reviewer suggests a further simplification in render_pixel_preview.rs to remove an unnecessary multiplication by physical_scale when applying the transform to the metadata, since the upstream metadata is already in logical space.

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 on lines 72 to +74
result
.metadata
.apply_transform(footprint.transform * DAffine2::from_translation(upstream_min) * DAffine2::from_scale(DVec2::splat(physical_scale)));
.apply_transform(logical_transform * DAffine2::from_translation(upstream_min) * DAffine2::from_scale(DVec2::splat(physical_scale)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

With upstream_render_params.scale set to 1.0, the upstream metadata is already in logical space, so we no longer need to multiply by physical_scale to undo the mismatch.

Suggested change
result
.metadata
.apply_transform(footprint.transform * DAffine2::from_translation(upstream_min) * DAffine2::from_scale(DVec2::splat(physical_scale)));
.apply_transform(logical_transform * DAffine2::from_translation(upstream_min) * DAffine2::from_scale(DVec2::splat(physical_scale)));
result
.metadata
.apply_transform(logical_transform * DAffine2::from_translation(upstream_min));

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 issues found across 4 files

Confidence score: 2/5

  • In node-graph/nodes/gstd/src/render_cache.rs, unvalidated render_params.scale is used in divisor-based zoom/transform math, so a zero or non-finite scale can propagate invalid values and break cached rendering output for users — validate scale first and fall back to direct render when invalid before computing transforms.
  • In node-graph/nodes/gstd/src/render_background.rs, unconditional 1.0 / render_params.scale can emit non-finite SVG transform values when scale is zero, which can corrupt or fail background rendering — add a scale guard before inversion (or skip/apply a safe default transform) before merging.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="node-graph/nodes/gstd/src/render_background.rs">

<violation number="1" location="node-graph/nodes/gstd/src/render_background.rs:55">
P2: Unconditional `1.0 / render_params.scale` can create non-finite transforms when scale is 0. Guard scale before inversion to avoid invalid SVG transform output.</violation>
</file>

<file name="node-graph/nodes/gstd/src/render_cache.rs">

<violation number="1" location="node-graph/nodes/gstd/src/render_cache.rs:348">
P1: Unvalidated `render_params.scale` is used as a divisor, allowing divide-by-zero/non-finite transform math. Guard invalid scale and fall back to direct render before computing zoom/transforms.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic


let device_scale = render_params.scale;
let zoom = footprint.scale_magnitudes().x;
let zoom = footprint.scale_magnitudes().x / render_params.scale;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: Unvalidated render_params.scale is used as a divisor, allowing divide-by-zero/non-finite transform math. Guard invalid scale and fall back to direct render before computing zoom/transforms.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/render_cache.rs, line 348:

<comment>Unvalidated `render_params.scale` is used as a divisor, allowing divide-by-zero/non-finite transform math. Guard invalid scale and fall back to direct render before computing zoom/transforms.</comment>

<file context>
@@ -345,12 +345,10 @@ pub async fn render_output_cache<'a: 'n>(
 
-	let device_scale = render_params.scale;
-	let zoom = footprint.scale_magnitudes().x;
+	let zoom = footprint.scale_magnitudes().x / render_params.scale;
 	let rotation = footprint.decompose_rotation();
 
</file context>
Suggested change
let zoom = footprint.scale_magnitudes().x / render_params.scale;
let scale = render_params.scale;
if !scale.is_finite() || scale <= 0. {
log::warn!("render_output_cache: invalid render scale ({scale}), falling back to direct render");
let context = OwnedContextImpl::from(ctx.clone()).with_footprint(*footprint).with_vararg(Box::new(render_params.clone()));
return data.eval(context.into_context()).await;
}
let zoom = footprint.scale_magnitudes().x / scale;

} => {
let mut render = SvgRender::new();

let logical_transform = glam::DAffine2::from_scale(glam::DVec2::splat(1.0 / render_params.scale)) * render_params.footprint.transform;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Unconditional 1.0 / render_params.scale can create non-finite transforms when scale is 0. Guard scale before inversion to avoid invalid SVG transform output.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/render_background.rs, line 55:

<comment>Unconditional `1.0 / render_params.scale` can create non-finite transforms when scale is 0. Guard scale before inversion to avoid invalid SVG transform output.</comment>

<file context>
@@ -52,6 +52,8 @@ async fn render_background<'a: 'n>(
 		} => {
 			let mut render = SvgRender::new();
 
+			let logical_transform = glam::DAffine2::from_scale(glam::DVec2::splat(1.0 / render_params.scale)) * render_params.footprint.transform;
+
 			if render_params.viewport_zoom > 0. {
</file context>

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.

1 participant