Skip to content

Fix flipy#212

Merged
xiaoiver merged 2 commits intoreleasefrom
fix-flipy
Apr 17, 2026
Merged

Fix flipy#212
xiaoiver merged 2 commits intoreleasefrom
fix-flipy

Conversation

@xiaoiver
Copy link
Copy Markdown
Contributor

@xiaoiver xiaoiver commented Apr 17, 2026

🤔 This is a

  • New feature
  • Bug fix
  • Site / Document optimization
  • TypeScript definition update
  • Refactoring
  • Performance improvement
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@xiaoiver xiaoiver merged commit 6d118ff into release Apr 17, 2026
0 of 5 checks passed
@xiaoiver xiaoiver deleted the fix-flipy branch April 17, 2026 09:28
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements dynamic viewport height calculation for Y-coordinate flipping in WebGPU render passes. Feedback focuses on simplifying the logic for determining the flip height and maintaining proper encapsulation by avoiding string-indexed access to private class members.

Comment thread src/webgpu/RenderPass.ts
Comment on lines +212 to +225
const firstColor =
this.gfxColorAttachment.length > 0 ? this.gfxColorAttachment[0] : null;
const level0 =
this.gfxColorAttachmentLevel.length > 0
? this.gfxColorAttachmentLevel[0] || 0
: 0;
if (firstColor !== null && firstColor !== undefined) {
this._viewportFlipHeight = Math.max(1, firstColor.height >>> level0);
} else if (descriptor.depthStencilAttachment) {
const ds = descriptor.depthStencilAttachment as unknown as Attachment_WebGPU;
this._viewportFlipHeight = Math.max(1, ds.height >>> 0);
} else {
this._viewportFlipHeight = this.device['swapChainHeight'];
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The logic for determining the viewport flip height can be simplified by using the nullish coalescing operator and leveraging the already populated internal state (this.gfxColorAttachment and this.gfxDepthStencilAttachment).

Also, note that accessing this.device['swapChainHeight'] via string indexing is a bypass of TypeScript's private access modifier. It would be better to expose this value through a public getter in the Device_WebGPU class to maintain proper encapsulation.

    const level0 = this.gfxColorAttachmentLevel[0] ?? 0;
    const firstColor = this.gfxColorAttachment[0];
    if (firstColor) {
      this._viewportFlipHeight = Math.max(1, firstColor.height >>> level0);
    } else if (this.gfxDepthStencilAttachment) {
      this._viewportFlipHeight = Math.max(1, this.gfxDepthStencilAttachment.height >>> 0);
    } else {
      this._viewportFlipHeight = this.device['swapChainHeight'];
    }

Comment thread src/webgpu/RenderPass.ts
Comment on lines +241 to +242
const height =
this._viewportFlipHeight ?? this.device['swapChainHeight'];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Accessing the private property swapChainHeight via string indexing here also breaks encapsulation. Since _viewportFlipHeight is guaranteed to be initialized in setRenderPassDescriptor (which is called by beginRenderPass), you can likely rely on it being set or use a safer fallback that doesn't bypass access modifiers.

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.

1 participant