Skip to content

Fix SOGS rendering scenes with SH 1 or 2 #7703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 2, 2025

Conversation

vincentwoo
Copy link
Contributor

@vincentwoo vincentwoo commented May 22, 2025

SOGS scenes with SH 1 or 2 do not render currently, owing to missing handlers for their spherical harmonics.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@willeastcott
Copy link
Contributor

Thanks for the PR, @vincentwoo! Would you be able to pop a PR description up too please? Thanks! 🙏

@willeastcott willeastcott requested a review from Copilot May 22, 2025 21:30
@willeastcott willeastcott added bug area: graphics Graphics related issue labels May 22, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes rendering issues with SOGS scenes by introducing overloaded versions of the readSHData function to handle different spherical harmonics (SH) vector depths.

  • Added a comment clarifying the function overload rationale.
  • Introduced two new overloads of readSHData for SH vector sizes of 8 and 3.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

We'll need to fix WGSL shaders to match as well

@slimbuck slimbuck requested a review from Copilot May 29, 2025 18:33
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the issue with SOGS scenes not rendering for SH 1 or 2 by adding missing shader handlers and updating the spherical harmonics coefficient calculations.

  • Added new WGSL shader chunks for SOGS (gsplatSogsColorVS, gsplatSogsDataVS, and gsplatSogsSHVS)
  • Updated shader code in both WGSL and GLSL to use the configurable SH_COEFFS macro instead of hard-coded values, and refined uniform usage
  • Revised preprocessor conditionals in the common shader files to correctly include the appropriate shader chunks based on SH_BANDS

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/scene/shader-lib/wgsl/collections/shader-chunks-wgsl.js Added imports and array entries for new SOGS shader chunks.
src/scene/shader-lib/wgsl/chunks/gsplat/vert/gsplatSogsSH.js Updated SH coefficient reading logic to use SH_COEFFS and refined uniform references.
src/scene/shader-lib/wgsl/chunks/gsplat/vert/gsplatSogsData.js Adjusted uniform references in data extraction functions.
src/scene/shader-lib/wgsl/chunks/gsplat/vert/gsplatSogsColor.js Updated uniform usage in the color extraction function.
src/scene/shader-lib/wgsl/chunks/gsplat/vert/gsplatCommon.js Reorganized SH_COEFFS macros and conditional includes based on SH_BANDS.
src/scene/shader-lib/glsl/chunks/gsplat/vert/gsplatSogsSH.js Applied similar SH_COEFFS logic for reading coefficients in GLSL.
src/scene/shader-lib/glsl/chunks/gsplat/vert/gsplatCommon.js Revised preprocessor definitions and conditional includes for proper SH handling in GLSL.

@slimbuck
Copy link
Member

I've also updated the wgsl shaders to work with SOGS.

@slimbuck slimbuck merged commit d6fb421 into playcanvas:main Jun 2, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants