Hd/fix lookdev environment inspector undo [Hold]

[RSlysz]

Purpose of this PR

Fix issue https://fogbugz.unity3d.com/f/cases/1296931/ (nullref when exiting play mode) (fixed in another PR that landed prior this one)
Fix issue https://fogbugz.unity3d.com/f/cases/1296655/ (embedded Environment Inspector was not supporting Undo/Redo)
Fix error in the console when an environment is selected and a domain reload occurs.

---

Testing status

Tested that

• exiting play mode with LookDev opened don't raise issue in the console
• domain reload with LookDev opened and environment selected don't raise issue in the console
• Undo works on fields and give the previous value
  • name
  • Sky with sun
  • Rotation
  • Exposure
  • Sun Position
  • Sun Position button (compute brightest position)
  • Shadow tint
• Redo works on field
  • Sky with sun

---

Comments to reviewers

Backport for 10.x.x could be nice

@Unity-Technologies/gfx-qa-hdrp a full pass testing on undo redo mixed with domain reload qnd exit play mode could be usefull

[TomasKiniulis]

Checking out the PR, I seem to be getting the following:

ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
System.ThrowHelper.ThrowArgumentOutOfRangeException (System.ExceptionArgument argument, System.ExceptionResource resource) (at <9577ac7a62ef43179789031239ba8798>:0)
System.ThrowHelper.ThrowArgumentOutOfRangeException () (at <9577ac7a62ef43179789031239ba8798>:0)
UnityEditor.Rendering.LookDev.DisplayWindow.GetSelectedThumbnail () (at C:/Users/Tomas/Documents/Github_Projects/Graphics2/com.unity.render-pipelines.core/Editor/LookDev/DisplayWindow.EnvironmentLibrarySidePanel.cs:255)
UnityEditor.Rendering.LookDev.EnvironmentElement.RefreshIfNecessary () (at C:/Users/Tomas/Documents/Github_Projects/Graphics2/com.unity.render-pipelines.core/Editor/LookDev/Environment.cs:514)
UnityEditor.Undo.Internal_CallUndoRedoPerformed () (at <f3e15d0b0fba46daa8106ea31d8c9721>:0)
UnityEditor.EditorApplication:Internal_CallGlobalEventHandler()

This happens when changes are made in in Environment Settings, no HDRI is selected and changes are undone

[RSlysz]

@TomasKiniulis this should be fixed with last commit.

[TomasKiniulis]

Thanks @RSlysz! That works now, although I'm hitting another issue while checking now.

When "Sky With Sun is Set" and is changed to another one, tweak any other parameter and press undo twice. You will notice that undo step to change back HDRI is skipped.
https://user-images.githubusercontent.com/50582134/103790241-9ce4b100-5049-11eb-9dcd-51a3c6aaefda.mp4

Other than that looks all good, I've did a pass changing values before entering playmode, in playmode, after exiting playmode and undoing/redoing, no issues here so far.

[RSlysz]

@TomasKiniulis I fixed above issue and found another one: If you change a thing on an Environment then select anothe one and change things here, then you can undo up to the selection change. So I added the selection in the undo/redo and this fix it.

I also found that calling Undo.RegisterObject (which is required to be able to use Undo/Redo) dirty the scene. Yes I know it is weird as there is nothing registered that is in the scene. But I checked Master and when we add an Environment to the EnvironmentLibrary, the Undo.RegisterObject already do this. So it is not a regression even if the current PR make a lot more call on it.

Should we post pone this fix because of this or should we keep this QoL improvment?

[TomasKiniulis]

@TomasKiniulis I fixed above issue and found another one: If you change a thing on an Environment then select anothe one and change things here, then you can undo up to the selection change. So I added the selection in the undo/redo and this fix it.

Thanks for the fix!

I also found that calling Undo.RegisterObject (which is required to be able to use Undo/Redo) dirty the scene. Yes I know it is weird as there is nothing registered that is in the scene. But I checked Master and when we add an Environment to the EnvironmentLibrary, the Undo.RegisterObject already do this. So it is not a regression even if the current PR make a lot more call on it.
Should we post pone this fix because of this or should we keep this QoL improvment?

Oh! I'd be more up for postponing, until there's a fix for Undo.RegisterObject. It's better to have less unexpected scene saving functionality in my opinion.

Filed a ticket for undo issue here: https://fogbugz.unity3d.com/f/cases/1306519/

[sebastienlagarde]

note from Seb. This PR is still block by C++ PR and high likely it will not land before 22.1, maybe we will simply close this PR in this end.

[RSlysz]

As there is an initiative to redo the LookDev as a dedicated package, I bet this version will be not fixed on time :/

[sebastienlagarde]

FYI the fogbugzz is now solve on C++ side

[sebastienlagarde]

Closing this PR as we will deprecated the lookdev in a near future