wave-harmonic / crest

A class-leading water system implemented in Unity
MIT License
3.47k stars 478 forks source link

[HDRP] Option to disable mask renderer custom pass GameObject #727

Closed dkyeck closed 3 years ago

dkyeck commented 3 years ago

hi there,

i am just going over the code for the new maskrenderer custom pass. and i wonder why you do custom aabb tests here?

wouldn't it be possible to just grab the camera's culling result and then render the mask based on this? well, it would need you to add 2 materials to each chunk: 1: regular ocean, 2. mask mat. then use DrawRenderers and limit it by material.

i added the posssibility to add it directely to the scene as custom pass so that the gameObject part is skipped and we have full control over the setup. this also means that it may be limited to a local volume - which is a nice performance improvement.

daleeidd commented 3 years ago

and i wonder why you do custom aabb tests here?

I cannot remember why I didn't go with the culling result. Probably to not introduce too many changes. I'll have to look into why we had this in the first place and see if we can use it.

i added the posssibility to add it directely to the scene as custom pass so that the gameObject part is skipped and we have full control over the setup. this also means that it may be limited to a local volume - which is a nice performance improvement.

Sounds like a good idea. I'll add an option to bypass the game object.

huwb commented 3 years ago

i think we are doing custom aabb tests and also using a custom distance culling when underwater to aggressively cull tiles. these both live together in the code. i'm not really clear on how this affects this suggestion, but just mentioning.

dkyeck commented 3 years ago

using the culling result from the camera should already give you exactly this i think. at least the aabb test. you may add a more aggressive distance based culling on top of this.

dkyeck commented 3 years ago

i had another look into this: simply using the same logic as in the underwaterpostprocessing effect might already do the trick.

so in "void Execute":

// check for OceanRenderer.Instance if (OceanRenderer.Instance == null) { return; } // Check height above water if (OceanRenderer.Instance.ViewerHeightAboveWater > 2f) { return; }

daleeidd commented 3 years ago

using the culling result from the camera should already give you exactly this i think. at least the aabb test. you may add a more aggressive distance based culling on top of this.

Just remembered that we are also trying to bypass occlusion culling since that can cause gaps in the mask which we don't want.

huwb commented 3 years ago

Good point - the underwater post processing mask must be populated using the complete water-tight surface geo. I think baked occlusion was the case that broke it if that sounds right @daleeidd ?

dkyeck commented 3 years ago

good point, true. i haven't worked with occlusion culling for quite a while - but unchecking dynamic occlusion might do the trick here?

daleeidd commented 3 years ago

Good point - the underwater post processing mask must be populated using the complete water-tight surface geo. I think baked occlusion was the case that broke it if that sounds right @daleeidd ?

That's right. Issue #479.

unchecking dynamic occlusion might do the trick here?

We will want dynamic occlusion for the transparent pass. Can we have it for one and not the other?

dkyeck commented 3 years ago

Can we have it for one and not the other? maybe. but i am not that deep into configuring the RendererListDesc to get the renderers and bypassing occlusion.

daleeidd commented 3 years ago

4.12 will finally have most of this covered. The mask will also deactivate when the underwater effect deactivates. Also, you can control this behaviour by disabling/enabling the new UnderwaterRenderer component as it now is responsible for enabling/disabling the custom passes. Lastly, there is a debug option which disables the 2m height check if you really need it.

As for using the provided culling result, it is still something we will look into at some stage. But I think this should be enough to close this once 4.12 is released.

daleeidd commented 3 years ago

4.12 is out which covers pretty much all of this. Thanks for reporting.