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

Fix issue causing asset dependencies to be ignored #438

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

pbbastian
Copy link
Contributor

@pbbastian pbbastian commented May 13, 2020

Purpose of this PR

Import dependencies from the Generator class were being ignored due to re-assignment of list without out. I changed it to AddRange on the input list. Also bumped the importer version so that all Shader Graphs get re-imported and then have the dependencies setup correctly.

The issue was introduced in this PR: https://github.com/Unity-Technologies/ScriptableRenderPipeline/pull/4940

Testing status

Manual Tests

  • Reproduced issue locally
  • Verified fix locally

To reproduce the issue, you can make any change to com.unity.render-pipelines.universal/Editor/ShaderGraph/Targets/UniversalTarget.cs and notice that without the fix it does not cause a re-import of Shader Graphs, even though it registers itself as a source dependency.

You can also modify put a Debug.Log in here to see what ends up getting declared, and notice that without the fix it does not declare anything.
https://github.com/Unity-Technologies/Graphics/blob/master/com.unity.shadergraph/Editor/Importers/ShaderGraphImporter.cs#L168

@pbbastian pbbastian requested review from a team as code owners May 13, 2020 10:58
@pbbastian
Copy link
Contributor Author

@Unity-Technologies/gfx-qa-bellevue Any update on this?

@ghost ghost added the needs-backport-9.x label Jun 4, 2020
@ghost
Copy link

ghost commented Jun 5, 2020

I can not get the debug.log to print anything. I put it where you said, and then made a change to UniversalTarget.cs on your branch, and it did not cause the debug.log to happen. Am I missing something?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Changed the location I put the debug log. It doesn't reimport on master, but I've confirmed it does on this branch, when UniversalTarget.cs is changed. I'll approve.

@pbbastian pbbastian merged commit ecf72b6 into master Jun 9, 2020
@pbbastian pbbastian deleted the sg/fix-import-dependencies branch June 9, 2020 08:19
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.

3 participants