-
-
Notifications
You must be signed in to change notification settings - Fork 977
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
Adds native dependencies for win-arm64 #2644
base: master
Are you sure you want to change the base?
Conversation
<StrideNativePathLibsWindows>libCompilerRt.lib libCelt.lib</StrideNativePathLibsWindows> | ||
<StrideNativePathLibsUWP>libCompilerRt.lib libCelt.lib Xaudio2.lib</StrideNativePathLibsUWP> | ||
<StrideNativePathLibsWindows>libCelt.lib</StrideNativePathLibsWindows> | ||
<StrideNativePathLibsUWP>libCelt.lib Xaudio2.lib</StrideNativePathLibsUWP> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this lib is not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linker was happy. It would've complained I assume.
@@ -515,10 +515,10 @@ internal class Utilities | |||
[DllImport("DxtWrapper", CallingConvention = CallingConvention.Cdecl, CharSet = CharSet.Unicode), SuppressUnmanagedCodeSecurity] | |||
private extern static void dxtComputePitch(DXGI_FORMAT fmt, int width, int height, out int rowPitch, out int slicePitch, CP_FLAGS flags); | |||
|
|||
[DllImport("DxtWrapper", CallingConvention = CallingConvention.Cdecl, CharSet = CharSet.Auto), SuppressUnmanagedCodeSecurity] | |||
[DllImport("DxtWrapper", CallingConvention = CallingConvention.Cdecl, CharSet = CharSet.Ansi), SuppressUnmanagedCodeSecurity] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ansi ? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we look in the C header, the method is defined with char
(1 byte). With Auto we don't know what it will take, clearly on arm64 it took the wrong one. I debugged that part, the data I got on unmanaged was garbage with Auto. With Ansi all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit. I quickly tested on Linux DDS and TGA files, no regression here 👍
11c7e9e
to
6ceecf7
Compare
@Kryptos-FR @Jklawreszuk I just force pushed - added more info in the PR summary. For those deps where Stride maintains a fork I created PRs, and for those where it doesn't I added patch files or forked myself if the original repo was hosted on github. |
- The used versions of those dependencies are often rather dated and didn't build out of the box on win-arm64 / Visual Studio 2022, minimal patches had to be applied - As far as I understood PVRTexLib is delivered as binary only and there's no win-arm64 - therefor added a check to not load it at all. In the end it's only needed by the asset compiler, which in turn is platform neutral. However when invoking it on a arm64 machine, the `dotnet path/to/asset/compiler` will launch it in arm64. If there would be some option to launch it in x64 we wouldn't have any issues with asset compiling. Still the editor might have issues, since its running in process? - Removed the FreeImageNET load - it's not used anywhere in code - Fixes some PInvoke signatures in DxtWrapper
6ceecf7
to
aa6e55c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick initial pass, just based on a read through
@@ -138,6 +144,11 @@ | |||
<Exec Condition="'%(StrideNativeCFile.Extension)' == '.cpp'" Command=""$(StrideNativeClangCommand)" -gcodeview -fno-ms-extensions -nobuiltininc -nostdinc++ $(StrideNativeClangCPP) $(StrideNativeClang) -DNEED_DLL_EXPORT -o "$(OutputObjectPath)\win-x64\%(StrideNativeCFile.Filename).obj" -c "%(StrideNativeCFile.FullPath)" -fms-extensions -DWINDOWS_DESKTOP -target x86_64-pc-windows-msvc" /> | |||
<MSBuild Projects="$(MSBuildThisFileDirectory)WindowsProjects\WindowsDesktop\WindowsDesktop.vcxproj" Targets="Build" Properties="StrideNativeOutputName=$(StrideNativeOutputName);StrideNativeOutputDir=$(StrideNativeOutputPath)\runtimes\win-x64\native;StrideDependenciesDir=$(MSBuildThisFileDirectory)..\..\deps\;StrideNativePathLibs=libNativePath.lib $(StrideNativePathLibsWindows);StrideNativeProjectFolder=$(ProjectDir);StrideNativeProjectObjFolder=$(OutputObjectPath)\win-x64;Configuration=$(Configuration);Platform=x64" StopOnFirstFailure="true" /> | |||
|
|||
<MakeDir Directories="$(OutputObjectPath)\win-arm64" Condition="'$(StrideNativeWindowsArm64Enabled)' == 'true'"/> | |||
<Exec Condition="'%(StrideNativeCFile.Extension)' != '.cpp' AND '$(StrideNativeWindowsArm64Enabled)' == 'true'" Command=""$(StrideNativeClangCommand)" -gcodeview -fno-ms-extensions -nobuiltininc -nostdinc++ $(StrideNativeClang) -DNEED_DLL_EXPORT -o "$(OutputObjectPath)\win-arm64\%(StrideNativeCFile.Filename).obj" -c "%(StrideNativeCFile.FullPath)" -fms-extensions -DWINDOWS_DESKTOP -target aarch64-pc-windows-msvc" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse my unfamiliarity with the Stride3D codebase here, but have you tested this with clang?
If it works, it might even be beneficial to compile everything with clang, instead of MSVC, I found when I moved Blender builds to clang-cl instead of MSVC, I got a 46% perf improvement in rendering workloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep the changes arm64 related only. Modernising the native build chain would certainly be great but I believe that should be done separately. There are already attempts to so, for example one using zig which in turn uses clang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we need another issue for it.
Awesome job on picking this up again after I unceremoniously gave up. I know my attempt was pretty messy, but hopefully there was something worthwhile to be taken from it, in particular the strange arm64 msbuild registry location. |
Probably introduced by ddc7423
While trying to get a grip on the build error of the Visual Studio package on teamcity:
|
Did you try to build on Linux or Windows? It's ok to fail on Linux because those package are for Visual Studio which is Windows-only. |
sources/tools/Stride.VisualStudio.Package.Tests/Stride.VisualStudio.Package.Tests.csproj
Show resolved
Hide resolved
Turns out the Visual Studio update removed .NET 8 on the build machine. @xen2 just re-installed it, should be ok now. |
PR Details
Allows to develop and also run games on
win-arm64
.Needs MSVC v143 - VS2022 C++ ARM64 build tools (Latest)
dotnet path/to/asset/compiler
will launch it in arm64. If there would be some option to launch it in x64 we wouldn't have any issues with asset compiling. Still the editor might have issues, since its running in process?The game studio also runs on arm64, but one must set the runtime identifiers in Stride.GameStudio/Editor/Assets.Presentation accordingly. Would be nice to add an arm64 version to the installer, but different topic I'd say.
FFmepg
checkout.bat says to use n3.3.3 tag however I didn't manage to get it to build. Was therefor using release/3.4 as a base.
Basically had to cherry-pick two commits from main branch in regards to arm64.
A quick test with importing a video asset and playing it later in the game worked.
https://github.com/azeno/FFmpeg/tree/win-arm64
FreeImage
Recast
Guessed the commit by comparing the checked in header files to those in the original repo. Added the used commit-id to checkout.bat file.
Needed PRs in other repos
Related Issue
#2397
Types of changes
Checklist