Adapt selection picker to URP

[thomas-tu]

Purpose of this PR

This PR updates the picking system so it properly runs on URP. We used to use an old hack in URP project: the picking system would force the BiRP to run temporarly to render the lookup texture. This is no longer something we can do due to some others packages that could react to the change of RP asset changes (example: VFX graph).

To solve this and since URP now allows render pass injection, I've created our own URP render pass and inject it when we run the picking system. URP uses HLSL so I had to migrate our CG shaders to HLSL.

Manually tested.

Links

Jira: UUM-133859

[u-pr]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪ The PR adds a new URP rendering path plus shaders and assembly definition changes, which require careful validation across multiple Unity render pipeline configurations.

🏅 Score: 70 The overall direction looks correct, but there are several high-risk integration/compatibility issues (assembly dependencies, shader name mismatch, and lifecycle/cleanup concerns) that need addressing before merge.

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Compatibility Adding unconditional assembly references to URP/Core/HDRP runtime assemblies can break compilation in projects that don’t have those packages installed; consider isolating URP-specific code into a separate asmdef (or otherwise avoiding hard references) so built-in pipeline users aren’t forced to install URP. { "name": "Unity.ProBuilder", "rootNamespace": "", "references": [ "Unity.ProBuilder.Poly2Tri", "Unity.ProBuilder.KdTree", "Unity.RenderPipelines.HighDefinition.Runtime", "Unity.RenderPipelines.Universal.Runtime", "Unity.RenderPipelines.Core.Runtime" ], Possible Issue The URP face picker shader string appears inconsistent with the actual shader name added in this PR (the shader file declares a Hidden/ProBuilder path); verify that the k_FacePickerShader (and related material paths) resolve correctly at runtime to avoid null shaders/materials. #if PB_URP_MODE static string k_EdgePickerShader = "Hidden/ProBuilder/EdgePickerURP"; static string k_VertexPickerShader = "Hidden/ProBuilder/VertexPickerURP"; static string k_FacePickerShader = "Shader Graphs/FacePickerURP"; static string k_EdgePickerMaterial = "Materials/EdgePickerURP"; static string k_FacePickerMaterial = "Materials/FacePickerURP"; static string k_VertexPickerMaterial = "Materials/VertexPickerURP"; #else static string k_EdgePickerShader = "Hidden/ProBuilder/EdgePicker"; static string k_FacePickerShader = "Hidden/ProBuilder/FacePicker"; static string k_VertexPickerShader = "Hidden/ProBuilder/VertexPicker"; static string k_EdgePickerMaterial = "Materials/EdgePicker"; static string k_FacePickerMaterial = "Materials/FacePicker"; static string k_VertexPickerMaterial = "Materials/VertexPicker"; #endif Cleanup/Leaks The URP render path subscribes to RenderPipelineManager.beginCameraRendering and allocates temporary objects, but cleanup is not protected by try/finally; failures or early exits could leave the callback subscribed and/or leak the temporary camera/RT, and using UnityEditor in a Runtime file may also break player builds unless editor-guarded. using UnityEditor; using UnityEngine.Rendering; #if PB_URP_MODE using UnityEngine.Rendering.Universal; using UObject = UnityEngine.Object; using static UnityEngine.Rendering.RenderPipeline; #endif namespace UnityEngine.ProBuilder { internal partial class SelectionPickerRenderer { internal class SelectionPickerRendererURP : ISelectionPickerRenderer { public static Camera temporaryCamera; /// <summary> /// Render the camera with a replacement shader and return the resulting image. /// RenderTexture is always initialized with no gamma conversion (RenderTextureReadWrite.Linear) /// </summary> /// <param name="camera"></param> /// <param name="shader"></param> /// <param name="tag"></param> /// <param name="width"></param> /// <param name="height"></param> /// <returns></returns> public Texture2D RenderLookupTexture( Camera camera, Shader shader, string tag, int width = -1, int height = -1) { #if PB_URP_MODE bool autoSize = width < 0 || height < 0; int _width = autoSize ? (int)camera.pixelRect.width : width; int _height = autoSize ? (int)camera.pixelRect.height : height; GameObject go = new GameObject(); Camera renderCam = go.AddComponent<Camera>(); temporaryCamera = renderCam; renderCam.CopyFrom(camera); renderCam.renderingPath = RenderingPath.Forward; renderCam.enabled = false; renderCam.clearFlags = CameraClearFlags.SolidColor; renderCam.backgroundColor = Color.white; renderCam.allowHDR = false; renderCam.allowMSAA = false; renderCam.forceIntoRenderTexture = true; renderCam.GetUniversalAdditionalCameraData(); RenderTextureDescriptor descriptor = new RenderTextureDescriptor() { width = _width, height = _height, colorFormat = renderTextureFormat, autoGenerateMips = false, depthBufferBits = 16, dimension = UnityEngine.Rendering.TextureDimension.Tex2D, enableRandomWrite = false, memoryless = RenderTextureMemoryless.None, sRGB = false, useMipMap = false, volumeDepth = 1, msaaSamples = 1 }; RenderTexture rt = RenderTexture.GetTemporary(descriptor); #if PB_DEBUG /* Debug.Log(string.Format("antiAliasing {0}\nautoGenerateMips {1}\ncolorBuffer {2}\ndepth {3}\ndepthBuffer {4}\ndimension {5}\nenableRandomWrite {6}\nformat {7}\nheight {8}\nmemorylessMode {9}\nsRGB {10}\nuseMipMap {11}\nvolumeDepth {12}\nwidth {13}", RenderTexture.active.antiAliasing, RenderTexture.active.autoGenerateMips, RenderTexture.active.colorBuffer, RenderTexture.active.depth, RenderTexture.active.depthBuffer, RenderTexture.active.dimension, RenderTexture.active.enableRandomWrite, RenderTexture.active.format, RenderTexture.active.height, RenderTexture.active.memorylessMode, RenderTexture.active.sRGB, RenderTexture.active.useMipMap, RenderTexture.active.volumeDepth, RenderTexture.active.width)); */ #endif var request = new StandardRequest() { destination = rt }; RenderPipelineManager.beginCameraRendering += CustomRenderPass; if (RenderPipeline.SupportsRenderRequest(renderCam, request) == false) Debug.LogWarning("RenderRequest not supported."); RenderPipeline.SubmitRenderRequest<StandardRequest>(renderCam, request); RenderTexture prev = RenderTexture.active; RenderTexture.active = rt; Texture2D img = new Texture2D(_width, _height, textureFormat, false, false); img.ReadPixels(new Rect(0, 0, _width, _height), 0, 0); img.Apply(); RenderTexture.active = prev; RenderTexture.ReleaseTemporary(rt); RenderPipelineManager.beginCameraRendering -= CustomRenderPass; temporaryCamera = null; UObject.DestroyImmediate(go); return img;

Update review

---

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

[u-pr]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix shader name mismatch Make the URP face picker shader identifier consistent with the actual shader name to prevent Shader.Find/material creation from failing. The added URP shader file declares Hidden/ProBuilder/FacePickerURP, so use that here as well. Runtime/Core/BuiltinMaterials.cs [73-75] static string k_EdgePickerShader = "Hidden/ProBuilder/EdgePickerURP"; static string k_VertexPickerShader = "Hidden/ProBuilder/VertexPickerURP"; -static string k_FacePickerShader = "Shader Graphs/FacePickerURP"; +static string k_FacePickerShader = "Hidden/ProBuilder/FacePickerURP"; Suggestion importance[1-10]: 9 __ Why: The k_FacePickerShader variable is assigned a path ("Shader Graphs/FacePickerURP") that does not match the actual shader name defined in FacePickerURP.shader ("Hidden/ProBuilder/FacePickerURP"). This discrepancy will cause the shader lookup to fail at runtime, breaking the face picking functionality when using URP.	High
	Always clean up temporary rendering resources Ensure render pipeline event unsubscription and temporary resource cleanup happen even if rendering/reading pixels throws. Wrap the render request + readback in a try/finally that always restores RenderTexture.active, releases rt, unsubscribes, and destroys the temporary camera object. Runtime/Core/SelectionPickerRendererURP.cs [97-116] +RenderTexture prev = RenderTexture.active; RenderPipelineManager.beginCameraRendering += CustomRenderPass; -if (RenderPipeline.SupportsRenderRequest(renderCam, request) == false) - Debug.LogWarning("RenderRequest not supported."); +try +{ + if (RenderPipeline.SupportsRenderRequest(renderCam, request) == false) + Debug.LogWarning("RenderRequest not supported."); -RenderPipeline.SubmitRenderRequest<StandardRequest>(renderCam, request); -RenderTexture prev = RenderTexture.active; -RenderTexture.active = rt; + RenderPipeline.SubmitRenderRequest<StandardRequest>(renderCam, request); -Texture2D img = new Texture2D(_width, _height, textureFormat, false, false); -img.ReadPixels(new Rect(0, 0, _width, _height), 0, 0); -img.Apply(); + RenderTexture.active = rt; -RenderTexture.active = prev; -RenderTexture.ReleaseTemporary(rt); -RenderPipelineManager.beginCameraRendering -= CustomRenderPass; -temporaryCamera = null; -UObject.DestroyImmediate(go); + Texture2D img = new Texture2D(_width, _height, textureFormat, false, false); + img.ReadPixels(new Rect(0, 0, _width, _height), 0, 0); + img.Apply(); -return img; + return img; +} +finally +{ + RenderTexture.active = prev; + RenderTexture.ReleaseTemporary(rt); + RenderPipelineManager.beginCameraRendering -= CustomRenderPass; + temporaryCamera = null; + UObject.DestroyImmediate(go); +} Suggestion importance[1-10]: 7 __ Why: Implementing a try/finally block ensures that resources such as the temporary RenderTexture are released, event subscriptions are removed, and the temporary GameObject is destroyed even if an exception occurs during the rendering or pixel reading process. This enhances the robustness of the selection picking logic.	Medium
More suggestions

---

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

[codecov-github-com]

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Runtime/Core/SelectionPickerRendererURP.cs	0.00%	3 Missing ⚠️

@@           Coverage Diff           @@
##           master     #657   +/-   ##
=======================================
  Coverage   36.05%   36.06%           
=======================================
  Files         277      278    +1     
  Lines       34909    34916    +7     
=======================================
+ Hits        12588    12592    +4     
- Misses      22321    22324    +3     

Flag	Coverage Δ
probuilder_MacOS_6000.0	34.58% <66.66%> (+<0.01%)	⬆️
probuilder_MacOS_6000.3	34.58% <66.66%> (+<0.01%)	⬆️
probuilder_MacOS_6000.4	34.57% <66.66%> (+<0.01%)	⬆️
probuilder_MacOS_6000.5	34.57% <66.66%> (+<0.01%)	⬆️
probuilder_MacOS_6000.6	34.57% <66.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
Runtime/Core/BuiltinMaterials.cs	52.42% <100.00%> (ø)
Runtime/Core/SelectionPickerRenderer.cs	92.72% <100.00%> (+0.05%)	⬆️
Runtime/Core/SelectionPickerRendererURP.cs	0.00% <0.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thomas-tu]

I should remove this, it's not in the plan to expose the render pass in the UI.

[thomas-tu]

Todo: remove copy of camera, no longer needed with the RenderRequest API.