Skip to content

Conversation

FrancescoC-unity
Copy link
Contributor

Fix for: https://fogbugz.unity3d.com/f/cases/1263426/

Unfortunately it looks like we don't have callbacks for when reset is called on Editors, moreover the scope change doesn't trigger on Reset.

The simplest way would be to update the diffusion profile every time regardless if the inspector is drawn. This is not super slow so might as well. I left the original branch there to make sure it is clear why we don't do the scope change and to change the code if we ever get a call back (Asked editor devs)

What have I tested: The repro case and checked that diffusion profiles still work on a simple sphere with an SSS material and the profile I am editing assigned.

Copy link
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Both Diffusion Profile previews and Transmittance previews are getting correctly updated now.

However, there seems to be additional issues regarding diffusion profile reset

  • Reset does not match the settings of newly created Diffusion Profile
  • After reset Diffusion Profile breaks and is not updated anymore changing values

Occurs in PR and HDRP/staging Logged a ticket here: https://fogbugz.unity3d.com/f/cases/1269462/

Copy link
Member

@alelievr alelievr left a comment

Choose a reason for hiding this comment

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

IIRC the diffusion profile is a ScriptableObject so we should be able to use this callback to update the cache: https://docs.unity3d.com/2020.1/Documentation/ScriptReference/ScriptableObject.Reset.html ?

If not I'm good with the current changes :)

@FrancescoC-unity
Copy link
Contributor Author

IIRC the diffusion profile is a ScriptableObject so we should be able to use this callback to update the cache: https://docs.unity3d.com/2020.1/Documentation/ScriptReference/ScriptableObject.Reset.html ?

If not I'm good with the current changes :)

I think I tried without success, but a lot of time has passed :) Will try later

@sebastienlagarde
Copy link
Contributor

I think I tried without success, but a lot of time has passed :) Will try later
poke on the PR regarding this statement ^

@FrancescoC-unity
Copy link
Contributor Author

I think I tried without success, but a lot of time has passed :) Will try later
poke on the PR regarding this statement ^

Yup confirmed that it does not work, we need to do like it is done in this PR

@sebastienlagarde sebastienlagarde merged commit 16da7f1 into HDRP/staging Sep 8, 2020
@sebastienlagarde sebastienlagarde deleted the HDPR/fix-diffusion-profile-reset branch September 8, 2020 23:40
@sebastienlagarde sebastienlagarde mentioned this pull request Sep 17, 2020
6 tasks
sebastienlagarde added a commit that referenced this pull request Nov 16, 2020
* Fix warning in HDAdditionalLightData OnValidate #885

* Fix XR multipass #1133

* Update Override-Micro-Shadows.md (#1236)

* Added disclaimer to volumes (#1247)

* Added missing volumes API toc entry (#1259)

* Fix XR Display providers not getting zNear and zFar updated on them. #1269

* Remove MSAA debug mode when renderpipeline asset has no MSAA #1289

* Added menu items doc

* Moved requirements in toc to make it consistent with URP/VFX #1352

* Fix compilation issue when XR is not available #1391

* Fix an issue with dynamic resolution handler in case no OnResolutionChange callback is defined #1403

* Change a multi compile to multi compile local to reduce number of keyword #1444

* Docs quality fixes #1445

* Added enable Volume snippet

* Update Volume-Override-Enable-Override.md

* Update HDCamera.cs

* Fix layer-related error caused by disabling emissive area light mesh #1506

* Make sure sun icon is not clipped in lookdev window #1515

* Fix for LookDev displaying probes as pink spheres #1521

* Fix issue with disc area light editor not updating #1526

* Fixed an issue where only one of the two lookdev views would update when changing the default lookdev volume profile. #1529

* Hdrp/update decal atlas when texture changes #1532

* Fix Screen position out of view frustum issues when planar reflection probe is at same camera location #1537

* Make sure diffusion profile is correct upon its editor reset #1538

* Added propagating nans doc (#1562)

* Added propagating nans doc

* Added information about HDRP's NanTracker

* GFXGI-237: Force update for static skies when camera type is set to S… #1570

* Hdrp/docs/shader additions #1580

* Added information about HDRP not upgrading particle shaders (#1601)

* Added information about HDRP not upgrading particle shaders

* Update Upgrading-To-HDRP.md

* Update Upgrading-To-HDRP.md

* Changed cog to gear to adhere to style guide rules #1611

* Added build settings setup (#1631)

* Fixing the remapping of Min/Max parametrizations values to Amplitude parametrizations values

* Updating UI to match documentation of LayeredLit

* Hdrp/fix/terrain layer parametrization #1678

Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com>
Co-authored-by: Fabien Houlmann <44069206+fabien-unity@users.noreply.github.com>
Co-authored-by: JordanL8 <lewis.jordan@hotmail.co.uk>
Co-authored-by: robinb-u3d <robinb-u3d@users.noreply.github.com>
Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com>
Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com>
Co-authored-by: JulienIgnace-Unity <julien@unity3d.com>
Co-authored-by: Ben Spencer <github@raytracing.co.uk>
Co-authored-by: Jean-Philippe Grenier <jeanphilippe@unity3d.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants