Fix missing Utilities.CopyMemory for other graphics APIs

[Ethereal77]

PR Details

When building the runtime, the other graphics APIs that are not Direct3D 11 (OpenGL, Vulkan, Direct3D 12) results in errors stating that Utilities.CopyMemory() and BinarySerialization.Read() don't exist.

A previous PR replaced several many interop and IL weaved methods with the use of refs, Span<T>, and Unsafe calls, but several of those for the other graphics APIs were not replaced.

This PR completes the replacement and fixes those errors when building Stride.Runtime.sln.

Description

• Replaces several uses of Utilities.CopyMemory() with Unsafe.CopyBlockUnaligned().
• Replaces several uses of BinarySerialization.Read() with Unsafe.Read().
• Adds a small section in Stride.props where users can set the design-time graphics API to override the default one with so the proper #defines are set, Visual Studio can know what packages to restore, and IntelliSense can show the correct completions. This section is commented by default, but it's useful when working or modifying a specific graphics API.
• Small cleanups and use of modern C# syntax.

Related Issue

This PR continues the work of ericwj's PR #1469.
The original issue that motivated that PR was #1461.

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.

TODO

• Document in stride-docs the way to set a specific graphics API at design-time.

[Jklawreszuk]

Em...Is it a duplicate of my PR #1690 ?

[Ethereal77]

From a quick overview... yes, it seems so.
Sorry about that. Searched GitHub but didn't noticed this PR, or maybe the name confused me. Also asked in Discord, but no one mentioned this.

[Jklawreszuk]

No worries 👌, I'll name my PR more precisely next time.

[Eideren]

There's a couple of nice changes, but a ton of unnecessary stylistic changes in there, please refrain from auto formatting files - think of us poor maintainer, we don't want to dig in looking for the actual changes to logic between all of those stylistic ones.

[Eideren]

@Jklawreszuk does @Ethereal77's PR cover yours in its entirety ?

[Jklawreszuk]

@Eideren After quick look into @Ethereal77 PR and the only difference I can see his PR has some refactoring. Plus I've also replaced Utilities.ClearMemory method with dotnet alternative, because that also causes compilation errors

[Eideren]

Okay, thanks @Jklawreszuk . Could you take care of this @Ethereal77 ? If not that's fine too, I'll do it after I merge yours in.

[Ethereal77]

Sorry about stylistic changes. Force of habit and also auto-formatting enabled :(

[Ethereal77]

@Eideren Of course, but... What specifically are you asking me to do?
😁

[Eideren]

Looking at Jklawreszuk's PR I'm pretty sure he's talking about this stuff:
https://github.com/stride3d/stride/pull/1690/files#diff-284c3b71f0fb7853170f2e88cbbc04c001e67c025c6a8a88dba4122a9acadf8f

[Ethereal77]

Ok, I'll check that out and also I'll search all Stride for uses of CopyMemory, ClearMemory, etc. to assess not a single one escapes, even if commented out.

[Eideren]

@Ethereal77 is this PR ready for merge ?

[Eideren]

While it does build, vulkan is broken on my end. Launching a game with Vulkan as the graphics API spawns a blank fully white window and closes after a second or so.

An unhandled exception of type 'System.ExecutionEngineException' occurred in Unknown Module.

There's no callstack or anything else in the console.

Vulkan does build and run properly on the main branch if we add back the Utilities.CopyMemory and BinarySerialization we removed in #1654

[Ethereal77]

Ok. I'll check that out. You mean launching a game with <StrideGraphicsApi>Vulkan</StrideGraphicsApi>, right?

[Eideren]

Yeah, after building the engine with msbuild /t:Build /p:StrideGraphicsApiDependentBuildAll=true Stride.sln of course

[Ethereal77]

I've compiled Stride as you say for all the supported graphics APIs. Then I've created a new simple game (as DX runs fine), I've set it to use Vulkan, and executed it.
It does as you say: a white screen for a second, then closes with the following exception:

Unhandled exception. System.AggregateException: One or more errors occurred. (One or more errors occurred. (An error occurred trying to start process 'win-x64\glslangValidator.exe' with working directory 'D:\Stride Projects\MyGame2\MyGame2.Windows'. El sistema no puede encontrar el archivo especificado.))

It can't find the file glslangValidator.exe.

[Eideren]

@Ethereal77 add <PackageReference Include="Stride.Shaders.Compiler" Version="4.1.0.1" /> to your csproj, haven't looked into why we need to add that now but it should fix this issue

[Ethereal77]

@Eideren Can you check if it says the same for you? Just run the project from the command line to see what exception (if any) it prints

[Ethereal77]

I see what you mean. Now it starts compiling the fragment and vertex shaders, and later exits just saying:

Fatal error. Internal CLR error. (0x80131506)

[Eideren]

I think your best bet to debug this is to use the previous Utilities.CopyMemory and BinarySerialization we removed in #1654 and add your changes one by one for the vulkan side, feel free to go about this however you want to of course

[Ethereal77]

This btw brings a question. I haven't checked but, is there a way in place to run the graphics tests for all supported graphics APIs?
Because this should have been detected in the tests I ran, but maybe they ran only for D3D11.

[Eideren]

Yeah but not by default, here's how TeamCity's Vulkan test is set up:


As text:

Runner type: .NET (Provides .NET toolchain support for .NET projects)
Execute: 
If all previous steps finished successfully
Command: msbuild
Paths: build\Stride.build
Targets: RunTestsWindows
Command line parameters: /nr:false /p:StrideTestCategories=%TestCategories% /p:StrideGraphicsApis="%StrideGraphicsApis%" /p:StrideGraphicsApisTest="%StrideGraphicsApisTest%" 

And

StrideGraphicsApis	Direct3D11;Vulkan
StrideGraphicsApisTest	Vulkan
TestCategories	Game

[Ethereal77]

Ok. I've tracked it down to a stupid mistake I made when reading the shader bytecode 🤦.
I assumed incorrectly that it was a blob-like structure like in DX, but it is a Stride custom type with references to dictionaries and other things that needs custom serialization logic, so my Unsafe.Read was corrupting the memory and not deserializing those types.

I'll make another commit with the fix.

By the way, I think instead of removing BinarySerialization that all it did was calling BinarySerializationReader and BinarySerializationWriter we should have tried to modify it so its convenience methods could operate on Span<byte> directly without going first to MemoryStream.
Before that change, it was only BinarySerialization.Read<T>(...), and after it is new BinarySerializationReader(new MemoryStream(...)).Read<T>(). Quite ugly and we are still allocating the two things.

[Eideren]

Nice !
I'm not too sure I follow, BinarySerializationReader does have a signature for Span<byte> so we can use that one instead ... ?

[Ethereal77]

Yeah, but that overload needs anyway that you construct a new BinarySerializationReader specifying the underlying Stream. So if you only needed to serialize / deserialize to memory you would need a new MemoryStream to encapsulate the data, then a new reader to encapsulate the stream, both of single use if you just wanted to read a single thing, like in the case of this last commit for example.

[Eideren]

Right, we don't have a static method that just takes in a Span<T> or byte[] and spits out an arbitrary T. Issue is that this is not any random T, it contains complex types - deserializing those isn't straightforward.
Looking at the implementation of the Read<T> method you're using there does show that it should be possible to remove most allocations but I can't see it happening without changing a lot of the serialization logic. You can work on that if you want but it's a big one to deal with.

stride/sources/core/Stride.Core/Serialization/SerializerExtensions.cs

Lines 92 to 100 in 6d20024

	public static void Serialize<T>([NotNull] this SerializationStream stream, ref T obj, ArchiveMode mode)
	{
	var dataSerializer = stream.Context.SerializerSelector.GetSerializer<T>();
	if (dataSerializer == null)
	throw new InvalidOperationException($"Could not find serializer for type {typeof(T)}.");
	dataSerializer.PreSerialize(ref obj, mode, stream);
	dataSerializer.Serialize(ref obj, mode, stream);
	}

Though this stuff is used very occasionally during a game session so it is unlikely to have any significant impact when playing.

[Ethereal77]

Oh, I know. Maybe a thing to look at a later time.
Anyway, this PR is ready if Vulkan works. I have tested with a new game and it runs ok for me.

[Eideren]

Yep, works great now, thanks @Ethereal77 !