Fix footprint transform#4280
Conversation
There was a problem hiding this comment.
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.
| 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))); |
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 2/5
- In
node-graph/nodes/gstd/src/render_cache.rs, unvalidatedrender_params.scaleis 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, unconditional1.0 / render_params.scalecan 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; |
There was a problem hiding this comment.
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>
| 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; |
There was a problem hiding this comment.
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>
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

correct at 2.8x scale

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.