Skip to content

fix: gpui occlusion nre - #8819

Merged
lorux0 merged 1 commit into
devfrom
fix/gpui-nre
May 21, 2026
Merged

fix: gpui occlusion nre#8819
lorux0 merged 1 commit into
devfrom
fix/gpui-nre

Conversation

@lorux0

@lorux0 lorux0 commented May 19, 2026

Copy link
Copy Markdown
Collaborator

What does this PR change?

Fixes #8680
Fixes #8215

Fixes a NullReferenceException happening on the GPUI-PRO package: https://github.com/decentraland/unity-explorer-packages/pull/58

It can happen from unity 6. Seems to be race condition. The camera might trigger the rendering before the urp has finished registering the asset.

Test Instructions

Go through different scenes & worlds. . Check everything renders normally.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

@lorux0
lorux0 requested review from a team as code owners May 19, 2026 13:23
@github-actions
github-actions Bot requested a review from anicalbano May 19, 2026 13:24
@github-actions

github-actions Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 1m 19s —— View job


PR Review

  • Gather context
  • Review changed files
  • Assess root cause and fix correctness
  • Check for blocking issues
  • Final summary

Root-Cause Analysis

Problem: A NullReferenceException in the GPUI-PRO package occurs under Unity 6 due to a race condition: the camera may trigger rendering before URP has finished registering the asset, so the renderer is accessed while still null.

Fix: PASS — The fix addresses the actual cause. The package (PR #58 in unity-explorer-packages) made RegisterRenderer async so it waits for URP asset registration to complete before returning. The calling code in LandscapePlugin.cs is correctly updated to await this new async API. This is proper ordering of async initialization, not a symptom workaround or null-guard band-aid.


Blocking Issues

None found.


Non-Blocking Observations

LandscapePlugin.cs:90–91 — The new await GPUICoreAPI.RegisterRenderer(...) does not forward the CancellationToken ct that is available in InitializeAsync. Per async-programming guidelines, async calls should propagate the cancellation token where the API supports it. If the new package API accepts a CancellationToken, it should be passed here. Worth confirming against the package signature.


packages-lock.json

All three GPUI-related packages (com.gurbu.gpui-pro, one intermediate package, com.gurbu.gpui-pro.terrain) are pinned to the same new commit hash (e68a493d), consistent with a monorepo update in decentraland/unity-explorer-packages. This is correct.


REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Touches only plugin initialization code and a package lock file — no ECS systems, components, queries, or runtime paths modified.
QA_REQUIRED: YES

@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 2m 16s —— View job


Claude PR Review — fix/gpui-nre

  • Inspect changes to LandscapePlugin.cs
  • Inspect dependency change in packages-lock.json
  • Look up the registry/source metadata for the changed package
  • Assess supply-chain & runtime risk
  • Post review verdict

Summary

File / dep Version Source Type Scope Risk Evidence confidence
LandscapePlugin.cs repo C# source runtime LOW HIGH
com.dcl.gpui-assets 2488227c…e68a493d… git@github.com:decentraland/unity-explorer-packages.git (path /GPUInstancerPro/com.dcl.gpui-assets) unity package runtime LOW–MEDIUM MEDIUM
com.gurbu.gpui-pro d8112ca7…e68a493d… git@github.com:decentraland/unity-explorer-packages.git (path /GPUInstancerPro/com.gurbu.gpui-pro) unity package runtime LOW–MEDIUM MEDIUM
com.gurbu.gpui-pro.terrain a9457c91…e68a493d… git@github.com:decentraland/unity-explorer-packages.git (path /GPUInstancerPro/com.gurbu.gpui-pro.terrain) unity package runtime LOW–MEDIUM MEDIUM

All three lockfile entries bump to the same commit e68a493d0dfd76232103b71c9af6233131048950 — consistent with a monorepo (unity-explorer-packages) bump containing the upstream PR unity-explorer-packages#58.


Dependency security review

Provenance / pinning

  • [PR] Source is decentraland/unity-explorer-packages — Decentraland-controlled. ✅
  • [PR] Lockfile pins every entry to an immutable 40-char SHA. ✅
  • [PR] Manifest entries (Packages/manifest.json) do not include a #ref — they resolve to the default branch, with the lockfile recording the resolved commit. This is the existing convention across the repo for Decentraland-internal packages and is not introduced by this PR.
  • [Registry metadata] Upstream repo is private; no public registry listing exists for these packages. Reviewer cannot fetch metadata.

Runtime capability

  • [PR] GPUI Pro is a GPU instancing rendering library — capability (GPU rendering / URP integration) matches its purpose.
  • [PR] The C# call-site change (sync out → awaited RegisterRenderer reading result.rendererKey) matches the stated fix: defer until URP has finished registering the asset, eliminating the Unity 6 race condition that caused the NRE.
  • [UNKNOWN] Contents of upstream PR feat: scene runtime factory #58 cannot be verified from outside the private repo; reviewer must trust the team's upstream review.

Transitive risk

  • [PR] No transitive dependency changes — com.unity.burst@1.6.0, com.unity.collections@1.2.0, com.unity.mathematics@1.2.0 and com.gurbu.gpui-pro@0.9.19 are unchanged in the lockfile. ✅
  • [PR] No new binary files or .asmdef/build-script changes in this PR. ✅

Maintenance / known risk

  • [PR] Decentraland-controlled fork; no public advisory channel applies. License of upstream Gurbu GPUI Pro is outside this PR's scope and predates this change.

C# code review — LandscapePlugin.cs:87-94

The change is minimal and correct:

var result = await GPUICoreAPI.RegisterRenderer(landscape.Root,
    treePrototypes[prototypeIndex].asset, treesProfile);
treeRendererKeys[prototypeIndex] = result.rendererKey;
  • Preserves the per-iteration await UniTask.Yield(PlayerLoopTiming.LastPostLateUpdate, ct) that already provides cooperative cancellation.
  • Awaiting per item (vs. WhenAll) is appropriate — registration into URP must complete in order before terrain generators consume treeRendererKeys.
  • Minor (non-blocking): the cancellation token ct is not threaded into GPUICoreAPI.RegisterRenderer. If the new async overload accepts a CancellationToken, plumb it through so registration can short-circuit on plugin teardown. If it doesn't, ignore.
  • No exception handling was added — but the previous code didn't have any either, so behaviour is unchanged. If RegisterRenderer can now throw async ordering exceptions distinct from the previous sync path, consider wrapping with SuppressToResultAsync() per the project's async-programming patterns. Not necessary if the new API is documented to never throw.

Risk classification

LOW–MEDIUM

LOW: source is Decentraland-controlled, lockfile is SHA-pinned, the visible C# change matches the stated fix, no new transitive deps, no binaries, no asmdef/build-script changes.

MEDIUM (soft): the underlying upstream commit's content cannot be verified by external reviewers — confirmation rests on review of unity-explorer-packages#58.

Recommendations

  • Confirm unity-explorer-packages#58 has been reviewed and only contains the targeted async RegisterRenderer change (no unrelated additions, especially native plugins or new network/filesystem code).
  • Optional: pass ct into GPUICoreAPI.RegisterRenderer if the new overload accepts one.
  • Optional, longer-term: consider pinning manifest entries for internal git packages by #<sha> rather than relying solely on the lockfile, so a lockfile regeneration cannot silently float to a new commit.

DEPENDENCY_REVIEW: NEEDS_ATTENTION — only because external reviewers cannot inspect the upstream private commit; if unity-explorer-packages#58 is reviewed and scoped as expected by the maintainer, this is effectively a PASS.
Branch fix/gpui-nre

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@Ludmilafantaniella Ludmilafantaniella left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — Tested on Windows and Mac.

Visited multiple scenes and worlds — everything renders normally, no visual issues or NullReferenceExceptions observed.

LGTM! 🚀

8819-eviMac.mp4
8819-Evi.mp4

✅Smoke test performed:

  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • ✔️ Chat and multiplayer
  • ✔️ Profile card
  • ✔️ Skybox

@lorux0
lorux0 merged commit e084b9b into dev May 21, 2026
21 of 26 checks passed
@lorux0
lorux0 deleted the fix/gpui-nre branch May 21, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants