Skip to content

fix: preserve aspect-correct viewport for screen effects#1213

Open
xXJSONDeruloXx wants to merge 1 commit intoutkarshdalal:masterfrom
xXJSONDeruloXx:fix/screen-effects-aspect
Open

fix: preserve aspect-correct viewport for screen effects#1213
xXJSONDeruloXx wants to merge 1 commit intoutkarshdalal:masterfrom
xXJSONDeruloXx:fix/screen-effects-aspect

Conversation

@xXJSONDeruloXx
Copy link
Copy Markdown
Contributor

@xXJSONDeruloXx xXJSONDeruloXx commented Apr 14, 2026

Description

prevents screen effects from incorrectly altering aspect ratio when applied, fix up from internal and external resolution decouple with FSR work

Recording

image

Checklist

  • If I have access to #code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.
  • I have attached a recording of the change.
  • I have read and agree to the contribution guidelines in CONTRIBUTING.md.

Summary by cubic

Preserves the aspect-correct viewport when screen effects are enabled. Marks the viewport as dirty so the renderer restores the scene viewport each frame and non-scaling effects no longer stretch to the full surface.

  • Bug Fixes
    • In EffectComposer.render(), call renderer.setViewportNeedsUpdate(true) after binding the scene FBO and setting glViewport, so GLRenderer.drawScene() reapplies the aspect-correct viewport after effect passes override it.
    • Fixes stretching with non-scaling effects (CRT/NTSC/Vivid).

Written for commit c060ba7. Summary will update on new commits.

Summary by CodeRabbit

  • Bug Fixes
    • Improved viewport handling during scene rendering to ensure proper display updates in the graphics pipeline.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 14, 2026

📝 Walkthrough

Walkthrough

A single-line addition to EffectComposer.render() explicitly marks the renderer's viewport as needing an update immediately after binding the scene framebuffer and setting the viewport. This ensures viewport synchronization during scene rendering setup.

Changes

Cohort / File(s) Summary
Viewport Update Timing
app/src/main/java/com/winlator/renderer/EffectComposer.java
Added renderer.setViewportNeedsUpdate(true); call immediately after binding scene buffer and setting viewport, ensuring viewport synchronization during scene rendering initialization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A viewport update, small but true,
Placed just right in the render queue,
When scenes are bound and pixels align,
The renderer knows—everything's fine! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: preserving aspect-correct viewport when screen effects are applied.
Description check ✅ Passed PR description includes clear issue explanation, recording/image, and completed checklist items, though Type of Change section is missing.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@xXJSONDeruloXx xXJSONDeruloXx force-pushed the fix/screen-effects-aspect branch from 9d08c33 to c060ba7 Compare April 14, 2026 03:34
@xXJSONDeruloXx xXJSONDeruloXx marked this pull request as ready for review April 14, 2026 10:25
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

No issues found across 1 file

Copy link
Copy Markdown
Contributor

@phobos665 phobos665 left a comment

Choose a reason for hiding this comment

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

Seems sensible. Thanks for fixing.

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.

2 participants