-
Notifications
You must be signed in to change notification settings - Fork 855
New UI for subdivision controller on probe volume #5529
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
Conversation
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. SRP Core 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the property thing in the UI.
One thing to note though is that users will lose their valuer of the old subdivision controller but I guess it's fine since it's still experimental.
if (serialized.highestSubdivisionLevelOverride.intValue < 0) | ||
serialized.highestSubdivisionLevelOverride.intValue = maxSubdiv; | ||
|
||
serialized.highestSubdivisionLevelOverride.intValue = EditorGUILayout.IntSlider("Highest Subdivision Level", serialized.highestSubdivisionLevelOverride.intValue, 0, maxSubdiv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing EditorGUI.BeginProperty
and EndProperty to correctly handle presets, prefabs, undo, etc...
(you can take the code before your PR as a reference to add it)
NOTE: There is an issue (we always had, before too) with what we display. Looking at it. |
Fixed the info boxes, now we give proper info (before upper range in profile was wrong and the lower bound wrong on the probe volume itself) |
… into HDRP/new-pv-subdiv-ui # Conflicts: # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumeSceneData.cs
* New UI * Move comment * Couple of annoying whitelines * Review fix * Fix subdiv info boxes. Co-authored-by: Julien Ignace <julien@unity3d.com>
This changes the UX as discussed in slack, should hopefully be a bit more clear.
We still have a fairly poor nomenclature around subdivision terms, I logged a task to look into it.
Video to show it:
NewSubdiv.mp4