Skip to content

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Nov 2, 2021

Purpose of this PR

Added new debug widgets for the debug menu to show what diffusion profiles are currently in use by the volume system.

At the same time, i made a slight change to the volume debug table to improve readability, that you can see in the first screenshot below. I changed the column header name to be only the profile name, and added a row to link to the gameobjects contaning the profile. (that was an artist feedback from a long time ago)

This is how they show in the volume debug menu in the editor:
_menu_editor

This is the same as above, but in play mode:
_menu_playmode

This is a test to show the look of the two new widgets, but outside of the volume debug menu table
_menu_editor2


Testing status

See the screenshots
Tested in a standalone build

@github-actions
Copy link

github-actions bot commented Nov 2, 2021

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://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, 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
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

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.

@adrien-de-tocqueville adrien-de-tocqueville force-pushed the hd/diffusion-profile-debug-menu branch 2 times, most recently from 77faa29 to 3f44a9f Compare November 2, 2021 12:17
@adrien-de-tocqueville adrien-de-tocqueville force-pushed the hd/diffusion-profile-debug-menu branch from 3f44a9f to df050cc Compare November 2, 2021 13:03
@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as ready for review November 3, 2021 09:15
@adrien-de-tocqueville adrien-de-tocqueville requested review from a team, alex-vazquez and sebastienlagarde and removed request for a team November 3, 2021 09:15
Copy link
Contributor

@alex-vazquez alex-vazquez left a comment

Choose a reason for hiding this comment

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

Really minor things, I think you should also have @arttu-peltonen and @JulienIgnace-Unity on the review.

@arttu-peltonen arttu-peltonen self-requested a review November 3, 2021 11:46
Copy link
Contributor

@arttu-peltonen arttu-peltonen left a comment

Choose a reason for hiding this comment

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

Looks great from my side. Only thing UI wise I noticed - the table view in the editor UI looks like it has darker font color now. Does this mean the UI is disabled, and you can't interact with it? It would be useful to be able to jump to the diffusion profile asset by clicking at it. If that's not easy to do, it's fine to leave as future improvement.

@adrien-de-tocqueville
Copy link
Contributor Author

adrien-de-tocqueville commented Nov 3, 2021

Indeed the font is darker because everything is disabled now it makes more sense (previously everything was disabled except the text fields). Though I reenabled the paramter column as this is not modifiable anyway, so it looks like that:
image
But the objectfields are clickable even if they are disabled !

@sebastienlagarde sebastienlagarde merged commit 20d3f36 into master Nov 4, 2021
@sebastienlagarde sebastienlagarde deleted the hd/diffusion-profile-debug-menu branch November 4, 2021 20:11
Copy link
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Did a quick check with multiple cameras, multiple volumes affecting only certain cameras and it looks good.

Ran into a couple of Rendering Debugger bugs, but none are PR related:
https://fogbugz.unity3d.com/f/cases/1379200/
https://fogbugz.unity3d.com/f/cases/1379202/

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.

5 participants