Skip to content

Conversation

alex-vazquez
Copy link
Contributor


Purpose of this PR

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


Testing status

  • Check the problem of the bug is solved
  • Check other parameters
  • Change the max to a high number, change the value to that number, decrease the max.

Comments to reviewers

The main problem is that the properties used to modifiy the UI values are serialized, this is correct, but the range values are only set on the constructor,( something that is specified on the Volume Component), therefore they do not need to be serialized.

Just adding the attribute makes them to recover on whatever is on the constructor.

Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

Testing seems sufficient on this one ! ✔️

Copy link
Contributor

@RSlysz RSlysz left a comment

Choose a reason for hiding this comment

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

as long as the value is initialized by the constructor, it is good

@alex-vazquez alex-vazquez marked this pull request as ready for review February 24, 2021 15:21
@alex-vazquez alex-vazquez requested a review from a team February 24, 2021 15:22
@iM0ve iM0ve removed the request for review from a team February 25, 2021 09:52
@alex-vazquez alex-vazquez requested a review from a team March 1, 2021 10:04
@alex-vazquez
Copy link
Contributor Author

@Unity-Technologies/gfx-qa-hdrp , could you please do a quick pass on this PR?

@remi-chapelain remi-chapelain self-requested a review March 3, 2021 13:42
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

What I did test :

  • Tested to reduce max value of a ClampedParameter ✔️
  • Tested to increase min value of a ClampedParameter ✔️
  • Tested to modify quality settings of one override to see if upgrade behaved properly ✔️
  • Tested to add/remove item of an enum ✔️

The only thing that "sort of fail" in "common" parameters is upgrading a ColorParameter from hdr:false to hdr:true, you need to reset the component (or remove/add it) to make HDR info to appear in the color picker
However, Changing parameters to showAlpha gets shown instantly in the override in this PR (In master you still need to reset the override)

c85941ea0e8e990ab03eb2238d05f60f.mp4

public ColorParameter color = new ColorParameter(Color.black, hdr: false, showAlpha: false, showEyeDropper: false);

Also, not related to this PR, but the showEyeDropper is always present wether you set it to true or false. ¯\(ツ)

@remi-chapelain remi-chapelain self-requested a review March 3, 2021 18:06
@alex-vazquez alex-vazquez force-pushed the x-pipeline/ClamedFloatParameter-reload branch from 12b8449 to e502071 Compare March 5, 2021 11:14
@remi-chapelain remi-chapelain requested a review from RSlysz March 5, 2021 13:53
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

Thanks, this fixes the HDR issue.

As for what I said about the eyeDropped, I was looking at the wrong thing, it's working as expected !

@alex-vazquez alex-vazquez added the ready-to-merge Add this tag whenever your PR is ready to be merged. i.e, non draft, all reviewers approved, ABV label Mar 5, 2021
@sebastienlagarde sebastienlagarde merged commit 92a7746 into master Mar 5, 2021
@sebastienlagarde sebastienlagarde deleted the x-pipeline/ClamedFloatParameter-reload branch March 5, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Add this tag whenever your PR is ready to be merged. i.e, non draft, all reviewers approved, ABV SRP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants