Skip to content

[URP] Factorize template & SubTarget<UniversalTarget> #5467

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
Closed
wants to merge 4 commits into from
Closed

[URP] Factorize template & SubTarget<UniversalTarget> #5467

wants to merge 4 commits into from

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Aug 30, 2021 •
edited
Loading

Purpose of this PR

This PR follows up the initial refactor PR #3705, but adding for Sprite & Decal Targets:

  • Use UniversalSubTarget instead of SubTarget<UniversalTarget>
  • Use the über ShaderTemplate

It will simplify the new ShaderGraph integration for VFX : #5448

Testing status

Yamato ⏳

Comments to reviewers

See also this conversation

@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Aug 30, 2021
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Aug 30, 2021
@PaulDemeulenaere PaulDemeulenaere marked this pull request as ready for review August 30, 2021 16:58
@PaulDemeulenaere PaulDemeulenaere requested review from a team as code owners August 30, 2021 16:58
@PaulDemeulenaere PaulDemeulenaere requested review from cdxntchou and joshua-davis and removed request for joshua-davis August 30, 2021 16:59
PaulDemeulenaere
PaulDemeulenaere commented Aug 31, 2021
View reviewed changes
com.unity.render-pipelines.universal/Editor/ShaderGraph/Targets/UniversalDecalSubTarget.cs
public override void Setup(ref TargetSetupContext context)
{
base.Setup(ref context);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

N.B.: Decal Sub Target still uses an independent template DecalPass.template
 and it doesn't include the kUberTemplatePath.
It probably deserves to be factorized with the common template at some point.

@PaulDemeulenaere
Copy link
Contributor Author

FYI: I moved the target from master to HDRP/sg-vfx-integration-improved-to-urp but I can move it back to master if there is any interest to include these change earlier in common master branch.

@PaulDemeulenaere PaulDemeulenaere changed the base branch from master to HDRP/sg-vfx-integration-improved-to-urp September 1, 2021 16:30
@simon-engelbrecht-soerensen simon-engelbrecht-soerensen requested review from simon-engelbrecht-soerensen and removed request for a team September 2, 2021 09:08
Base automatically changed from HDRP/sg-vfx-integration-improved-to-urp to master September 17, 2021 06:43
simon-engelbrecht-soerensen
simon-engelbrecht-soerensen approved these changes Sep 22, 2021
View reviewed changes
Copy link

Choose a reason for hiding this comment

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

Tested with Sprite & Decal shadergraphs, havent seen any regressions.

@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Sep 23, 2021
@PaulDemeulenaere
Merge branch 'master' into urp/vfx/factorize-template-to-ease-sg-support
03cd89b 
# Conflicts:
#	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteCustomLitSubTarget.cs
#	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteLitSubTarget.cs
#	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteUnlitSubTarget.cs
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Sep 30, 2021
pbbastian
pbbastian approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@pbbastian pbbastian left a comment

Choose a reason for hiding this comment

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

LGTM

cdxntchou
cdxntchou approved these changes Oct 14, 2021
View reviewed changes
Copy link

@cdxntchou cdxntchou left a comment

Choose a reason for hiding this comment

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

Looks good

phi-lira added a commit that referenced this pull request Oct 20, 2021
@phi-lira
Factorize template & SubTarget<UniversalTarget> #5467
d72e0b2 
commit 03cd89b
Merge: d3ec70a 9fd657c
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 09:36:27 2021 +0200

    Merge branch 'master' into urp/vfx/factorize-template-to-ease-sg-support

    # Conflicts:
    #	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteCustomLitSubTarget.cs
    #	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteLitSubTarget.cs
    #	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteUnlitSubTarget.cs

commit d3ec70a
Merge: 7502dba 2116818
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Aug 30 13:51:30 2021 +0200

    Merge branch 'master' into urp/vfx/factorize-template-to-ease-sg-support

commit 7502dba
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Aug 30 12:31:35 2021 +0200

    Use common UniversalSubTarget for Sprite & Decal

commit 8d271f3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Aug 30 10:41:17 2021 +0200

    Use uber template for all urp target
@phi-lira phi-lira mentioned this pull request Oct 20, 2021
Merged
phi-lira added a commit that referenced this pull request Oct 22, 2021
@phi-lira
Factorize template & SubTarget<UniversalTarget> #5467
31106af 
commit 03cd89b
Merge: d3ec70a 9fd657c
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 09:36:27 2021 +0200

    Merge branch 'master' into urp/vfx/factorize-template-to-ease-sg-support

    # Conflicts:
    #	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteCustomLitSubTarget.cs
    #	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteLitSubTarget.cs
    #	com.unity.render-pipelines.universal/Editor/2D/ShaderGraph/Targets/UniversalSpriteUnlitSubTarget.cs

commit d3ec70a
Merge: 7502dba 2116818
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Aug 30 13:51:30 2021 +0200

    Merge branch 'master' into urp/vfx/factorize-template-to-ease-sg-support

commit 7502dba
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Aug 30 12:31:35 2021 +0200

    Use common UniversalSubTarget for Sprite & Decal

commit 8d271f3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Aug 30 10:41:17 2021 +0200

    Use uber template for all urp target
@phi-lira
Copy link
Contributor

Merged onto staging branch. Staging branch landed onto master on #6085

@phi-lira phi-lira closed this Oct 22, 2021
@PaulDemeulenaere
Copy link
Contributor Author

Thanks @phi-lira, it will ease future factorization in VFX

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

@pbbastian pbbastian pbbastian approved these changes

@simon-engelbrecht-soerensen simon-engelbrecht-soerensen simon-engelbrecht-soerensen approved these changes

@cdxntchou cdxntchou cdxntchou approved these changes

@kennytann kennytann kennytann approved these changes

Assignees
No one assigned
Labels
universal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PaulDemeulenaere @phi-lira @pbbastian @simon-engelbrecht-soerensen @cdxntchou @kennytann