fix: repoint avatar preview renderer to renderfeatures main, and fix GeoffNoise.hlsl import error - #9956
Conversation
…unity-6-5 branch The Unity 6.5 upgrade pinned decentraland.renderfeatures to `#chore/unity-6-5` in avatar-preview-renderer so it could pick up the 6.5 fixes before they landed. That branch was merged as unity-explorer-packages#62 and deleted, but the pin was never moved back to `main` the way Explorer's was. Resolution still works today only because the locked hash (4426b066) is an ancestor of main, so UPM checks it out by SHA -- the renderer build on dev at 7870599 succeeded 80 minutes after the branch was deleted. It breaks the moment packages-lock.json is regenerated or dropped, because UPM then has to resolve the branch ref and gets a 404. Drop the pin and relock onto 04b022e9, which is what Explorer's lock already carries, so both projects consume byte-identical RenderFeatures. This also picks up unity-explorer-packages 1691b33d (gate skybox cubemap regeneration on change or interval), the one functional commit the renderer was behind on. It is inert here: the renderer wires only AvatarOutline and ObjectHighlight, and never references SkyboxEnvironmentProbe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mport
GeoffNoise.hlsl is a dual-target source: HLSL for the mountain shaders,
C# for a CPU mirror. It picked the branch with `#if SHADER_TARGET`, which
only holds while a shader is actually being compiled. Unity 6.5 parses
`.hlsl` files during asset import, where no shader macros are defined, so
the preprocessor fell through to the C# branch and the parser died on
line 14:
Assets/DCL/Landscape/Shaders/GeoffNoise.hlsl:(14): Error: Shader syntax
error. Expected '{' but found '.'.
`namespace Decentraland.Terrain` -- `namespace` is an HLSL keyword, so the
parser wanted a brace and found the dot of the qualified name.
Key off CSHARP_7_3_OR_NEWER instead, which Unity defines for every C#
compilation and never in HLSL, so the branch resolves the same way with or
without shader context. All five guards are inverted to match.
Verified by evaluating the file's directives directly: the original with no
macros defined does select the C# branch (reproducing the error), the
patched file selects the HLSL branch, and the patched file's import-time
output is byte-identical to the original's output under SHADER_TARGET=50 --
so shader compilation is unchanged. The C# branch is also still selected
when C# symbols are defined.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Lint did not finish (
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #33744406450. Full reportPR #9956, run #33744406450 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9956
STEP 2 — Root-cause check ✅
This PR fixes two distinct root causes left over from the Unity 6.5 upgrade (#9871):
-
Stale branch pin —
avatar-preview-renderer/Packages/manifest.jsonreferenced#chore/unity-6-5, which was merged and auto-deleted. The current resolution works only by coincidence (UPM resolves the locked SHA frompackages-lock.json); it will break the moment the lock file is regenerated. The fix correctly drops the branch fragment so UPM resolves againstmain, and relocks onto04b022e9— byte-identical to Explorer's lock entry. -
Incorrect preprocessor guard —
GeoffNoise.hlslused#if SHADER_TARGETto distinguish HLSL from C#. That macro is only defined during actual shader compilation; Unity 6.5 now syntax-checks.hlslfiles at asset import with no shader macros active, so the preprocessor fell through to the C# branch and hitnamespace Decentraland.Terrain— a syntax error in HLSL. Replacing all five guards with#if !CSHARP_7_3_OR_NEWER/#if CSHARP_7_3_OR_NEWERis correct:CSHARP_7_3_OR_NEWERis defined for every C# compilation and never in HLSL or at import time.
Both changes address the cause, not a symptom.
STEP 3 — Design & integration ✅
No new units, systems, managers, or persistent state introduced. No lifecycle changes. The shader file is not a class — it is a dual-target source file with preprocessor branching. The package repin is a JSON config change. Design review is not applicable.
STEP 4 — Member audit ✅
No public properties or members added or changed. Not applicable.
STEP 5 — Line-level review ✅
Preprocessor logic verification — all five guard sites checked:
| Location | Old guard | New guard | Semantics preserved? |
|---|---|---|---|
| Line 7 (HLSL macros) | #if SHADER_TARGET |
#if !CSHARP_7_3_OR_NEWER |
✅ HLSL path when not in C# |
| Line 626 (rotation matrices) | #if SHADER_TARGET |
#if !CSHARP_7_3_OR_NEWER |
✅ Static const arrays for HLSL |
| Line 696 (SampleBilinearClamp) | #if !SHADER_TARGET |
#if CSHARP_7_3_OR_NEWER |
✅ C#-only method |
| Line 713 (GetOccupancy) | #if !SHADER_TARGET |
#if CSHARP_7_3_OR_NEWER |
✅ C#-only method |
| Line 791 (namespace close) | #if !SHADER_TARGET |
#if CSHARP_7_3_OR_NEWER |
✅ Close C# namespace/class |
All inversions are consistent: SHADER_TARGET (defined in shader compilation) → !CSHARP_7_3_OR_NEWER (not defined in shader/import = same path). The three negated guards (!SHADER_TARGET → CSHARP_7_3_OR_NEWER) are similarly correct.
Package repin verification:
- Explorer's
manifest.json(ondev):"decentraland.renderfeatures": "git@github.com:decentraland/unity-explorer-packages.git?path=/RenderGraphs/RenderFeatures"— no branch pin ✅ - Explorer's
packages-lock.json(ondev): hash04b022e939c25c339fbdec5a30e11b2d3e851237✅ - PR's avatar-preview-renderer lock: same hash
04b022e9...✅ — both projects now consume the same RenderFeatures commit.
The new commit picks up 1691b33d (SkyboxToCubemapRendererFeature.cs), which is confirmed inert for the renderer — SkyboxEnvironmentProbe appears nowhere in avatar-preview-renderer/.
No issues found (P0, P1, or P2).
Security review ✅
- No secrets, credentials, or tokens in the diff
- Package reference stays on the same Decentraland-owned repository (
decentraland/unity-explorer-packages), moving from a deleted branch to the default (main) — reduces supply chain risk - Preprocessor symbols (
CSHARP_7_3_OR_NEWER) are compiler-defined, not user-influenceable - No injection vectors
Consumer impact ✅
No public API surface changes. The shader output is byte-identical under compilation. The package repin affects only the avatar-preview-renderer project's internal dependency resolution.
STEP 6–9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Preprocessor guard swap in a shader file and a JSON package repin — no runtime logic, ECS, async, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Avatar Preview Renderer — Vercel Preview is ready!
|
2 participants
Pull Request Description
What does this PR change?
Two leftovers from the Unity 6.5 upgrade (#9871), both found on
devafter it merged. Unrelated to each other, bundled at request — 3 files, 11 insertions.1. Avatar Preview Renderer still pinned to a deleted branch
The upgrade pinned
decentraland.renderfeaturesto#chore/unity-6-5inavatar-preview-renderer/Packages/manifest.jsonso the renderer could consume the 6.5 fixes before they landed. That branch merged asunity-explorer-packages#62and was auto-deleted; Explorer's pin was moved back tomain, the renderer's was not.It resolves today only by luck: the locked hash
4426b066is an ancestor ofmain(behind by 3), so UPM checks it out by SHA frompackages-lock.json. Evidence it currently works — the renderer build ondevat78705998succeeded at 12:29Z, 80 minutes after the branch was deleted at 11:09Z. It breaks as soon aspackages-lock.jsonis regenerated or dropped, because UPM then has to resolve the branch ref and gets a 404.Dropping the pin and relocking onto
04b022e9makes the entry byte-identical to Explorer's, so both projects consume the same RenderFeatures. It also picks up1691b33d(gate skybox cubemap regeneration on change or interval), the one functional commit the renderer was behind. That commit is inert here — the renderer wires onlyAvatarOutlineandObjectHighlightand never referencesSkyboxEnvironmentProbe.2.
GeoffNoise.hlslparses its C# branch at asset importGeoffNoise.hlslis a dual-target source — HLSL for the mountain shaders, C# for a CPU mirror — and selected its branch with#if SHADER_TARGET. That macro only exists while a shader is actually being compiled. Unity 6.5 parses.hlslfiles during asset import, where no shader macros are defined, so the preprocessor fell through to the C# branch and the parser hit line 14:namespace Decentraland.Terrainnamespaceis an HLSL keyword, so the parser wanted{and found the dot of the qualified name.The fix keys off
CSHARP_7_3_OR_NEWER, which Unity defines for every C# compilation and never in HLSL, so the branch resolves identically with or without shader context. All five guards are inverted to match.Test Instructions
Steps (standard run):
metaforge explorer run XXXX # ← replace with this PR numberExpected result: terrain and mountains render as on
dev— no flat, black, or missing mountain geometry. The console shows noGeoffNoise.hlslshader syntax error.Steps (Avatar Preview Renderer): open the Preview URL from the
Avatar Preview Renderer — Vercel Preview is ready!comment on this PR.Expected result: the avatar renders toon-shaded (not magenta) with correct outlines — the outline and highlight features come from the repointed package.
Prerequisites
Explorer/in Unity 6000.5.9f1 at least once — the shader fix is an import-time error, so it only shows on a project open / reimport, not in a built playerTest Steps
Explorer/in the editor. Console has noGeoffNoise.hlslerror.Assets/DCL/Landscape/Shaders/GeoffNoise.hlsl→ Reimport. Still no error.dev.?mode=configurator&username=test— the configurator renderer also loads (it wires both features).Additional Testing Notes
Verification actually performed — please read before approving:
SHADER_TARGET=50. So shader compilation cannot change — but "the import error is gone in the editor" is inferred, not observed. Step 1 above is the check that matters.1691b33daltersSkyboxToCubemapRendererFeature.cs(+20/-10). I confirmed the renderer's two URP renderer assets reference onlyAvatarOutlineandObjectHighlight, and thatSkyboxEnvironmentProbeappears nowhere inavatar-preview-renderer/, so the change should be inert — but the renderer's Web output is built by CI, not locally, so the Preview is the first real exercise.GeoffNoise.hlslis dead: there is no.cscounterpart, nothing compiles the file as C#, and nothing referencesGeoffNoisefrom C#. This PR preserves it rather than deleting it, since removing ~120 lines of someone's intended CPU/GPU parity source is a separate decision. Worth a follow-up.Quality Checklist