Skip to content

Conversation

remi-chapelain
Copy link
Contributor

@remi-chapelain remi-chapelain commented Aug 27, 2021

Purpose of this PR

image

This PR adds a "Current Scene" section (only in the HDRP+DXR Wizard).
This section contains a "check" button that triggers the "Check Scene Content For HDRP Ray Tracing" Edit > Rendering menu.
The goal of this is to inform users that this check exists and could help fix a bad setup.
It also helps to understand what this menu really checks instead of searching for clues in the doc.

CheckContentForRTX.mp4

I believe we could create other functions like to benefit HDRP only setup as well because some things in the checks (like verifying if meshes has null materials, renderer in LODs... etc could also benefit Non DXR HDRP setup.


Testing status

Tested opening the wizard with any of the tabs selected by default ✔️
Tested clicking on "Check" with and without issue to see if console was giving information properly ✔️
Tested that the menu was still working properly ✔️


Comments to reviewers

I'm open for suggestions about:

  • Better place to put this check instead of creating another section.
  • Better wording of what the check is doing.
  • Better way to ensure that the button will always stays rather than creating a function (CheckForRTX()) that always returns false.

@remi-chapelain remi-chapelain requested a review from RSlysz August 27, 2021 09:52
@github-actions
Copy link

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.

@github-actions github-actions bot added the HDRP label Aug 27, 2021

bool CheckForRTX()
{
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this check is "non-resolvable", this is how I ensure that users will be able to always click the button, but there may be another cleaner solution.

@remi-chapelain remi-chapelain requested review from a team and sebastienlagarde August 27, 2021 11:58
@remi-chapelain remi-chapelain marked this pull request as ready for review August 27, 2021 11:58
@remi-chapelain remi-chapelain changed the title Hdrp/add rtx check wizard [HDRP] Adding a new button in HDRP+DXR Wizard to check scene content for Ray Tracing issues Aug 27, 2021
@RSlysz
Copy link
Contributor

RSlysz commented Aug 30, 2021

I dislike this idea A LOT.

Wizard was always planed and designed only for PROJECT configuration. Now we open the door to scene by scene change check.
Why this is wrong:

  • Wizard is not meant to be open each time we create a new scene so there is no easy way for user to check the new scene he create comply some constraint. Each time he should open back the wizard or have it always opened somewhere taking unneeded space.
  • There is many edge case you need to check now: What hapens if we are currently viewing a prefab stage instead of a scene? What about multiple opened scene?
  • In the end, what is added is just a checker not a fixer. We introduced checker to some corner case that we cannot easily change because no scripting API was available. Or because user can want to do it in some context. It is not meant to be generalized more than this : the more we add check in Wizard, the more we dilute information to solve project issue.

Please discuss this change with UX.

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.

Please check with UX (see above)

@RSlysz
Copy link
Contributor

RSlysz commented Aug 30, 2021

So currently it is already a menu item.
Do we have other similar action we want to check onto the scene? I would prefer having a new dedicated small Scene Configuration Checker window with really few line that can be docked somewhere as it take almost no space in this case. Still be sure to add checks on the edges cases listed above.

@remi-chapelain
Copy link
Contributor Author

remi-chapelain commented Aug 30, 2021

Thanks for the feedbacks !

Wizard was always planed and designed only for PROJECT configuration. Now we open the door to scene by scene change check.

I agree this is debatable indeed. The initial goal was to use the "notoriety" of the wizard to bring users attention to this unknown/unused feature for raytracing setup. But maybe that's not the proper location to put it.

Do we have other similar action we want to check onto the scene?

As of right now, we don't, but as I said in the PR description, some check done in this menu could also benefit non DXR hdrp setup (like checking if renderer has null materials, checking if meshfilter has null mesh.. etc)

Checker window with really few line that can be docked somewhere as it take almost no space in this case.

I believe having a new "checker window" with this check in it, will suffer from the same problem the command menu is already suffering from: discoverability. So that's not really a solution either.

What happens if we are currently viewing a prefab stage instead of a scene?

The check (as was the command menu already doing) is still going to check the current opened scene.

What about multiple opened scene?

If you have multiple scene opened, the check will iterate through all the gamobjects in the current opened scenes and treat the multiple scenes as one big opened scene.

Please check with UX (see above)

Only comment from @TheoWong-pixel from now is to make sure that the console will open if it's closed to be sure users won't miss messages which I believe also makes sense.


However, with all your concerns, don't really know if that makes sense to still move forward with this :).
What does the rest of the QA thinks about it?

@sebastienlagarde
Copy link
Contributor

I would advice to not go further with more windows that do check, and I am agree with RemiS.

The Wizard is already a huge investment on our side and something very specific to HDRP. I don't want to make it more complex or have more functionnality, it should stay really simple. We have already enough issue reported with it, don't want to have more :D

@remi-chapelain
Copy link
Contributor Author

Let's close it then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants