Skip to content

Subviewport render fix#1303

Merged
Arctis-Fireblight merged 2 commits into
Redot-Engine:masterfrom
GeneralProtectionFault:subviewport_render_fix
Jul 16, 2026
Merged

Subviewport render fix#1303
Arctis-Fireblight merged 2 commits into
Redot-Engine:masterfrom
GeneralProtectionFault:subviewport_render_fix

Conversation

@GeneralProtectionFault

@GeneralProtectionFault GeneralProtectionFault commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #1283

The bug results from an issue handling viewport "siblings" at the same level in a parent-child hierarchy. They were sorted by RID, which causes problems depending on what order they were added to the scene tree, etc...

If the viewport's texture is consumed by a sibling viewport, it needs to be rendered first. or else the sibling can get a stale texture, hence epilepsy flicker texture.

This adds/uses a property to the Viewport struct to track if a viewport's texture is consumed by another.
This gives us the info we need to build a dependency graph, and sort the sibling viewports.

Summary by CodeRabbit

  • Bug Fixes
    • Improved viewport rendering order when a viewport consumes another viewport’s texture.
    • Ensured consuming viewports render after the viewports producing their textures.
    • Made dependency tracking frame-accurate to prevent stale ordering decisions.
    • Recalculated ordering automatically when a viewport’s parent changes.
    • Added cycle safeguards to preserve a stable render order when dependencies loop.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 92bb8796-f40c-4f6f-bae9-d004bcc68088

📥 Commits

Reviewing files that changed from the base of the PR and between 68f2512 and 0d561c6.

📒 Files selected for processing (2)
  • servers/rendering/renderer_viewport.cpp
  • servers/rendering/renderer_viewport.h
🚧 Files skipped from review as they are similar to previous changes (2)
  • servers/rendering/renderer_viewport.cpp
  • servers/rendering/renderer_viewport.h

Walkthrough

Viewport rendering now records sibling texture-consumption dependencies each frame, topologically orders sibling viewports so consumers render before producers, falls back on cycles, and invalidates ordering when parent relationships change.

Changes

Viewport dependency ordering

Layer / File(s) Summary
Dependency-aware sibling sorting
servers/rendering/renderer_viewport.h, servers/rendering/renderer_viewport.cpp
Adds used_by_viewport tracking and a topological sibling sort used by _sort_active_viewports(), with fallback to the original order when cycles are detected.
Frame dependency capture
servers/rendering/renderer_viewport.cpp
Clears stale dependencies, snapshots render-target usage before rendering, records newly consumed sibling targets, and marks ordering dirty after parent changes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RendererViewport
  participant ViewportRenderTargets
  participant ActiveViewportSorter
  RendererViewport->>ViewportRenderTargets: Snapshot render_target_was_used()
  RendererViewport->>ViewportRenderTargets: Render current viewport
  ViewportRenderTargets-->>RendererViewport: Report newly used sibling targets
  RendererViewport->>RendererViewport: Update used_by_viewport
  RendererViewport->>ActiveViewportSorter: Mark sorted ordering dirty
  ActiveViewportSorter->>ActiveViewportSorter: Topologically sort sibling viewports
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is related to the change, but it is too vague to explain the specific viewport ordering fix. Rename it to describe the main fix, e.g. "Fix subviewport sibling render ordering to prevent texture flicker".
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The change addresses #1283 by ordering sibling subviewports so texture producers render before consumers, matching the flicker fix.
Out of Scope Changes check ✅ Passed The changes stay within subviewport sibling ordering and render-dependency tracking; no unrelated functionality appears to be added.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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.

@Shakai-Dev Shakai-Dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix LGTM, just needs testing to see if it works

@GeneralProtectionFault

Copy link
Copy Markdown
Contributor Author

Fix LGTM, just needs testing to see if it works

Wow, quick on the trigger :). I did provide a test build to https://github.com/Flashfyre and he said it was working for him. It's probably worth checking in with him to see if that's still the case tomorrow.

@GeneralProtectionFault

Copy link
Copy Markdown
Contributor Author

Just checked with Sam, he wasn't able to reproduce the bug on the test build, think we're in good shape.

@Shakai-Dev Shakai-Dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested by the author of the issue & myself, lgtm.

@GeneralProtectionFault GeneralProtectionFault requested review from a team July 15, 2026 15:01
@Shakai-Dev Shakai-Dev self-requested a review July 15, 2026 15:06

@Shakai-Dev Shakai-Dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Codewise LGTM but I think it's a good idea to test it again

@GeneralProtectionFault

Copy link
Copy Markdown
Contributor Author

Codewise LGTM but I think it's a good idea to test it again

Sam already confirmed last night that the new bug is only in a specific circumstance, and that the random one was fixed by this change, and also that the new bug still appears on both the fixed, and currently released versions. I don't think there's a regression on that front.

@Shakai-Dev Shakai-Dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Arctis-Fireblight Arctis-Fireblight merged commit 6bb02aa into Redot-Engine:master Jul 16, 2026
17 checks passed
@github-project-automation github-project-automation Bot moved this from Open to Done in Engine Overview Jul 16, 2026
@GeneralProtectionFault GeneralProtectionFault deleted the subviewport_render_fix branch July 16, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Subviewport / AnimatedSprite2D Flicker Bug

3 participants