Skip to content

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Dec 3, 2021


Purpose of this PR

Fix this case 1363279

The VFX relies on these require tranforms to populate local elementToWorld matrix (see here)


Testing status

Tested locally

Before

_before_fix_missing_need_transform.mp4

After

_after_fix_missing_need_transform.mp4

Comments to reviewers

See also this conversation

The changelog is actually only on VFX because I don't see another case where this missing declaration can cause an issue (maybe DOTS ?).

Missing World To Object matrix requirement
Only in VFX, I don't see other cases where this fix is applicable, maybe DOTS ?
@PaulDemeulenaere PaulDemeulenaere requested a review from a team December 3, 2021 14:55
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Dec 3, 2021
@PaulDemeulenaere PaulDemeulenaere marked this pull request as ready for review December 3, 2021 15:10
@PaulDemeulenaere PaulDemeulenaere requested a review from a team as a code owner December 3, 2021 15:10
@VitaVFX VitaVFX requested review from VitaVFX and removed request for a team December 3, 2021 18:26
Copy link

@VitaVFX VitaVFX left a comment

Choose a reason for hiding this comment

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

Thanks, looking solid!

Tested:

  • Ran VFX FTP SG Scene tests
  • Checked other conversions of Transform node (Vertex Stack and Fragment Stack)

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.

Just two things that might be good to add, but not necessary.
Rest looks good!

@PaulDemeulenaere
Copy link
Contributor Author

Double check locally at 65a242c (after adjustement)

_basic_check.mp4

@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Dec 8, 2021
@PaulDemeulenaere
Copy link
Contributor Author

Awaiting for Yamato result before merge ⏳

@PaulDemeulenaere
Copy link
Contributor Author

The failure about VFX_URP are expected.
See this conversion

@PaulDemeulenaere PaulDemeulenaere merged commit 807e271 into master Dec 8, 2021
@PaulDemeulenaere PaulDemeulenaere deleted the vfx/fix/1363279-invalid-tangent-space-using-sg branch December 8, 2021 16:21
PaulDemeulenaere added a commit that referenced this pull request Dec 13, 2021
* Fix TransformNode usage in VFX

Missing World To Object matrix requirement

* *Update changelog

Only in VFX, I don't see other cases where this fix is applicable, maybe DOTS ?

* Better filtering of needed transform

Fix issue #6475 (comment)
Fix issue #6475 (comment)
# Conflicts:
#	com.unity.shadergraph/Editor/Data/Util/SpaceTransformUtil.cs
#	com.unity.visualeffectgraph/CHANGELOG.md
@PaulDemeulenaere
Copy link
Contributor Author

PaulDemeulenaere commented Dec 13, 2021

Backport to 21.2 in blocked , see #6579

PaulDemeulenaere added a commit that referenced this pull request Dec 13, 2021
* Fix TransformNode usage in VFX

Missing World To Object matrix requirement

* *Update changelog

Only in VFX, I don't see other cases where this fix is applicable, maybe DOTS ?

* Better filtering of needed transform

Fix issue #6475 (comment)
Fix issue #6475 (comment)
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
PaulDemeulenaere added a commit that referenced this pull request Jan 28, 2022
* Fix TransformNode usage in VFX

Missing World To Object matrix requirement

* *Update changelog

Only in VFX, I don't see other cases where this fix is applicable, maybe DOTS ?

* Better filtering of needed transform

Fix issue #6475 (comment)
Fix issue #6475 (comment)
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
@PaulDemeulenaere
Copy link
Contributor Author

Cancelling the needs-backport-2021.2 because root change isn't available.

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.

4 participants