FacticiusVir / SharpVk

C# Bindings for the Vulkan API & SPIR-V
MIT License
147 stars 18 forks source link

Potential Marshaling issue in ImageBlit #43

Closed SinnersSumit closed 6 years ago

SinnersSumit commented 6 years ago

I've believe I've found the source of the problem at https://github.com/FacticiusVir/SharpVk/blob/73ede8512f3f58c07524b054e09f3a6877995396/src/SharpVk/ImageBlit.gen.cs#L77

While writing an app I got this error;

[Warning]       vkCmdBlitImage: pRegions[0].srcOffsets specify a zero-volume area.

[Warning]       vkCmdBlitImage: pRegions[0].dstOffsets specify a zero-volume area.

[Error] vkCmdBlitImage: region [0], source image of type VK_IMAGE_TYPE_1D or VK_IMAGE_TYPE_2D with srcOffset[].z values of (0, 0). These must be (0, 1).
The spec valid usage text states 'If the calling command's srcImage is of type VK_IMAGE_TYPE_1D or VK_IMAGE_TYPE_2D, then srcOffset[0].z must be 0 and srcOffset[1].z must be 1.'
(https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkImageBlit-srcImage-00247)

[Error] vkCmdBlitImage: region [0], dest image of type VK_IMAGE_TYPE_1D or VK_IMAGE_TYPE_2D with dstOffset[].z values of (0, 0). These must be (0, 1).
The spec valid usage text states 'If the calling command's dstImage is of type VK_IMAGE_TYPE_1D or VK_IMAGE_TYPE_2D, then dstOffset[0].z must be 0 and dstOffset[1].z must be 1.'
(https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkImageBlit-dstImage-00252)

Using the debugger I have determined that this is not in fact true, as the values are {{0, 0, 0}, {401, 377, 1}} and {{0, 0, 0}, {401, 377, 1}} respectively. Perhaps I am misinterpreting things, but the code at https://github.com/FacticiusVir/SharpVk/blob/73ede8512f3f58c07524b054e09f3a6877995396/src/SharpVk/ImageBlit.gen.cs#L77 does not appear to marshal the srcOffset or dstOffset values.

SinnersSumit commented 6 years ago

I'm downloading the source so I can attempt to manually fix this right now. Will post progress.

SinnersSumit commented 6 years ago

I can confirm, the issue is the offsets are not being marshaled; I changed line 77 of ImageBlit.cs from https://github.com/FacticiusVir/SharpVk/blob/73ede8512f3f58c07524b054e09f3a6877995396/src/SharpVk/ImageBlit.gen.cs#L77

internal unsafe void MarshalTo(SharpVk.Interop.ImageBlit* pointer)
        {
            pointer->SourceSubresource = this.SourceSubresource;
            pointer->DestinationSubresource = this.DestinationSubresource;
        }

to

        internal unsafe void MarshalTo(SharpVk.Interop.ImageBlit* pointer)
        {
            pointer->SourceSubresource = this.SourceSubresource;
            pointer->DestinationSubresource = this.DestinationSubresource;
            pointer->DestinationOffsets_0 = DestinationOffsets[0];
            pointer->DestinationOffsets_1 = DestinationOffsets[1];
            pointer->SourceOffsets_0 = SourceOffsets[0];
            pointer->SourceOffsets_1 = SourceOffsets[1];
        }

My code works now, but unfortunately I have no idea how your generator works, so I can't fix what ever is going wrong in it.

FacticiusVir commented 6 years ago

Hey, thanks for spotting this; looks like the MarshalTo actions for fixed-length non-primitive (e.g. not int, float, etc) array fields was missing.

And it's fair about the generator, I need to sit down and write out some proper documentation for it :-D For issues like this (around the marshalling of fields or method parameters) the behaviour is typically under https://github.com/FacticiusVir/SharpVk/tree/master/src/SharpVk.Generator/Generation/Marshalling - in this case, under the FixedLengthMemberPattern.

FacticiusVir commented 6 years ago

I've added some checking on the marshal actions so it doesn't exception when no value is provided for those fields; but having thought about it, having an array at all seems like overkill for a fixed pair of values. It'd be much cleaner to expose this and similar fields as (maybe) a System.ValueType<Extent3D, Extend3D> so there's no heap allocations and default, empty values are provided implicitly; though a marshalling step is still necessary as you can't pass generic types to unmanaged functions. Thoughts?

SinnersSumit commented 6 years ago

ValueTuples are always good. I do agree though, I wish they were blitable. Hopefully we'll see something like that in the next C# version.

SinnersSumit commented 6 years ago

@FacticiusVir I believe I've just found a new error,

Unhandled Exception: System.MissingMethodException: Method not found: 'Void SharpVk.ImageBlit.set_SourceOffsets(System.ValueTuple`2<SharpVk.Offset3D,SharpVk.Offset3D>)'.
   at VulkanFormsTest.Form1.PresentImage()
   at VulkanFormsTest.Form1.<BeginRender>b__26_0() in D:\User\Documents\Programming\TestCode\VulkanFormsTest\VulkanFormsTest\Form1.cs:line 184
   at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()

No idea what is going on here, doesn't make a whole lot of sense, and I'm having trouble tracing it.

FacticiusVir commented 6 years ago

Looks like the DLL it's building against is not making it to the output directory; clean the project output and rebuild. You can also check the output DLL version with .NET Reflector.

At a guess, there might be some confusion in the build chain from your original fix, but it definitely sounds like it's running against an old DLL version.

SinnersSumit commented 6 years ago

Still happening. I even started a new project, and the problem persists. If it helps, nuget refuses to install sharpvk without explicitly installing system.memory first.

FacticiusVir commented 6 years ago

The System.Memory one is odd, I had the same issues when updating the SharpVk samples. Can you try building the Hello Triangle sample and see if that gives you the same exception?

Which framework are you building against?

SinnersSumit commented 6 years ago

HelloTriangle works as expected.

I was originally building against .net framework 4.7, but it looks line the cloned project was targeted to 4.6.1. After switching versions I started getting a "missing compilerservices.valuetupleattribute" error, but forcing everything to reinstall seems to have fixed that.

It's working again now, without the odd setter error, though something is definitely still going on with the nuget installation process.

FacticiusVir commented 6 years ago

Is it something about the targets? I'll need to run some experiments, but both SharpVk & System.Memory have .NETStandard2.0 and not .NETFramework4.7; it might be that NuGet fails to get a dependency for which there isn't an explicit target but can brute-force it if you explicitly install, and maybe (/maybe/) there's a DLL version mismatch on System.ValueTuple between targets?

Either way, building against .NET Framework 4.7.1 should work fine across the board, and I'll see what I can do to make the experience smoother for older versions.

FacticiusVir commented 6 years ago

Okay, there's a new patch up without the System.Memory dependency; it's not needed yet, and I can reintroduce it when Span is a little more mature. Oddly, .NET 4.7.1 projects with package.config get the NuGet error, but using packagereferences don't - maybe a NuGet-plugin issue?

FacticiusVir commented 6 years ago

Looks like the package version of System.Memory that SharpVk was built against has been removed, and only the latest package is indexed; so NuGet can fail dependency lookups but will install the package directly, and then accept the higher version for the dependency.Let me know if the new 0.4.2 package works okay for you, and I'll close off the issue.

SinnersSumit commented 6 years ago

I can confirm it is working now.