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.

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

Auto detect render pipelines #2114

Merged

Conversation

thoxey
Copy link

@thoxey thoxey commented Jul 25, 2023

This PR adds code to automatically detect the render pipeline and adds utility functions that returns the correct IMaterialDescriptorGenerator based on the current pipeline.

It also implements a use case in the Vrm10Importer that means that in a URP project at runtime a VRM10 is loaded in using the correct materials.

thoxey added 2 commits July 25, 2023 12:23
These allow for the creation of materialdescriptorgenerators that use the current render pipeline
… used elsewhere

in my local version of UniVRM I have the change I made, and will leave other changes to your discretion
@vrm-github-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@ousttrue ousttrue added this to the v0.113 milestone Jul 26, 2023
@Santarh
Copy link
Contributor

Santarh commented Jul 26, 2023

Jenkins

@ousttrue ousttrue modified the milestones: v0.113, v0.114 Jul 26, 2023
Copy link
Contributor

@Santarh Santarh left a comment

Choose a reason for hiding this comment

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

Big thanks for your pull request on UniVRM.

There seems to be a problem with some of the unit tests.
Could you please look at the code again and fix it?

Perhaps fixing the code I pointed out will fix it.


if (currentPipeline == null)
{
return RenderPipelineTypes.Unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be RenderPipelineTypes.BuiltinRenderPipeline when currentPipelin is null.

Copy link
Author

@thoxey thoxey Aug 2, 2023

Choose a reason for hiding this comment

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

I will try make the changes. Just need some more details.

Do you mean that the null check is not working?

Can I see the output of the unit test in question?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that currentPipeline == null means current render pipeline is Built-in RenderPipeline in Unity.

But this implementation has different behaviour.
The loader can't decide RenderPipelineTypes correctly when Project's RenderPipeline is Built-in RenderPipeline.

Suggested change is below.

Suggested change
return RenderPipelineTypes.Unknown;
return RenderPipelineTypes.BuiltinRenderPipeline;

How to run unit tests

Select Window -> General -> Test Runner then Test Runner Windows is opened.
In Test Runner Window, select Run All button then tests are running.
You can find failed tests in the list.

Copy link
Author

Choose a reason for hiding this comment

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

Please see latest changes

@Santarh
Copy link
Contributor

Santarh commented Aug 15, 2023

Jenkins

@Santarh Santarh self-requested a review August 15, 2023 03:33
Copy link
Contributor

@Santarh Santarh left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@Santarh Santarh merged commit 15f9de1 into vrm-c:master Aug 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants