feat: add minimal Toon SH lighting - #58
Conversation
- Add intentional RED tests for fixed dominant-direction and two-band SH behavior. - Verify lightmap, ForwardAdd, Hybrid, and ABI boundaries with focused Unity tests.
- Add Toon-only dominant direction and bright/dark spherical-harmonics lighting. - Preserve Shader-Core, ForwardAdd, PBR, and Hybrid contracts with focused Unity tests.
- Document Toon lighting ownership, compatibility, and lilToon/OpenLit provenance. - Align package identity and shipped documentation with 0.2.0-beta.2.
- Reopen valid-but-unloaded fixture scenes through the existing controlled scope. - Verify the static-lightmap transaction in the Daily test suite.
- Upload runtime readbacks, cold reset receipts, summaries, and logs from validation runs. - Preserve strict package and repository-state evidence exports.
- Require all approved runtime evidence paths in hosted review contracts. - Keep direct artifact child paths free of parent-directory traversal.
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 2 minor |
🟢 Metrics 78 complexity · 0 duplication
Metric Results Complexity 78 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
Adds a minimal, Toon-only spherical-harmonics (SH) lighting implementation to Pure-Base’s BIRP host, keeping Shader-Core responsible for light acquisition/lightmaps/shadows and preserving existing PBR/Hybrid ownership boundaries. The change is reinforced with deterministic Daily contracts, workflow artifact export contracts, and documentation/versioning updates for the 0.2.0-beta.2 boundary.
Changes:
- Introduces Toon dominant-direction + bright/dark two-band SH evaluation via a new shared HLSL helper, wired into the Toon model and BIRP host while keeping
ForwardAddenvironment-free. - Adds/extends automated contracts (Daily + Release/Hosted workflow checks) to lock down Toon lighting behavior, lightmap determinism expectations, and evidence export paths.
- Bumps package/documentation versioning to
0.2.0-beta.2and records provenance/compatibility boundaries (lilToon/OpenLit concepts).
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Shaders/Common/toon_lighting.hlsl |
New minimal Toon lighting helper (binary direct factor, dominant direction, two-band SH). |
Shaders/Common/toon_lighting.hlsl.meta |
Unity importer metadata for the new shader include. |
Shaders/Models/toon.hlsl |
Routes Toon direct + ambient to the helper and defines aggregate-direction selection hook. |
Shaders/Models/pbr.hlsl |
Defines PBR aggregate-direction selection hook to preserve existing PBR/Hybrid direction behavior. |
Shaders/Common/birp_host.hlsl |
Adds aggregate-direction selection hook + ensures ForwardAdd remains environment-free. |
Tests/Daily/Editor/PureBaseToonLightingContractTests.cs |
New deterministic runtime/oracle contracts for Toon SH bands/direction and Hybrid/PBR boundaries. |
Tests/Daily/Editor/PureBaseToonLightingContractTests.cs.meta |
Unity importer metadata for the new Daily test. |
Tests/Daily/Editor/PureBaseValidationSceneRegressionTests.cs |
Adds regression coverage asserting a committed Toon static lightmap produces a finite nonzero delta via transient clone readback. |
Tests/Daily/Editor/PureBaseRenderingModeRenderingTests.SourceContracts.cs |
Adds source-order/ownership contracts ensuring Toon-only inclusion and host/environment/ForwardAdd boundaries. |
Tests/Release/Run-PureBaseReleaseValidation.Tests.ps1 |
Adds contract asserting the Release workflow uploads evidence (including runner-side runtime artifacts). |
.github/workflows/release-validation.yml |
Extends evidence artifact upload paths to include runtime readbacks, cold reset receipts, summaries, and logs. |
.github/tests/HostedUnityReviewContracts.Tests.ps1 |
Updates Hosted review contracts to require the expanded evidence upload path list exactly. |
Docs/technical-information.md |
Documents Toon lighting boundary/ownership and updates version to 0.2.0-beta.2. |
Docs/technical-information.ja.md |
Japanese counterpart of the Toon boundary/ownership + version update. |
Docs/pure-base-shader-contract.md |
Updates contracted Toon/Hybrid behavior description and adds compatibility/boundary classification. |
README.md |
Updates package version reference and clarifies independence/provenance language. |
README.ja.md |
Japanese counterpart of README version + independence/provenance clarification. |
NOTICE |
Adds explicit third-party lighting concept provenance and inspected-source references (lilToon/OpenLit). |
package.json |
Bumps package version and release URL to 0.2.0-beta.2. |
CHANGELOG |
Adds 0.2.0-beta.2 entry summarizing Toon lighting + contract additions. |
Files not reviewed (2)
- Shaders/Common/toon_lighting.hlsl.meta: Generated file
- Tests/Daily/Editor/PureBaseToonLightingContractTests.cs.meta: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull Request Overview
The PR introduces a minimal Toon lighting model for the Built-in Render Pipeline, but currently does not meet quality standards. A significant logic error was identified in birp_host.hlsl, where the environment light calculation may cause double-lighting when lightmaps are active.
Additionally, while the test coverage for the new lighting model is extensive, the validation suite introduces maintenance debt. Specifically, the test cleanup logic in ToonLightingCaptureScope is overly complex, and the lack of proper equality implementation for SH coefficients may lead to performance overhead during test execution. These issues should be resolved to ensure a stable and maintainable lighting implementation.
About this PR
- The newly added test suite for Toon lighting contracts contains systemic maintainability issues, including high cyclomatic complexity in resource cleanup and suboptimal equality checks for large structs. Ensure that test code follows the same quality and performance standards as production code.
Test suggestions
- Verify Toon dominant light direction calculation matches the expected C# oracle for both standard and degenerate (zero-length) vectors.
- Verify Toon SH evaluation correctly selects between bright and dark bands based on the surface normal.
- Confirm that ForwardAdd point lights ignore SH environment contribution and preserve destination alpha.
- Ensure Hybrid materials retain PBR direct specular behavior and ignore injected Toon SH directions.
- Verify that static lightmaps contribute finite, nonzero baked delta to Toon surfaces in the validation scene.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
- Split Toon runtime contract support and extract focused test assertion helpers. - Document the non-comparison SH carrier rationale and retain Unity fixture validation.
- Replace the shared normal texture in the isolated Toon capture with a deterministic fixture-owned neutral texture. - Validate the four historical readback failures and the 67-test Daily EditMode assembly.
- Build a completed nonpersistent mesh for each isolated Toon lighting draw instead of mutating shared vertex state. - Validate the historical D3D11 contracts and the 67-test Daily EditMode assembly.
- Render Toon lighting contracts with isolated BIRP renderer and ForcePixel lights instead of synthetic direct-light globals. - Validate focused Toon contracts and the full Daily EditMode assembly locally.
- Extract preview-scene resource initialization from the Toon lighting capture constructor. - Verify focused Toon contracts and the complete Daily EditMode assembly locally. Co-authored-by: Copilot <copilot@github.com>
Summary
0.2.0-beta.2compatibility/provenance boundary for lilToon 2.3.4 and OpenLit 1.0.2.Validation
31464812403: passed on426f6bf60892489356ae09e28304783ecd5d66efusing Unity2022.3.22f1and Shader-Core0.1.9.31464671497: passed (Pester and actionlint).31464674628: passed (C# and Actions).Known Limitations
pull_requestorpush masterevent.Not Found; no passing Codacy result is claimed.No tag, GitHub Release, or VPM repository mutation is included.