Skip to content

Conversation

@Ethereal77
Copy link
Contributor

@Ethereal77 Ethereal77 commented Jul 18, 2023

PR Details

Replace SharpDX bindings for DXGI, Direct3D 11, Direct3D 12, the DirectX Shader Compiler, and any other use of SharpDX with Silk.NET bindings.

Description

SharpDX is the base for all the Direct3D (both 11 and 12) graphics frameworks in Stride. But SharpDX is no longer active nor mantained, the repository is archived, and the NuGets are still (and forever) netstandard2.0. Also, it is based on a class-based abstraction over the DirectX interfaces.

Meanwhile, Silk.NET is a new bindings library created to take advantage of modern .NET, use the modern C# performance-oriented primitives and features (like Span<T>, for example), and it's designed more as a raw bindings as close as possible to the underlying DirectX APIs, without class-based abstractions. Its main drawback however is that it is inherently unsafe (in the context of using pointers and memory management directly) That however is of no importance to Stride as it will be under the hood of the Stride.Graphics framework.

This PR consists of the following points:

  • Convert Direct3D 11 framework to Silk.NET.Direct3D11.
  • Convert Direct3D 12 framework to Silk.NET.Direct3D12.
  • Convert DirectX Shader Compiler to Silk.NET.Direct3D.Compilers.
  • Improve the documentation on DirectX-related methods and types, removing references to SharpDX or to the native constants, types, or functions where appropriate.
  • Introduce small convenience types and helper functions to aid in the implementation of interop improvements like this one, now and in the future.

Related Issue

This PR tries to solve issue #432. It is similar to PR #1123 by ykafia, but that one is pretty out of date at this point, so it was easier to start afresh branching the current master.

Motivation and Context

The motivation for converting to Silk.NET is twofold:

  • The SharpDX libraries are pretty solid and stable at this point. However its design filosophy (more to be used as a framework itself ala XNA) and its age makes us lose opportunities to improve performance, control memory allocations more tightly and take advantage of modern C#. Also it is no longer mantained, so no more bug fixes or improvements of any kind.
  • Silk.NET is actively mantained and uses all the utilities modern C# and .NET has to offer. As it is actively mantained it's easier to get bug fixes and improvements, and more importantly, access to future (and current) technologies we may be interested in (like DirectStorage, DXVA, etc).

If all goes well with this PR, you should not see a difference in Stride. Maybe better performance and memory consumption. The improvement is mainly to be more up to date and more inline with modern .NET.

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.

@Ethereal77 Ethereal77 mentioned this pull request Sep 14, 2023
7 tasks
@ly3027929699
Copy link

i can't build this branch via some errors has not resolve

@Eideren
Copy link
Collaborator

Eideren commented Oct 21, 2023

@ly3027929699 Yeah, the PR is still marked as wip, you shouldn't expect it to work as is yet :P

Comment on lines 38 to 44
HResult result = dxgi.CreateDXGIFactory2(debugFlag, SilkMarshal.GuidPtrOf<IDXGIFactory2>(), (void**) &factory2);

if (result.IsFailure)
result.Throw();

NativeFactory = factory2;
ComPtr<IDXGIFactory2> adapterFactory = new() { Handle = factory2 };
Copy link

Choose a reason for hiding this comment

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

FWIW we have overloads now that can simplify this

ComPtr<IDXGIFactory2> adapterFactory = dxgi.CreateDXGIFactory2<IDXGIFactory2>(debugFlag);

It's mentioned in the 2.17 blog post: https://dotnet.github.io/Silk.NET/blog/apr-2023/silk2170.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen them. I'm first making it work and checking things, then I'll make it pretty using the overloads.

@ykafia ykafia mentioned this pull request Nov 18, 2023
7 tasks
@Aggror Aggror linked an issue Dec 9, 2024 that may be closed by this pull request
@Kryptos-FR
Copy link
Member

Kryptos-FR commented Feb 24, 2025

@Ethereal77 I'm trying to merge master into this branch to solve the conflicts introduced by #2643. However, I don't seem to have the rights to push to your repo. Could you add me as a contributor?

nvm: I was able to fix the conflicts in the web editor

@Ethereal77
Copy link
Contributor Author

@Kryptos-FR I can indeed add you as a contributor. No problem with that.

However, I still have changes locally that I have not yet pushed. I'm gradually distilling the changes from another repo I have. As this has taken more time than I wanted it to take initially it has been a bit of a mess, where most of the changes I've made to a "prototype" repo where I try things.
My plan is to rebase all this on Stride's master instead of merging master here.

So, with this in mind, if you merge master to this branch now you'll make my life a little miserable right now 😁

What I can do is push some of the changes I want to do today and then do a rebase to the current master. What do you think?

P.D: If you still want me to add you as contributor, as I said above, no problem.

@Kryptos-FR
Copy link
Member

Kryptos-FR commented Feb 24, 2025

@Ethereal77 Oh oh. I saw your comment after merging master. You should be able to rebase on top of the commit before the merge to ignore it.

If you are unsure how to do it (and not want to mess with your local branches), if you add me as a contributor I can restore the remote branch to where it was before the merge.

@Ethereal77
Copy link
Contributor Author

Don't worry. I'll finish some changes I have in progress, rebase on master at the previous commit and force push. That would do it, isn't it?
I know I should have this branch as clean and as merge-ready as possible, but as it is now it's a little mess with more than 100 uncommitted files I'm slowly reviewing, cleaning, and pushing.
I also have many more modified files in which I've added or expanded documentation, but I wanted to add that progressively and on top of the more important changes (the Silk.NET conversion) instead of mixing them, with the hope that reviewing the changes is easier this way.

@Ethereal77
Copy link
Contributor Author

@Kryptos-FR I've rebased on latest master (except the last merge commit). Hope everything went well.

@Kryptos-FR
Copy link
Member

Kryptos-FR commented Feb 24, 2025

@Ethereal77 looks like, except maybe the reference to Silk.NET.D3DCompiler in Directory.Package.props, shouldn't it be Silk.NET.Direct3D.Compilers instead?

@Ethereal77
Copy link
Contributor Author

It looks like the package was named Silk.NET.D3DCompiler but has been renamed recently (?). I've just upgraded to the latest Silk.NET version and now it is indeed named Silk.NET.Direct3D.Compilers.

<PackageReference Include="SharpDX.Direct3D12" Condition="'$(TargetFramework)' == '$(StrideFramework)'" />
<PackageReference Include="SharpDX.D3DCompiler" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
<PackageReference Include="Vortice.Vulkan" Condition="'$(TargetFramework)' == '$(StrideFramework)'" />
<PackageReference Include="Silk.NET.Direct3D11" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
Copy link
Member

Choose a reason for hiding this comment

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

UWP might be removed by #2450. Just to keep in mind in case it gets merged. No need to fix it now.

<ItemGroup>
<ProjectReference Include="..\Stride.Shaders.Parser\Stride.Shaders.Parser.csproj" />
<PackageReference Include="SharpDX.D3DCompiler" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
<PackageReference Include="Silk.NET.Direct3D.Compilers" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
Copy link
Member

Choose a reason for hiding this comment

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

UWP might be removed by #2450. Just to keep in mind in case it gets merged. No need to fix it now.

<PackageReference Include="SharpDX.MediaFoundation" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
<PackageReference Include="SharpDX.Direct3D11" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
<PackageReference Include="SharpDX.Direct3D12" Condition="'$(TargetFramework)' == '$(StrideFramework)'" />
<PackageReference Include="Silk.NET.Direct3D11" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
Copy link
Member

Choose a reason for hiding this comment

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

UWP might be removed by #2450. Just to keep in mind in case it gets merged. No need to fix it now.

<ProjectReference Include="..\..\engine\Stride.Graphics\Stride.Graphics.csproj" />
<PackageReference Include="Microsoft.Win32.Registry" />
<PackageReference Include="SharpDX.Direct3D11" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
<PackageReference Include="Silk.NET.Direct3D11" Condition="'$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkUWP)'" />
Copy link
Member

Choose a reason for hiding this comment

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

UWP might be removed by #2450. Just to keep in mind in case it gets merged. No need to fix it now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

microsoft/microsoft-ui-xaml#9983 (comment)
there is also the idea to make it available again

@Ethereal77
Copy link
Contributor Author

Yeah, I'm following the issue on UWP support. Will remove any reference entirely if not needed anymore.

@Ethereal77
Copy link
Contributor Author

I'm starting to upload most of the changes that I think are mostly ready.

As Direct3D 11 and 12 share a common way of doing things, I'm pushing just D3D 11 and DXGI for now. D3D 12 will come a bit later when I get feedback on D3D11.

I've just started with the Buffer class with the code changes. I have ready the added/improved documentation I've also writing, and I want to push it in a separate commit.

The question is: Should I do the documentation pass now? Or postpone it for a later time?
I ask this because the documentation is hugely verbose and will make more difficult reviewing the changes. There are a lot of additions also, so it will appear as a lot of green in diffs.

@VaclavElias
Copy link
Contributor

Thank you @Ethereal77. Lots of updates here. I would be ok with docs later but in this case I woud like a feedback from @Eideren or @Kryptos-FR regarding the docs because in some cases it might be very important.

@Kryptos-FR
Copy link
Member

Kryptos-FR commented Feb 26, 2025

I think it's fine to push the documentation here. As long as it's on a separate commit, it's always possible to do an offline review that ignore that particular commit.

But let's wait for @Eideren's point of view. I'm not a graphics expert so my review will be a bit more superficial, and is less likely to be encumbered by lots of comments 😅.

@Eideren
Copy link
Collaborator

Eideren commented Mar 1, 2025

Yep, I agree with @Kryptos-FR, if you already have the docs, might as well include it. My thinking is that it may also help the reviewer

@xen2
Copy link
Member

xen2 commented Jan 5, 2026

I took the liberty to push some changes to your repo:

  • merged all the D3D12/Vulkan changes from my branch (merging was a bit of a mess, but it worked out).
  • Reverted Effect/Param Collection changes. Somehow it was not compiling, and even after fixing it I had runtime crash; it's probably trivial to fix but I prefer to keep this in a separate PR for proper review.

I will do some additional testing but hopefully we're getting very close to merging it in master!

@xen2 xen2 marked this pull request as ready for review January 5, 2026 16:32
@xen2 xen2 merged commit b989a9c into stride3d:master Jan 6, 2026
1 of 5 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Roadmap Jan 6, 2026
Comment on lines +88 to +90
// Somehow, this makes the compiler crash with internal errors in some cases when using loops
//if (profile >= GraphicsProfile.Level_10_0)
// shaderFlags |= D3DCOMPILE_SKIP_OPTIMIZATION;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xen2 Have you noticed some regression with these? I had my reserves on this change, but couldn't test it properly yet.

@Ethereal77
Copy link
Contributor Author

Nice! I have some more improvements and docs in progress. Will better create new PRs for those, as this was becoming BIG already.

@Kryptos-FR
Copy link
Member

Kryptos-FR commented Jan 7, 2026

@xen2 it looks like this PR wasn't merged correctly. Instead of doing the usual merge that either creates a merge commit or a squash merge that pushes a new commit on top of master, the master branch was reset on top of this PR's branch. This messes up then history completely because now, this branch is considered to be the main branch and the graph looks like this:
image
The green branch is now considered to be the main branch, and most tools will show it on the left, while the previous master branch (blue) is now represented as a remote branch. That's because during a merge there is the concept of left and right children. The left child is "ours" the local branch (in case of a merge it is master), while the right child is "theirs" the remote branch (in case of the PR the feature branch). But now it is reversed. This could make it harder to investigate issue in the future, especially if we continue doing that. For instance it affects tools such as git bisect.

Instead we should always create a squash commit (I explain why in other PR's comment) and also never to manual merge locally bu use the merge feature on GitHub.

I don't have the rights to force-push on master, so could you reset master to commit 1d45526, the merge this PR branch (commit b989a9c) to it and force push so that we get again a clean history. Then cherry-pick commits 0d02132 and 9a2f741.

@Eideren Eideren changed the title [WIP] Convert from SharpDX to Silk.NET. feat: Convert from SharpDX to Silk.NET Jan 7, 2026
@xen2
Copy link
Member

xen2 commented Jan 7, 2026

Apologies, I forgot left/right had a specific meaning/importance.
There is no need to force push, I simply merged this commit starting from a previous master commit, so everything should be good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Convert from SharpDX to Silk.NET

9 participants