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

[WIP][Rendering] Blendshapes (on behalf of Noa7/Noah7071) #2136

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Eideren
Copy link
Collaborator

@Eideren Eideren commented Jan 31, 2024

@noa7 / @Noah7071 has issues opening pull requests, I'm opening this one on his behalf with his approval

PR Details

Adds blendshape/morphtarget support for models, see #339

  • Importing process: Use the preexisting FBX pipeline, add a new toggle to the import UI to include morph targets, store morph targets into a new collection inside of the model asset. - Eideren: No toggle but I doubt it would see much use anyway
  • Animation Binding: Importing animations would have to retrieve and include morph target keys as well. When those animations run, the associated morph target of the model being animated should be affected as well.
  • Runtime: ModelComponent would have a new collection of weights, which could be floats with values from 0 to 1 for example. Each of those weights relate to a morph target in the Model referenced. Those values would be controlled from editor or at runtime through C#.
  • Rendering: Applying those morph targets over the model when rendering based on the associated weights, after the skeleton blending pass.
  • Documentation
    • Add a new sample for morph target, that sample contains a scene with a model and a small script. The script changes morph target blending at run time to show users how one would set it up.
    • Add a page to the Animation documentation that explains what morph targets are and how they can be used

Related Issue

Fix #339

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out. - Eideren: I'll try it out shortly

If @tebjan and/or @ykafia could take a look at this one too that would be great :)

@ykafia
Copy link
Contributor

ykafia commented Jan 31, 2024

Will try to take some time to review some things on Thursday/tomorrow :)

Copy link
Collaborator Author

@Eideren Eideren left a comment

Choose a reason for hiding this comment

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

Thanks again for those changes @noa7 / @Noah7071
Pretty sure you can delete Stride.Core.Assets.Editor_5yobaqbr_wpftmp.csproj from your changes - that's a temporary file
There's a lot of fairly minor things to clean up still, tried to mark most of them for you.

@@ -332,7 +332,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Stride.NuGetResolver", "..\
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "NuGetResolver", "NuGetResolver", "{158087CF-AF74-44E9-AA20-A6AEB1E398A9}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Stride.Core.Presentation", "..\sources\presentation\Stride.Core.Presentation\Stride.Core.Presentation.csproj", "{0C63EF8B-26F9-4511-9FC5-7431DE9657D6}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Best to roll this change back, afaict it is not necessary

Copy link
Member

Choose a reason for hiding this comment

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

It's actually correct but since it isn't related to the changes of this PR, let's revert it indeed. Otherwise it might conflict later with changes on the editor.

Position = new Vec4[value.Length];
for (int i = 0; i < value.Length; i++)
{
Position[i] = value[i] * .01f;
Copy link
Collaborator Author

@Eideren Eideren Jan 31, 2024

Choose a reason for hiding this comment

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

What's this multiplier about ? Is that because of how fbx scales things ? Is there a way to apply that on import through the fbx importer ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I too was also not sure about this. The weight data in .FBX ranges 0-100. I normalized to 0-1 as more intuitive ranging to developers. but can remove the multiplier and keep it as it is. Please let me know what you think!

Copy link
Collaborator Author

@Eideren Eideren Feb 1, 2024

Choose a reason for hiding this comment

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

Ah, okay, that makes sense. Can you move the scaling to the importer code, that way if we have other formats that support blend shapes in the future they won't have this fbx-specific scaler ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

Comment on lines 55 to 83
[DataContract]
public class Vec4
{
public float x { get; set; }


public float y { get; set; }

public float z { get; set; }

public float w { get; set; }

public Vec4()
{

}

public static implicit operator Vector4(Vec4 v)
{
return new Vector4(v.x, v.y, v.z, v.w);
}

public static implicit operator Vec4(Vector4 v)
{
return new Vec4() { x = v.X, y = v.Y, z = v.Z, w = v.W };
}


}
Copy link
Collaborator Author

@Eideren Eideren Jan 31, 2024

Choose a reason for hiding this comment

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

This struct seems unnecessary ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It's gone. Done!

Comment on lines +73 to +75

goto label_force_compile;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will have to remove that before merge, just something to keep in mind

Copy link
Contributor

Choose a reason for hiding this comment

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

Roger!

@@ -655,7 +669,7 @@ public ref class MeshConverter
auto buffer = buildMesh->buffer;
auto vertexBufferBinding = VertexBufferBinding(GraphicsSerializerExtensions::ToSerializableVersion(gcnew BufferData(BufferFlags::VertexBuffer, buffer)), gcnew VertexDeclaration(vertexElements->ToArray()), buildMesh->polygonCount * 3, 0, 0);

auto drawData = gcnew MeshDraw();
//auto drawData = gcnew MeshDraw();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Small cleanup here and further below

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

Comment on lines +222 to +224
<ProjectReference Include="..\..\engine\Stride.Rendering\Stride.Rendering.csproj">
<Project>{ad4fdc24-b64d-4ed7-91aa-62c9eda12fa4}</Project>
</ProjectReference>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what's going on here, can you quickly go over why you had to remove the <Private>False</Private> here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to manually update reference in Importer .vcxproj to the project Stride.Redenring vcproj. Each time I update the Stride.Redenring vcproj, removing the reference and adding back in .vcxproj. It should be auto update but for some reason it was not. I will roll back so this goes to merge!

Dictionary<int, Vector3> mappings = new Dictionary<int, Vector3>();
foreach (var tup_id_vec in Draw.VCPOLYIN)
{
if (!mappings.ContainsKey(tup_id_vec.Item2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the new TryAdd of the dictionary to avoid double lookup in the dictionary

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

@noa7707
Copy link
Contributor

noa7707 commented Feb 1, 2024

Project("{F

Will try to take some time to review some things on Thursday/tomorrow

@ykafia Thanks Youness. Look forward to your feedback!

for(int i=0;i<MAX_MORPH_TARGETS;i++)
{
float4 morphedShape=BSHAPEDATA[i* MAX_VERTICES+ vID][0];
blendPos=+float4(morphedShape[0]+BasisKeyWeight,morphedShape[1], morphedShape[2] , pos[3])*morphedShape[3];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did you mean to write blendPos+=float4... instead of blendPos=+float4... ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, error on my part, fixed to standard a= a+b format in latest

@tebjan
Copy link
Member

tebjan commented Feb 1, 2024

Thanks a lot for the work, I'll try to have a look at it next week.

@Noah7071
Copy link
Contributor

Noah7071 commented Feb 2, 2024

Thanks a lot for the work, I'll try to have a look at it next week.

Thanks @tebjan, Look forward to your feedback!

<AnalyzersNotToRemove Include="@(Analyzer)" Condition="$([System.String]::Copy(%(FullPath)).Contains('Stride'))" />
<AnalyzersNotToRemove Include="@(Analyzer)" Condition="$(FullPath.Contains('Stride'))" />
Copy link
Member

@Kryptos-FR Kryptos-FR Feb 4, 2024

Choose a reason for hiding this comment

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

Revert that. It looks like you had a conflict but resolved it the wrong way.

It would otherwise conflict with #2105

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now the build is fairly fragile wrt those lines - let's wait until the PR is finalized and revert only then that way Noah can keep working on this.

Comment on lines -51 to +52
<AnalyzersNotToRemove Include="@(Analyzer)" Condition="$([System.String]::Copy(%(FullPath)).Contains('Generator'))" />
<AnalyzersNotToRemove Include="@(Analyzer)" Condition="$([System.String]::Copy(%(FullPath)).Contains('SourceGeneration'))" />
<AnalyzersNotToRemove Include="@(Analyzer)" Condition="$(FullPath.Contains('Generator'))" />
<AnalyzersNotToRemove Include="@(Analyzer)" Condition="$(FullPath.Contains('SourceGeneration'))" />
Copy link
Member

Choose a reason for hiding this comment

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

Same.

<AnalyzersToRemove Include="@(Analyzer)" Exclude="@(AnalyzersNotToRemove)" />
<Analyzer Remove="@(AnalyzersToRemove)"/>
Copy link
Member

Choose a reason for hiding this comment

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

Same

@SeleDreams
Copy link
Contributor

Thanks for your work.
I'm pretty excited about this development since blendshapes are a feature I waited for for years but lacked the rendering skills and codebase experience to implement.

For anime styled 3D games this will be essential as those rely fully on blendshapes for facial expressions.

@Eideren Eideren marked this pull request as draft February 8, 2024 06:46
@noa7
Copy link
Contributor

noa7 commented Oct 30, 2024

Sorry for delayed response, was away. Will review this and get back next few days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Reserved) Morph Target Animation (blend shapes, shape keys)
9 participants