feat: add a texture fill mode that doesn't stretch the texture#56
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds texture fitting modes ( Changes
Sequence DiagramsequenceDiagram
actor Shape as Shape Definition
participant RenderPass as Render Pipeline
participant UVCalc as UV Scale Calculator
participant InstanceData as Instance Data
participant Shader as GPU Shader
Shape->>RenderPass: Configure texture with fit mode (OriginalSize)
RenderPass->>UVCalc: Request UV scales for layer
UVCalc->>UVCalc: Calculate scale = texture_size / mapping_size
UVCalc-->>RenderPass: Return per-layer UV scales [x, y]
RenderPass->>InstanceData: Create InstanceTextureData<br/>(texture_ids, uv_scales)
RenderPass->>Shader: Pass InstanceMetadata with<br/>texture_uv_scale_layer0/1
Shader->>Shader: Compute layer0_tex_coords = uv \\* scale_layer0<br/>Compute layer1_tex_coords = uv \\* scale_layer1
Shader->>Shader: Sample textures at scaled UVs
Shader-->>RenderPass: Output pixels with<br/>correctly fitted textures
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/visual_regression.rs (1)
16-31: Make pixel assertions size-aware instead of open-coding the non-default path.Now that the renderer helper accepts arbitrary
physical_size,assert_pixels_match()being hard-wired toCANVAS_WIDTH/CANVAS_HEIGHTforced this test to duplicate the failure formatting. Passing(width, height)into the helper keeps custom-size regressions on the same assertion path and avoids future drift.♻️ Suggested refactor
-fn assert_pixels_match(pixel_buffer: &[u8], expectations: &[grafo_test_scenes::PixelExpectation]) { - let failures = check_pixels(pixel_buffer, CANVAS_WIDTH, CANVAS_HEIGHT, expectations); +fn assert_pixels_match( + pixel_buffer: &[u8], + size: (u32, u32), + expectations: &[grafo_test_scenes::PixelExpectation], +) { + let failures = check_pixels(pixel_buffer, size.0, size.1, expectations); if !failures.is_empty() { let message = format!( "{} pixel expectation(s) failed:\n{}", @@ - assert_pixels_match(&pixel_buffer, &expectations); + assert_pixels_match(&pixel_buffer, (CANVAS_WIDTH, CANVAS_HEIGHT), &expectations); @@ - let failures = check_pixels( - &pixel_buffer, - physical_size.0, - physical_size.1, - &expectations, - ); - if !failures.is_empty() { - let message = format!( - "{} pixel expectation(s) failed:\n{}", - failures.len(), - failures.join("\n"), - ); - panic!("{message}"); - } + assert_pixels_match(&pixel_buffer, physical_size, &expectations);Also applies to: 122-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/visual_regression.rs` around lines 16 - 31, The test duplicates failure formatting because assert_pixels_match is hard-coded to CANVAS_WIDTH/CANVAS_HEIGHT instead of using the physical_size passed to create_headless_renderer_with_size_and_scale; update the test(s) (including the failure path currently duplicating formatting) to call assert_pixels_match with the actual (width, height) used for the renderer rather than relying on CANVAS_WIDTH/CANVAS_HEIGHT, and adjust any helper callsites so create_headless_renderer_with_size_and_scale and the assertion path share the same (physical_size) tuple to keep custom-size regressions on the common assertion logic (references: create_headless_renderer_with_size_and_scale, assert_pixels_match, CANVAS_WIDTH/CANVAS_HEIGHT).src/shape.rs (1)
111-139: Extract one shared bounds helper for path UVs andtexture_mapping_size.
compute_texture_mapping_size()re-runs the same AABB pass thatPathShape::tessellate_into_buffers()already uses to generatetex_coords. These two calculations now define the same mapping contract, so letting them drift will desynchronizeOriginalSizescaling from the actual vertex UVs. Please derive both from one helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shape.rs` around lines 111 - 139, compute_texture_mapping_size() duplicates the AABB pass used in PathShape::tessellate_into_buffers() (which produces tex_coords) causing potential drift; extract a single helper (e.g. compute_vertex_bounds or compute_aabb_from_vertices) that takes &VertexBuffers<CustomVertex, u16> or &[CustomVertex] and returns the computed bounds or size (min_x,min_y,max_x,max_y or width,height). Replace the AABB loop in compute_texture_mapping_size() with a call to that helper to derive the size (clamped with .max(1e-6) as before) and update PathShape::tessellate_into_buffers() to call the same helper when generating tex_coords and when computing OriginalSize so both UVs and mapping_size come from the same bounds.src/shaders/shader.wgsl (1)
22-24: Optional: pack per-layer UVs into a singlevec4varying.
layer0_tex_coordsandlayer1_tex_coordseach occupy a varying interpolator slot. Packing them asvec4<f32>(xy = layer0, zw = layer1) saves one interpolator per pipeline and keeps the vertex/fragment interfaces a bit tighter, which is helpful givenGradientVertexOutputalready uses 7 locations. Same applies totexture_uv_scale_layer0/1on the vertex input — they can be a singlevec4attribute. Purely an optimization; functionally equivalent.Also applies to: 30-33, 39-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shaders/shader.wgsl` around lines 22 - 24, Pack per-layer UV varyings and per-layer UV scale vertex inputs into vec4s to save an interpolator: replace separate varyings layer0_tex_coords and layer1_tex_coords with a single varying (e.g., layer_tex_coords: vec4<f32> where xy = layer0, zw = layer1) and update all uses to swizzle (.xy / .zw); likewise replace texture_uv_scale_layer0 and texture_uv_scale_layer1 with a single attribute (e.g., texture_uv_scale_layers: vec4<f32>) and update references to use .xy and .zw. Update the GradientVertexOutput / corresponding vertex input/output structs and any places that read those symbols so types and swizzles match and remove the now-unused individual fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shaders/shader.wgsl`:
- Around line 329-331: Remove the dead helper function
texture_coords_are_in_bounds from src/shaders/shader.wgsl: locate the fn
texture_coords_are_in_bounds(tex_coords: vec2<f32>) -> bool { ... } and delete
it entirely (it is unused and bounds checking is handled by ClampToEdge +
transparent padding), and verify no other shader code references this symbol
before committing.
In `@src/shape.rs`:
- Around line 1087-1098: The docstring for the ShapeTextureFitMode::OriginalSize
variant is out of date: it claims sampling outside the original texture uses
clamp-to-edge and recommends a transparent 1px padding, but the implemented
behavior (and tests in grafo-test-scenes and visual_regression) reveals the
underlying fill/background outside the texture footprint. Update the
ShapeTextureFitMode::OriginalSize documentation to state that one texture texel
maps to one physical pixel before transform, translation is ignored while
scale/rotation/perspective apply, and importantly that samples outside the
original texture footprint do not clamp but instead allow the underlying
fill/background to show (remove the padding recommendation and clarify expected
compositing behavior).
---
Nitpick comments:
In `@src/shaders/shader.wgsl`:
- Around line 22-24: Pack per-layer UV varyings and per-layer UV scale vertex
inputs into vec4s to save an interpolator: replace separate varyings
layer0_tex_coords and layer1_tex_coords with a single varying (e.g.,
layer_tex_coords: vec4<f32> where xy = layer0, zw = layer1) and update all uses
to swizzle (.xy / .zw); likewise replace texture_uv_scale_layer0 and
texture_uv_scale_layer1 with a single attribute (e.g., texture_uv_scale_layers:
vec4<f32>) and update references to use .xy and .zw. Update the
GradientVertexOutput / corresponding vertex input/output structs and any places
that read those symbols so types and swizzles match and remove the now-unused
individual fields.
In `@src/shape.rs`:
- Around line 111-139: compute_texture_mapping_size() duplicates the AABB pass
used in PathShape::tessellate_into_buffers() (which produces tex_coords) causing
potential drift; extract a single helper (e.g. compute_vertex_bounds or
compute_aabb_from_vertices) that takes &VertexBuffers<CustomVertex, u16> or
&[CustomVertex] and returns the computed bounds or size (min_x,min_y,max_x,max_y
or width,height). Replace the AABB loop in compute_texture_mapping_size() with a
call to that helper to derive the size (clamped with .max(1e-6) as before) and
update PathShape::tessellate_into_buffers() to call the same helper when
generating tex_coords and when computing OriginalSize so both UVs and
mapping_size come from the same bounds.
In `@tests/visual_regression.rs`:
- Around line 16-31: The test duplicates failure formatting because
assert_pixels_match is hard-coded to CANVAS_WIDTH/CANVAS_HEIGHT instead of using
the physical_size passed to create_headless_renderer_with_size_and_scale; update
the test(s) (including the failure path currently duplicating formatting) to
call assert_pixels_match with the actual (width, height) used for the renderer
rather than relying on CANVAS_WIDTH/CANVAS_HEIGHT, and adjust any helper
callsites so create_headless_renderer_with_size_and_scale and the assertion path
share the same (physical_size) tuple to keep custom-size regressions on the
common assertion logic (references:
create_headless_renderer_with_size_and_scale, assert_pixels_match,
CANVAS_WIDTH/CANVAS_HEIGHT).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b521a577-f1b3-4a88-bc3c-4149ed2386da
📒 Files selected for processing (9)
grafo-test-scenes/src/scene.rssrc/renderer/draw_queue.rssrc/renderer/preparation.rssrc/renderer/traversal.rssrc/shaders/shader.wgslsrc/shape.rssrc/texture_manager.rssrc/vertex.rstests/visual_regression.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
OriginalSizetexture fitting mode that preserves original texture dimensions instead of stretching to fill shapes.