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

[VFX] Add Graphics Buffer support #4109

Merged
merged 66 commits into from May 21, 2021

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Apr 7, 2021

Purpose of this PR

Adding Graphics Bufffer support defined by this technical design document.

Add Graphics Buffer in VFX Graph as a first class citizen. Graphics Buffer would be like any other exposed type (except not serialized). This will allow more flexibility and simplicity for users to pass and interpret their own data. At the moment they have to rely on textures for that.

Global approach ⬇️ (the generated code isn't up to date in this screenshot)
image

  • Add two operators :
    _sampling_graphics_buffer _buffer_count
    ⚠️ TODO : document newly added operator.

  • Expose VFXType with a new optionnal attribute : Usage.GraphicsBuffer & moved VFXType to runtime, sample of usage :

public struct Rectangle
{
	public Vector2 size;
	public Vector3 color;
}

[VFXType(VFXTypeAttribute.Usage.GraphicsBuffer)]
struct CustomData
{
	public Rectangle rectangle;
	public Vector3 position;
}

⚠️ TODO : document newly exposed API.


Testing status

Adding a new graphic test which is displaying a set of rectangle, see this code : https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/236/files#diff-6e84b835af122c8437c16eb04557d42aR178
_graphics_test_buffer

Adding several editor test :

  • CreateComponent_And_BindGraphicsBuffer : Check bindings & the value of GetNativeBufferPtr consistency
  • CreateComponent_And_BindGraphicsBuffer_And_(Reinit) : Check the binding buffer is kept while calling Reinit
  • CreateComponent_And_BindGraphicsBuffer_And_(DisableAndRenable) : Check the binding buffer is kept while disabling/renabling visualEffect
  • CreateComponent_And_BindGraphicsBuffer_And_(ChangeVisualEffectAsset) : Switch to another visualEffectAsset (which also have the same buffer exposed) (fixed by this C++ change)
  • CreateComponent_And_BindGraphicsBuffer_And_(EditSerializedObject) : Modifying the serialized property (fixed by this C++ change)
  • Slot_Sanitize_When_A_Sub_Slot_Is_Missing_Link : Cover a behavior added in VFXSlot.cs, we can correctly sanitize if VFXType change its structure adding a field, if slot count doesn't correspond, try to copy by field name.

Tested locally some invalid VFXType :

  • Private field aren't supported in VFXType if GraphicsBuffer usage
public struct Type_Using_Private_And_GraphicsBuffer
{
	public int a;
	private int b;
}
  • Use another struct which isn't a VFXType
{
	public int a;
}

[VFXType]
public struct Type_Using_Not_VFX_Type_SubType
{
	public float a;
	public NotAVFXType b;
}
  • Unsupported blittable type by the VFX
public struct Type_Using_Not_Supported_Type_By_VFX
{
	public float a;
	public byte b;
}
  • Not blittable type
public struct Type_Not_Blittable
{
	public float a;
	public Texture2D b;
}

Note : I'm expecting other error case but I tried to cover the most obvious possible incompatibilities.

I've run yamato to check the expected result :

⚠️ TODO : upload image references ⚠️


Comments to reviewers

⚠️ This PR require C++ code : https://ono.unity3d.com/unity/unity/pull-request/123849/_/graphics/vfx/feature/add-graphics-buffer-support (not final, still an ownership issue to resolve see this conversation)

Known limitation :

  • We aren't checking the type of buffer, GraphicsBuffer.Target.Structured is kind of expected view

Known issue (wip) :

  • The inspector is displaying the GraphicsBuffer

This change requires C++

(cherry picked from commit 3c6f166)
(cherry picked from commit 76b908d)
(cherry picked from commit d9bc375)
(cherry picked from commit 1cd0c6b)
(cherry picked from commit 75e9b92)
(cherry picked from commit 3ac5fde)
(cherry picked from commit 741d304)
It checks stride & count

(cherry picked from commit c672220)
(cherry picked from commit 14fef70)
(cherry picked from commit 8651ac3)
(cherry picked from commit 7245458)
(cherry picked from commit afb9ccf)
(cherry picked from commit e235d4b)
(cherry picked from commit e980c68)
(cherry picked from commit edd6bac)
(cherry picked from commit 7dae601)
(cherry picked from commit 3c171c4)
(cherry picked from commit 4fe6f9c)
(cherry picked from commit 24868a9)
(cherry picked from commit dce4897)
Copy link
Contributor

@VladNeykov VladNeykov left a comment

Choose a reason for hiding this comment

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

Looking good, no outstanding issues
test doc

PaulDemeulenaere and others added 2 commits May 14, 2021 16:18
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
@PaulDemeulenaere
Copy link
Contributor Author

The needed change in C++ landed (see https://ono.unity3d.com/unity/unity/changeset/5676fbb6fbea) , launching Yamato on really last trunk : https://ono.unity3d.com/unity/unity/changeset/281d883a3da809b5684d799dea25486c92813302 (master didn't update the trunk head yet) on d9458b4

@PaulDemeulenaere PaulDemeulenaere marked this pull request as ready for review May 20, 2021 09:17
@julienf-unity julienf-unity merged commit cf9467a into master May 21, 2021
@julienf-unity julienf-unity deleted the vfx/feature/add-graphics-buffer-support branch May 21, 2021 16:02
sebastienlagarde added a commit that referenced this pull request Jun 4, 2021
sebastienlagarde added a commit that referenced this pull request Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants