[HDRP]Improvements and fixes on Volumes Components Inspectors

[alex-vazquez]

Purpose of this PR

This PR is fixing these bugs

• https://fogbugz.unity3d.com/f/cases/1297795/
• https://fogbugz.unity3d.com/f/cases/1303477/
• https://fogbugz.unity3d.com/f/cases/1297789/

Changes

• Fixed spacing between property fields due to a harcoded Rect
• ALL/NONE changed to Toggles and maintain the state
• When ALL/NONE are selected we check if we should affect to the additional properties
• Adding localization to undo stack
• Adding unit tests
• Bunch of test checking changes done on additional properties of volumes components
• Testing the Volume component editors to check the set and get of the overrides states.
• Update CHANGELOG.md

Testing status

• Spacing of properties on Volume Components editors
• ALL/NONE are update only the correct overrides (when advanced mode is off they do not affect to advanced properties)
• ALL/NONE mantain the state if all the overrides are checked or if any of them are checked.

Comments to reviewers

Documentation changes will be sync with @JulienIgnace-Unity

[RSlysz]

My main concern is that the current test freeze the code a little to much. I would have prefered to only test the feature than actually test all component and froze the additional fields eddition without editing your test.

Then check the API visibility and if public is good, fix the missing argument in the documentation.

[RSlysz]

I am not sure you do this the good way here.

What you want to test is that the mechanism work with the [Attribute]. But then if there is a decision to add new Additional Property in a component it should not broke your test. I mean you test more than the feature here. You also test that each component should not change regarding the feature usage. Also in case we swap two lines, the order will change and the test will fail.

A better way would be to only test the feature. For that you create a new VolumeComponent only for test purpose and check its list is right. You don't need to test each component as it is redundant. But you want to check special cases. So something like:

• some fields are [Additional] (normal case) (optional: but there is also non additional fields in between)
• no fields are [Additional] (edge case)
• all fields are [Additional] (edge case)

Additionaly if you want test several case with same method, what you have done works. But you can also provide multiple data entry for the same test without using inheritance. See HDAdditionalLightDataTest.cs for an example.

[RSlysz]

@JulienIgnace-Unity Shouldn't you use this attribute too for your PR on the displays of them? Can be perhaps usefull if we have a equivalent to default inspector for volumes.

[alex-vazquez]

We were discussing this, an we will look at the merge.

[jenniferd-unity]

I am not familiar with that notation. what does the ? mean after the type

[alex-vazquez]

This is making a nullable value type (since c# 8 you can do it on reference types).

Basically it allow us to set null to the variable(default value)

I am using it here to allow me to check if it has been initialized.

For instance imagine a bool that is binded to UI. bool will only have true(yes) or false(no) as possibilities, but you want to make sure that the user selects if the option is wanted or not. So, you could not default to false, So instead of having another bool to check if the the user has selected yes or no for this, you make a bool?
now our bool? has three states null, true, false. This will allow us to check easily if the user has selected a value for it.

So, to obtain the value of a nullable reference or value type you could do it by .HasValue (but it think performance wise will be the same as doing ==null), and to obtain the value stored in our case a Vector2 simply use .Value.

[jenniferd-unity]

thanks for the tip!

[JulienIgnace-Unity]

One comment about the name of the Additional Attribute otherwise lgtm.

[RSlysz]

Better on the test side. 📗 lgtm :)

[jenniferd-unity]

Nice first PR :) definitely good improvements there!

[alex-vazquez]

Reviewed with @MaddalenaUnity , green light from she

[iM0ve]

Before PR

After PR

Concerns:

• The 2px change increases the vertical gaps between checkboxes, making already long volume overrides already longer. Discussed with UX that this is an expected side effect
• The horizontal gaps between checkboxes also increased due to alignment with NONE field. Maybe we shouldnt align with NONE? See image below cc @MaddalenaUnity

Whats tested:
Since its just a visual change I only did basic testing of described features and Undo check.

Conclusion
These are minor concerns and fine to merge it as is, but would be good to consider them if we can.

[alex-vazquez]

@iM0ve @MaddalenaUnity

Updating with your comments: