Open JeroMiya opened 2 months ago
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:
git clean -xfd
is more radical than using dotnet clean
, but you might lose any uncommitted local changes.%LOCALAPPDATA%\Temp\Stride
.AssemblyProcessor
-p:StrideSkipUnitTests=true
) and without concurrent build (-m:1
).
msbuild build\Stride.build -t:Build -p:StrideSkipUnitTests=true -m:1
msbuild build\Stride.build -t:Build
b. or from Visual StudioFor 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.
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?
Even more info over there in the source https://github.com/stride3d/stride/blob/763a529cc15836a7a6436453718e6ca08317dda6/sources/tools/Stride.Importer.3D/MeshConverter.cs#L529-L532 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)
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.
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.
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.
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.
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
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
https://github.com/stride3d/stride/pull/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.
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.
Ah right, I should have realized my import process was different.. oops. Sounds good though, at least there is a repro available.
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:
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