removed sdfRenderOp, reduces object creation#790
Open
jfboeve wants to merge 15 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors SDF text rendering to remove the dedicated SdfRenderOp render-op object and instead render SDF text by submitting CoreTextNode instances directly to the WebGL render op list, aiming to reduce per-frame object creation/GC.
Changes:
- Replaced
TextRenderInfowith a discriminated union (SdfRenderInfo/CanvasRenderInfo) and adjusted renderer interfaces accordingly. - Removed
SdfRenderOpand moved SDF buffer/shader-prop setup intoCoreTextNode+ special-case binding inWebGlShaderProgram. - Updated
WebGlRendererrender-op typing and related glue code to accept SDF text nodes as render ops.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/text-rendering/TextRenderer.ts | Introduces new discriminated TextRenderInfo model and updates TextRenderer interface. |
| src/core/text-rendering/SdfTextRenderer.ts | Stops building SdfRenderOp; caches SDF render info and submits CoreTextNode as a render op. |
| src/core/text-rendering/CanvasTextRenderer.ts | Updates cache typing and returns CanvasRenderInfo with type: 'canvas'. |
| src/core/renderers/webgl/WebGlShaderProgram.ts | Adds SDF-specific uniform binding branch keyed off render-op type/flags. |
| src/core/renderers/webgl/WebGlRenderer.ts | Updates WebGlRenderOp type to include text nodes as render ops. |
| src/core/renderers/webgl/SdfRenderOp.ts | Deleted (render-op replaced by node-driven approach). |
| src/core/CoreTextNode.ts | Implements SDF render-op behavior directly: buffer upload/caching, shader props, and custom draw for SDF text. |
| src/core/CoreNode.ts | Adjusts isCoreNode type annotation. |
Comments suppressed due to low confidence (1)
src/core/CoreTextNode.ts:289
- This check only guards against
imageData === undefined, butCanvasTextRenderercan currently produceimageData === null(when canvas dims are 0). That meansnullcan flow into thecreateTexture({ src: ... })call below and crash. Update the check (or the renderer types/behavior) to handlenullexplicitly.
if (textRendererType === 'canvas') {
if (result.imageData === undefined) {
this.emit('failed', {
type: 'text',
Comment on lines
+453
to
+455
| override get quadBufferCollection(): BufferCollection { | ||
| return this._sdfQuadCollection || super.quadBufferCollection; | ||
| } |
Comment on lines
+252
to
+256
| if ( | ||
| isCoreNode === false && | ||
| (renderOp as CoreTextNode).sdfShaderProps !== null | ||
| ) { | ||
| const opShader = renderOp.shader!; // SdfRenderOp has .shader |
Collaborator
Author
There was a problem hiding this comment.
@copilot apply changes based on this feedback in seperate PR
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
Contributor
5 tasks
Contributor
#791) `CoreTextNode` was taking the SDF-specific branch by overriding `isCoreNode`, which also changed earlier RTT framebuffer selection in `bindRenderOp`. That coupling could produce incorrect `u_resolution` / `u_pixelRatio` for SDF text rendered under an RTT parent. - **What changed** - Added a dedicated `isSdfRenderOp` signal on render ops. - Switched SDF-specific bind logic in `WebGlShaderProgram` to use `isSdfRenderOp` instead of `isCoreNode`. - Restored `CoreTextNode` to the normal core-node framebuffer path so RTT parent dimensions continue to be used for resolution selection. - **Behavioral impact** - SDF text keeps its custom shader binding path. - RTT resolution/pixel ratio selection continues to follow core-node semantics for text nodes. - Render-op reuse remains stable for SDF text under RTT parents. - **Regression coverage** - Added focused WebGL tests for: - SDF text using parent RTT framebuffer dimensions during uniform binding - SDF render-op reuse when parent RTT dimensions match - **Illustrative change** ```ts // before if (isCoreNode === false && (renderOp as CoreTextNode).sdfShaderProps !== null) { // SDF bind } // after if (renderOp.isSdfRenderOp === true) { // SDF bind } ```
|
|
…ontract (#793) `SdfTextRenderer.renderText` declared `TextRenderInfo | null` as its return type, violating the `TextRenderer` interface which requires `TextRenderInfo`. A `null` return reaching `CoreTextNode.handleRenderResult` would throw on `result.type` access. ## Changes - **Return type corrected** — `renderText` now returns `TextRenderInfo`, matching the interface - **Redundant null guards removed:** - `props.text.length === 0` early-return dropped — `CoreTextNode` blocks `renderText` calls when text is empty - `fontData === undefined` null-return dropped — `CoreTextNode` verifies `isFontLoaded` before invoking `renderText`; font lookup inlined with `!` per zero-safety-checks policy ```ts // Before const renderText = (props: CoreTextNodeProps): TextRenderInfo | null => { if (props.text.length === 0) return null; // ... const fontData = SdfFontHandler.getFontData(props.fontFamily); if (fontData === undefined) return null; // ... }; // After const renderText = (props: CoreTextNodeProps): TextRenderInfo => { // ... const layout = generateTextLayout(props, SdfFontHandler.getFontData(props.fontFamily)!); // ... }; ```
…opilot/sub-pr-790-again # Conflicts: # src/core/CoreTextNode.test.ts # src/core/renderers/webgl/WebGlShaderProgram.test.ts
…#792) - [x] Inspect current branch state, recent commits, and whether merge conflicts exist locally - [x] Check recent GitHub Actions runs for `feat/removed-sdfrenderop` and this branch if needed - [x] Fetch the stacked base branch and compare it against the current branch - [x] Resolve any new conflicts if present and run targeted validation - [x] Run final validation and reply on the review thread
wouterlucas
approved these changes
May 11, 2026
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
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.
No description provided.