Fix range for HDRP Asset settings

[adrien-de-tocqueville]

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1344000

fix int and float parameters in the HDRP asset that didn't have range specifiers.

Note 1: Some fields are delayed to avoid saving the asset when dragging a value. Ideally all settings should be like that otherwise it's pretty unusable, but delayed fields don't work with range or min attributes, so i filed a bug: https://fogbugz.unity3d.com/f/cases/1346276/

Note 2: I didn't find a way to have the hdrp asset settings use the same range as the equivalent variables from the volume overrides (without adding a ton of static variables). Maybe there is a way using reflection ? for now I duplicated the values and therefore they should be updated in both places now.

---

Testing status

tested putting values to invalid ranges and verified that the are correclty clamped

[github-actions]

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

[JMargevics]

What's tested looks sufficient ✅

[RSlysz]

Adding range for inspector should also be reflected in the scripting API.
And in-code documentation should specify the edition condition if possible.

[RSlysz]

The Range Attribut only work for inspector display.

You need to transform the field visibility and use a property to actually ensure we cannot go over ranges by scripts.

E.G.:

public int toto = 42;

should become

[Range(2, 64)]
[FormerlySerializedAs("toto")]
private int m_Toto = 42;

public int toto
{
    get => m_Toto;
    set 
    {
        m_Toto = Mathf.Clamp(value, 2, 64);
    }
}

---

Also, I never tried the attribute on an array. Have you tested it really clamp in the inspector for each cells? (being curious here)

[adrien-de-tocqueville]

Yes the range tag works on an array, it's pretty cool.
For the property part, i don't see how to do that with array though, because i can't define a set/get on the array elements. I could change them to objects and use indexers, but that changes the way serialization works. I would get

AOStepCount:
    values: 200000000600000010000000

instead of

AOStepCount: 200000000600000010000000

which requires a migration and also breaks existing code accessing the values using serializedproperties, and I don't think there's a way around that.

[RSlysz]

Ha yes. I don't see how we can do it nicely without migration nor reimplementing the array :/ So lets drop that for array for now. But then please add comments on the limitation in the documentation of these fields.

---

The good things to do would be to have the array private and an accessor method like

private int[] m_Toto;
public void SetToto(int index, int value); // can also check out of range
public int GetToto(int index); // can also check out of range

because here, even the size can be changed by script as we can reassign a whole new array and if I am not wrong, it seams that the size is driven by the mechanism here and not fully editable.
But doing this is a breaking API change :/ So I assume we don't go this way right now. (@JulienIgnace-Unity for awareness)

---

By the way, changing things for a SerializedProperty path is acceptable. It is not direct public API anyway but more a reflection like mechanism.

[adrien-de-tocqueville]

actually there is kind of a way. It would require having both:

• the array for serializing (but moved to private with [SerializedField])
• a new public non serialized IntRangeParameter object, with indexer to preserve the API and the custom setter (I am not sure if that would be considered an API change though, do you know ?)
• use the trick described here to copy the data from one to the other before and after serialization

[adrien-de-tocqueville]

nevermind it wouldn't work, they would need to both have the same name

[RSlysz]

Well at this point you could go this way. But it is almost like reimplementing the array

public class ClassContainingArrayThatChangedRange
{
    [FormerlySerializedAs("toto")]
    [SerializedField]
    [Range(2, 64)]
    int[] m_Toto;
    
    public struct TotoAccessor
    {
        private ClassContainingArrayThatChangedRange reference;
        public int[int index]
        {
            get => reference.m_Toto[index];  //also check out of range
            set => reference.m_Toto[index] = Mathf.Clamp(value, 2, 64);  //also check out of range
        }
    
        public int length => reference.m_Toto.length;
        
        //any other relevant array API
        
        TotoStruct(ClassContainingArrayThatChangedRange encapsulatedClass)
            => reference = encapsulatedClass;
    }
    
    [System.NonSerialized]
    public TotoAccessor toto = new TotoAccessor(this); // or assign it in constructor
}

---

ISerializationCallbackReceiver have a small cost at each serialization/Deserialization. I am not sur of the usage of those array and I am pretty sur it only works on MonoBehaviour and we are in a custom class here.

[RSlysz]

Breaking API change are :

• previous API deprecated or modified (hard change)
• additional public API that don't modify what existed before (soft change)

Soft change are most of the time ok. In SemVer it would be in the minor update if we followed it completely. While Hard change require a deprecation plan and should only be in major change in SemVer.
Now that we are bound to unity version, it is realease management that define what is acceptable or not. I know they allowed soft changes in several cases so I assume it would be ok to do that without changing major unity version.

[github-actions]

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

[sebastienlagarde]

Note to self: redirect to master once hd/bugfix is merge