Stride.Physics.Test.Windows and Stride.Engine.Test.Windows projects fail to build on branch master

[JeroMiya]

Release Type: GitHub

Version: master

Platform(s): Windows

Describe the bug
master branch fails to build as of 9/21/2024. Failures occur in Stride.Engine.Test.Windows and Stride.Physics.Test.Windows

To Reproduce
Steps to reproduce the behavior:

1. Clone master branch of stride (following all instructions in README.md for prerequisites)
2. Open Stride.sln
3. Attempt to build (build fails)

Expected behavior
The Solution should build without errors

Screenshots
N/A

Log and callstacks

Build log is too long for github, here are a couple of screenshots (looks like VS aggregates the errors):

Additional context
N/A

[Kryptos-FR]

General notes on buiding

might not be related to the specific issue here, but good to know to help reproduce

It is sometimes necessary to first build the engine/tools before building the test projects. That's because of dependencies on the AssemblyProcessor and AssetCompilerApp, which if not built first might pick up another version (for instance from an installed version of Stride).

Try following those steps and see if the issue persists:

1. Make sure any instance of Visual Studio is closed.
2. Clean any previous build.
   a. Using git clean -xfd is more radical than using dotnet clean, but you might lose any uncommitted local changes.
3. Go to %LOCALAPPDATA%\Temp\Stride.
4. Delete the AssemblyProcessor
5. Build Stride without the tests
   a. Prefer building from the CLI as Visual Studio tends to lock the temporary location which might prevent copying the new version of the AssemblyProcessor. For the initial build, do it without the tests (-p:StrideSkipUnitTests=true) and without concurrent build (-m:1).
   msbuild build\Stride.build -t:Build -p:StrideSkipUnitTests=true -m:1
6. Build all
   a. either from CLI using msbuild build\Stride.build -t:Build
   b. or from Visual Studio

[Kryptos-FR]

For the current build issue, it seems to be related to #2291 (@Doprez) since we have this exact error message in the logs: "we currently do not handle these. This animation may not resolve properly".

Given that this regression is preventing tests from running successfully, it should either be fixed (ideally) or the test files should be updated to make it work.

[Doprez]

Its been a while but I know that PR was meant to fix many issues introduced from switching from the FBX importer to the AssImp one. If remember correctly that issue was supposed to be resolved by adding assimp.SetImportPropertyInteger(propStore, "IMPORT_FBX_PRESERVE_PIVOTS", 0); but that doesnt seem to work properly from the base AssImp library. I know @Eideren helped me with this PR to try and push a hotfix for all of the AssImp issues so maybe he can shed some more light.

I will look into it in the mean time.

Annoyingly you have to do a git clean -xfd every time you want to debug this since it looks like the asset compiler removes it from the build process? not really related to this but just something I dont fully understand since building it a second time seems to allow the tests to run as if those animations arent actually broken or maybe just arent used?

[Eideren]

Even more info over there in the source

stride/sources/tools/Stride.Importer.3D/MeshConverter.cs

Lines 529 to 532 in 763a529

	// TODO: Need to resample the animation created by the pivot chain into a single animation, have a look at the file hierarchy in Assimp's viewer to get a better clue
	// See: 'IMPORT_FBX_PRESERVE_PIVOTS' above and https://github.com/assimp/assimp/discussions/4966
	if (nodeAnim->MNodeName.AsString.Contains("$AssimpFbx$"))
	Logger.Error($"Animation '{animName}' contains a pivot bone ({nodeAnim->MNodeName.AsString}), we currently do not handle these. This animation may not resolve properly.");

I forgot half of why this nonsense exists, but from what I remember it's not trivial from our end - I'm not familiar enough with assimp to know exactly what would be possible, but it sounds likely that we can resample the animation for bones parented to those problematic nodes and then just remove them from the skeleton (as long as they themselves don't affect skinning)

[Doprez]

One thing I did to at least get around the failing asset compilation was to just change the log to a warning instead of an error. At least now I think I should be able to test the animation that's causing it but I don't even know where the animation "take001" is coming from to properly test it.

[Doprez]

Actually, going through each of the Knight fbx files I do see the $AssimpFbx$ looking at the model through Open3d model viewer on each of the FBX files so I guess IMPORT_FBX_PRESERVE_PIVOTS is working to some extent?

Walk.fbx

Edit: Ah ok, I found it. "Take001" shows in the animation tab of open3d.

[Kryptos-FR]

It doesn't seem like the projects actually fail to build, it does show errors in the logs but I saw the .exe were generated properly, following the steps I described earlier.

[Doprez]

The other good news is I dont think the problematic nodes are actually in use for the tests AFAICT?

he00_normal_idle.fbx

So that should mean the tests actually work which I think they do based on a visual comparison between the images I was checking in the generated folders. They are failing on my machine, I think due to the tsts expecting NVIDIA GeForce GTX 960 and from what I can see my images come out looking right on the transforms but they are brighter with my GPU which might be causing failure?

960 image:

3080 image:

We could change the code to just be a warning instead of an error for now and create an issue dedicated to fixing the problematic nodes issue? Unfortunately this is still a degradation from the original FBX importer we had but we could track it and so far has not been the cause for any recent animation issues (knock on wood).

If I'm not mistaken from some stuff I was reading online we would need to traverse the tree like @Eideren is saying and apply the root translations to the child nodes. I would need a model with child nodes that actually parent the problematic nodes to test properly.

[Eideren]

We have one in our tests: https://github.com/stride3d/stride/blob/master/sources/data/tests/knight/scenes/he03_run.fbx
The sword should be on the character's back:

But currently sits on the floor:

This is specific to assimp as we can see the same issue with another viewer using assimp:

I'm fine with making this a warning instead

[Doprez]

wait... how did you get that error? It is definitely broken for me in Open3D model viewer but works for me in Stride..

running game:

preview:

The files I had:
Knight.zip

[Doprez]

#2467 I pushed the change for the error to be a warning. Maybe instead of closing this issue we should change the topic since the root cause may still be a problem based on what @Eideren is seeing.

[Eideren]

I can repro your case by importing knight.fbx first as a model with skeleton, then importing he03_run.fbx as just an animation and filling in the skeleton and preview model with the knight's model and skeleton. If I don't import knight at all, and import he03_run.fbx as a model with animation and skeleton you should be able to see the issue above.

[Doprez]

Ah right, I should have realized my import process was different.. oops. Sounds good though, at least there is a repro available.