Skip to content

[VFX] Rename "Material Offset" in "Sorting Priority" #5646

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
Merged
merged 13 commits into from
Sep 30, 2021
Merged

[VFX] Rename "Material Offset" in "Sorting Priority" #5646

merged 13 commits into from
Sep 30, 2021

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Sep 14, 2021 •
edited
Loading

Purpose of this PR

This PR follows up the unification initiated by this change: #3966 from @alex-vazquez
It renames the "material offset" settings into "sorting priority" in VFX and use the same range in HDRP & URP.

Testing status

Yamato ⏳
Checking locally the behavior of material offset
_vfx_verify_offset

Comments to reviewers

The delayed and range attribute are incompatible. Since every setting update will trigger an invalidation, it's preferable to prioritize the delaying over the ranging.

I also renamed sortPriority in vfxSystemSortPriority to prevent the confusion in code (this value isn't visible for user)

See also this presentation
See also this discussion

The delayed mode doesn't work on slider:
_using_slider

That is why I replaced to a classic int field in VFXSlotContainerEditor
_using_delayed_field

@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Sep 14, 2021
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Sep 23, 2021
@PaulDemeulenaere PaulDemeulenaere marked this pull request as ready for review September 23, 2021 13:50
@github-actions github-actions bot added the HDRP label Sep 23, 2021
julienamsellem
julienamsellem reviewed Sep 23, 2021
View reviewed changes
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Sep 24, 2021
@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_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

VFX
/.yamato%252Fall-vfx.yml%2523PR_VFX_trunk

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.

alex-vazquez
alex-vazquez approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@alex-vazquez alex-vazquez left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. Great work. Some minor comments that I will leave up to you if you want to tackle them or not.

@sebastienlagarde
Copy link
Contributor

Are we ready to merge?

@PaulDemeulenaere
Copy link
Contributor Author

Are we ready to merge?

No, I need to apply minor suggestion from @alex-vazquez, I'm checking if we can factorize the +50/-50 constant somewhere

@PaulDemeulenaere
Copy link
Contributor Author

Waiting for some sanity run on Yamato before merging ⏳

PaulDemeulenaere added a commit that referenced this pull request Sep 30, 2021
@PaulDemeulenaere
Squashed commit of the following:
5f1ef7d 
commit d1ea7f9
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 30 11:05:33 2021 +0200

    Use StringBuilder instead of string addition

    Resolve case #5646 (comment)

commit 492527c
Merge: 9bbcea4 6f4f8d7
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 30 10:39:57 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

commit 9bbcea4
Merge: 7dacea5 11417d3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Sep 24 21:33:21 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

commit 7dacea5
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Sep 24 09:38:31 2021 +0200

    FIx minor issue

    Resolve #5646 (comment)
    Resolve #5646 (comment)

commit 8f7cc14
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 15:39:51 2021 +0200

    Apply rename in VFXHDRPSubOutput

commit 14efa73
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 15:26:08 2021 +0200

    Fix build & Apply rename in code as well

commit d5d7b76
Merge: 65f07ff 570437d
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 15:10:01 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

    # Conflicts:
    #	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderQueue.cs

commit 65f07ff
Merge: d5f1c36 0cf2fcb
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Tue Sep 14 10:08:25 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

commit d5f1c36
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Tue Sep 14 09:20:31 2021 +0200

    *Update changelog

commit 5515bd2
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Tue Sep 14 09:12:11 2021 +0200

    Update documentation

    & Clarify comment

commit f213fbd
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Sep 13 21:08:17 2021 +0200

    Fallback to DelayedIntField since Delayed & Range aren't compatibke

commit f89f534
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Sep 13 20:49:00 2021 +0200

    Rename sortPriority in vfxSystemSortPriority

    To avoid confusion with the "sortingPriority", this value isn't visible anyway

commit 26b15b3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Sep 13 17:47:31 2021 +0200

    Rename materialOffset in sortingPriority

(cherry picked from commit c223c95fea710cb49d1432dbc15907bb30b1c5b3)

# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
@PaulDemeulenaere PaulDemeulenaere mentioned this pull request Sep 30, 2021
Merged
@PaulDemeulenaere PaulDemeulenaere merged commit 9b914cf into master Sep 30, 2021
@PaulDemeulenaere PaulDemeulenaere deleted the vfx/fix/sorting-priority-ux-alignement branch September 30, 2021 13:42
PaulDemeulenaere added a commit that referenced this pull request Sep 30, 2021
@PaulDemeulenaere
Squashed commit of the following: (#5856)
4465a9b 
commit d1ea7f9
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 30 11:05:33 2021 +0200

    Use StringBuilder instead of string addition

    Resolve case #5646 (comment)

commit 492527c
Merge: 9bbcea4 6f4f8d7
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 30 10:39:57 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

commit 9bbcea4
Merge: 7dacea5 11417d3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Sep 24 21:33:21 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

commit 7dacea5
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Sep 24 09:38:31 2021 +0200

    FIx minor issue

    Resolve #5646 (comment)
    Resolve #5646 (comment)

commit 8f7cc14
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 15:39:51 2021 +0200

    Apply rename in VFXHDRPSubOutput

commit 14efa73
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 15:26:08 2021 +0200

    Fix build & Apply rename in code as well

commit d5d7b76
Merge: 65f07ff 570437d
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Thu Sep 23 15:10:01 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

    # Conflicts:
    #	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderQueue.cs

commit 65f07ff
Merge: d5f1c36 0cf2fcb
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Tue Sep 14 10:08:25 2021 +0200

    Merge branch 'master' into vfx/fix/sorting-priority-ux-alignement

commit d5f1c36
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Tue Sep 14 09:20:31 2021 +0200

    *Update changelog

commit 5515bd2
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Tue Sep 14 09:12:11 2021 +0200

    Update documentation

    & Clarify comment

commit f213fbd
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Sep 13 21:08:17 2021 +0200

    Fallback to DelayedIntField since Delayed & Range aren't compatibke

commit f89f534
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Sep 13 20:49:00 2021 +0200

    Rename sortPriority in vfxSystemSortPriority

    To avoid confusion with the "sortingPriority", this value isn't visible anyway

commit 26b15b3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Sep 13 17:47:31 2021 +0200

    Rename materialOffset in sortingPriority

(cherry picked from commit c223c95fea710cb49d1432dbc15907bb30b1c5b3)

# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
PaulDemeulenaere added a commit that referenced this pull request Oct 18, 2021
@PaulDemeulenaere
*Update all vfx
96a3a41 
Mainly due to this PR #5646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienf-unity julienf-unity julienf-unity approved these changes

@julienamsellem julienamsellem julienamsellem approved these changes

+1 more reviewer

@alex-vazquez alex-vazquez alex-vazquez approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
backported-2021.2 HDRP vfx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PaulDemeulenaere @sebastienlagarde @julienf-unity @alex-vazquez @julienamsellem