Skip to content

Fixed the display name of a Volume Parameter when is defined the attribute InspectorName #3348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

alex-vazquez
Copy link
Contributor


Purpose of this PR

The inspectors were shown like this:

image

When the user has defined them like this:

[Header("High noise")][Header("High noise")]    [InspectorName("Size and occurance"), Tooltip("Increase to make patches SMALLER, and frequent")] public ClampedFloatParameter _HighNoiseSpaceFreq = new ClampedFloatParameter(0.1f, 0.1f, 1f);
 [InspectorName("Wobble frequency"), Tooltip("Increase to make patches wobble more often")] public ClampedFloatParameter _HighNoiseTimeFreq = new ClampedFloatParameter(0.05f, 0f, 2f);
 [InspectorName("Pit depth"), Tooltip("Increase to make patches \"deeper\"")] public ClampedFloatParameter _HighNoiseAmplitude = new ClampedFloatParameter(0.4f,0f, 4f);
 [InspectorName("Wind strength"), Tooltip("Increase to make patches move faster")] public ClampedFloatParameter _HighNoiseMoveSpeedScale = new ClampedFloatParameter(0.2f, 0f, 10f);

So the attribute InspectorName was not properly readed to show the correct label for those properties.

After the changes in this PR, they appear correctly.

image


Testing status

  • Checking that the display name for Volume Parameters of Volume Components without a custom inspector are shown correctly.
  • Added unit test.

Comments to reviewers

It was missing an entry to check the InspectorNameAttribute, I've changed it to be a switch, and I extracted it to a method to be able to call it from a Unity test.


Comments for QA

Grab the test project from here

and check the TestVolumeComponent on the inspector.

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.

Nice test :)

@alex-vazquez alex-vazquez marked this pull request as ready for review February 4, 2021 07:52
@alex-vazquez alex-vazquez requested review from a team as code owners February 4, 2021 07:52
@alex-vazquez alex-vazquez requested a review from a team February 4, 2021 07:52
Copy link
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

Whats tested:

  • Tested that InspectorName attribute is now used to display name, instead variable name previously.
  • If InspectorName does not exist, the variable name is still used as fallback.
  • Parameters are now correctly grouped using the Header attribute.
  • Upgraded the project from Core to HDRP to see if there is any unexpected behaviour after update.
  • For above testing the fogbuz sample was used, to verify that the users issue is gone

Screenshot 2021-02-04 at 12 13 39

@alex-vazquez alex-vazquez force-pushed the HDRP/fix-InspectorName-VolumeComponents branch from 54751a1 to 30a0211 Compare February 4, 2021 10:31
@sebastienlagarde sebastienlagarde merged commit 369a27f into master Feb 5, 2021
@sebastienlagarde sebastienlagarde deleted the HDRP/fix-InspectorName-VolumeComponents branch February 5, 2021 00:19
sebastienlagarde pushed a commit that referenced this pull request Feb 14, 2021
sebastienlagarde pushed a commit that referenced this pull request Mar 3, 2021
sebastienlagarde added a commit that referenced this pull request Mar 3, 2021
* Merge Hd/bugfix #3313

* Fix frame count in editor #3173

* Fixed the display name of a Volume Parameter when is defined the attribute InspectorName #3348

* Added the default quality settings to the HDRP asset for RTAO, RTR and RTGI (case 1304370). #3362

* Formatting

* Windows vulkan reference screenshots

Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com>
Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.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.

7 participants