Robinseibold / Unity-URP-Outlines

A custom renderer feature for screen space outlines
MIT License
590 stars 51 forks source link

Update to 2023.2.0f1 API/URP 16.0 #28

Closed adrian-miasik closed 8 months ago

adrian-miasik commented 9 months ago

Overview

Updates the project to support Unity v2023.2 + Universal Render Pipeline v16.04

Purposely tried to make as few changes as possible to get this to work. Credit goes to @Chishikii for figuring out how to get this up and running in Unity 2023.2. I don't fully understand the use cases for the occluder, so would appreciate another set of eyes there on how it was implemented but generally I've tried to make no real changes to the logic except what was needed to get this working.

Though I recommend we use @Chishikii's solution instead: https://github.com/Chishikii/URP-Render-Features/blob/main/Assets/Render%20Features/Outlines/OutlineRenderFeature.cs

It has a ton of comments and I would argue it's a better approach. Though if you want less changes, use this.

Summary

Sources/References

adrian-miasik commented 9 months ago

We can probably use CoreUtils.Destroy() instead to dispose of the materials.

Robinseibold commented 9 months ago

Great work @adrian-miasik! Will hopefully get time to review close and possibly merge it later this week.

adrian-miasik commented 9 months ago

Glad I could be of some service!

I'll probably push a couple more commits to this before review, with things such as using CoreUtils and checking RT's before usage (currently gets a null error).

Another thing of note, is that it seems like the new RTHandle system can't handle color and depth data at the same time? https://forum.unity.com/threads/can-i-get-a-rt-with-a-depthstencil-buffer-using-rthandles-if-not-why.1404010/

Chishikii commented 9 months ago

Another thing of note, is that it seems like the new RTHandle system can't handle color and depth data at the same time? https://forum.unity.com/threads/can-i-get-a-rt-with-a-depthstencil-buffer-using-rthandles-if-not-why.1404010/

You're absolutely right they can't which is a shame but oh well. We can however just reuse the cameras depth buffer. We just need to clear it before with cmd.ClearRenderTarget(). I've fixed all the issues I found in my repo.

Currently there is no option for occlusion objects. I'll try to add that as well once i have the time. Should be fairly simple now that everything else works.

Robinseibold commented 9 months ago

I'll probably push a couple more commits to this before review, with things such as using CoreUtils and checking RT's before usage (currently gets a null error).

Beautiful! Will track this.

roomyrooms commented 9 months ago

Heya, I appreciate the work you guys are doing as I'm currently using a lower Unity + URP version solely because of this package, which is the only one I've found to work in my circumstances. It's a bit out of my depth to fix anything in it ATM, so your work is great!

If it's not too complicated / time-consuming, do you think it'd be possible to include alpha testing in this fix as well? No worries if not. Thanks for your contribution :)

adrian-miasik commented 9 months ago

Alright, I've pushed the last couple commits I've had. Should be good to look over! 🤞

As for alpha testing, that should probably be a separate issue/PR unless it's a breaking feature change. (Maybe I'm misunderstanding?) @roomyrooms, can you elaborate a bit further on what you mean by alpha testing? I don't think this project supported transparent/alpha materials before (see #19), but perhaps you mean something different?

Robinseibold commented 9 months ago

Great work @adrian-miasik !

It took me some time to get it to work because of two reasons:

Anything occluder related is no longer needed because of proper depth setup in ConfigureTarget:

ConfigureTarget(normals, renderingData.cameraData.renderer.cameraDepthTargetHandle);
ConfigureClear(ClearFlag.Color, settings.backgroundColor);

So if you could just make the following changes:

I'd be more than happy to merge this into main.

adrian-miasik commented 9 months ago
  • It seems to only work if ScreenSpaceOutlineSettings.depthBufferBits = 0

I should have specified this, since this is the only way I could also get it to work. Whoops. 😅

ScreenSpaceOutlines.renderPassEvent < RenderPassEvent.BeforeRenderingSkybox results in a form of Outlines "shadow" artefact.

Do you have a visual reference/screenshot of what this looks like? Will still change the default to RenderPassEvent.BeforeRenderingSkybox like you suggested.

Remove any code related to occluder both in ScreenSpaceOutlinePass and ScreenSpaceOutlines

Understood!

Remove the UnlitColor shader

I don't think I've....Ohhh, yeah the occluder set shader.


Okay I'll get on this. Thanks for the detailed breakdown. I'll likely tackle this before the end of the week. Will report back and ping you once it's ready!

adrian-miasik commented 9 months ago

Okay @Robinseibold, I've made the necessary changes. Working as intended. Ready for re-review/merge! 🤞 image