Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add 64 bits optionnal check for DXR wizard (for DXR player) #1632

Merged
merged 6 commits into from Sep 15, 2020
Merged

Add 64 bits optionnal check for DXR wizard (for DXR player) #1632

merged 6 commits into from Sep 15, 2020

Conversation

remi-chapelain
Copy link
Contributor

Purpose of this PR

RTX effects needs to have a 64 bits architecture for them to work in player but it was not mentioned anywhere.
This has just been added to the doc but since users don't read the doc, adding it to the optional check in the DXR wizard would be a great addition to what's already there.

image

image

Testing status

Not a lot to test there.
I just tried to change architecture to see if that is affecting correctly the check

Comments to reviewers

Just want to make sure that it makes sense and I did not make obvious mistakes.

@remi-chapelain remi-chapelain marked this pull request as ready for review August 18, 2020 15:53
@iM0ve
Copy link
Contributor

iM0ve commented Aug 20, 2020

Good addition, adds an informative text as in the images below for DXR.

Tested:

  • The warning is visible when 32 bit is selected
  • Warning is not visible in other non DXR menus
  • Warning turns to green checkbox when build is set to 64

Suggestion:
I think this option would benefit from having Fix button. Is there a downside why user would want to keep 32 bits in his DXR project? If 32 vs 64 is not an option, because 32 just doesnt work we should include this fix in the "Fix all" list if possible.

@remi-chapelain
Copy link
Contributor Author

Following @iM0ve comment, I have changed the check from optional to required and added a fix button for it.
I've check the fix button on a fresh new project and also if it works with the top "fix all" button.

image

iM0ve
iM0ve suggested changes Aug 20, 2020
View reviewed changes
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.

Can we address this warning?

image

@remi-chapelain
Copy link
Contributor Author

Obsolete function request fixed !

iM0ve
iM0ve approved these changes Aug 20, 2020
View reviewed changes
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.

All seems good now

@RSlysz
Copy link
Collaborator

RSlysz commented Aug 21, 2020

Following @iM0ve comment, I have changed the check from optional to required and added a fix button for it.
I've check the fix button on a fresh new project and also if it works with the top "fix all" button.

image

You just broke the resources group :O Please put the "Architecture 64 bits" outside of "DXR resources" (before or after but not inside especially if not indented.

RSlysz
RSlysz requested changes Aug 21, 2020
View reviewed changes
Copy link
Collaborator

@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.

Fix ordering and it should be good :)

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs
@@ -174,6 +174,8 @@ Entry[] entries
new Entry(InclusiveScope.DXR, Style.dxrStaticBatching, IsDXRStaticBatchingCorrect, FixDXRStaticBatching),
new Entry(InclusiveScope.DXR, Style.dxrActivated, IsDXRActivationCorrect, FixDXRActivation),
new Entry(InclusiveScope.DXR, Style.dxrResources, IsDXRAssetCorrect, FixDXRAsset),
new Entry(InclusiveScope.DXR, Style.dxr64bits, IsArchitecture64Bits, FixArchitecture64Bits),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this line one line above to not break grouping of "DXR Resources"

@remi-chapelain
Copy link
Contributor Author

You just broke the resources group :O Please put the "Architecture 64 bits" outside of "DXR resources" (before or after but not inside especially if not indented.

Did not realized thoses were grouped ! I moved the 64 bits thingy above ! Thanks for pointing it out.

anisunity
anisunity approved these changes Aug 26, 2020
View reviewed changes
Copy link
Contributor

@anisunity anisunity left a comment

Choose a reason for hiding this comment

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

lgtm

@sebastienlagarde sebastienlagarde merged commit f9f2fd3 into HDRP/staging Sep 15, 2020
@sebastienlagarde sebastienlagarde deleted the HDRP/64bits-optional-check-DXR-wizard branch September 15, 2020 12:09
@sebastienlagarde sebastienlagarde mentioned this pull request Sep 17, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastienlagarde sebastienlagarde sebastienlagarde approved these changes

@iM0ve iM0ve iM0ve approved these changes

@RSlysz RSlysz RSlysz approved these changes

@anisunity anisunity anisunity approved these changes

Assignees
No one assigned
Labels
dxr HDRP
Projects
None yet
Milestone
No milestone
5 participants
@remi-chapelain @iM0ve @RSlysz @sebastienlagarde @anisunity