LWJGL / lwjgl3

LWJGL is a Java library that enables cross-platform access to popular native APIs useful in the development of graphics (OpenGL, Vulkan, bgfx), audio (OpenAL, Opus), parallel computing (OpenCL, CUDA) and XR (OpenVR, LibOVR, OpenXR) applications.
https://www.lwjgl.org
BSD 3-Clause "New" or "Revised" License
4.67k stars 628 forks source link

VkSubmitInfo.npWaitSemaphores() does not set waitSemaphoreCount. #849

Closed theagentd closed 1 year ago

theagentd commented 1 year ago

Version

3.3.2 (nightly)

Platform

Linux x64, Linux arm64, Linux arm32, macOS x64, macOS arm64, Windows x64, Windows x86, Windows arm64

JDK

Zulu 17

Module

Vulkan

Bug description

While testing a simple Vulkan program, I ran into issues with the following code:

                VkSubmitInfo info = VkSubmitInfo.calloc(stack).sType$Default();
                info.pWaitSemaphores(stack.longs(acquireSemaphore));
                info.pCommandBuffers(stack.pointers(commandBuffer));
                info.pSignalSemaphores(stack.longs(presentSemaphore));

I received a validation warning saying that acquireSemaphore (passed into pWaitSemaphores()) was never consumed. This is caused by pWaitSemaphores() not setting waitSemaphoreCount to 1 automatically. The following code runs as expected:

                VkSubmitInfo info = VkSubmitInfo.calloc(stack).sType$Default();
                info.waitSemaphoreCount(1); //needed, but shouldn't be!
                info.pWaitSemaphores(stack.longs(acquireSemaphore));
                info.pCommandBuffers(stack.pointers(commandBuffer));
                info.pSignalSemaphores(stack.longs(presentSemaphore));

Looking in the source code of VkSubmitInfo, we can see that the buffer versions of npCommandBuffers() and npSignalSemaphores() correctly set counts as well, but npWaitSemaphores() does not.

public static void npWaitSemaphores(long struct, @Nullable LongBuffer value) { memPutAddress(struct + VkSubmitInfo.PWAITSEMAPHORES, memAddressSafe(value)); /*nwaitSemaphoreCount() MISSING HERE*/ } 

public static void npCommandBuffers(long struct, @Nullable PointerBuffer value) { memPutAddress(struct + VkSubmitInfo.PCOMMANDBUFFERS, memAddressSafe(value)); ncommandBufferCount(struct, value == null ? 0 : value.remaining()); }

public static void npSignalSemaphores(long struct, @Nullable LongBuffer value) { memPutAddress(struct + VkSubmitInfo.PSIGNALSEMAPHORES, memAddressSafe(value)); nsignalSemaphoreCount(struct, value == null ? 0 : value.remaining()); }

Stacktrace or crash log output

No response

knokko commented 1 year ago

In most cases, there is 1 pointer associated with each 'size' field. However, waitSemaphoreCount is special because it is associated to both pWaitDstStageMask (which you seem to be forgetting) and pWaitSemaphores.

I'm not really sure what the desired behavior would be: should both setters update waitSemaphoreCount? Should only 1 of them do it? (And which one?) Or should we keep the current behavior?

theagentd commented 1 year ago

Aha, that's my mistake for missing pWaitDstStageMask. I'm a bit surprised that both the validation layer and the driver was OK with the pWaitDstStageMask pointer being null...

Yeah, it makes sense when there are two pointers that both have to have the same length that it's ambiguous which one should set the length. I would vote for both of them to update the count, since they have to match anyway.

TheMrMilchmann commented 1 year ago

(This does pop up quite often. See also #539 and https://github.com/LWJGL/lwjgl3/issues/829#issuecomment-1336428874.)

Spasi commented 1 year ago

I'll put this up for discussion when work on LWJGL 4 starts, but right now I'm thinking that autosized parameters & struct members should not exist in 4.

Unlike NIO buffers, MemorySegment in Panama doesn't have position & limit attributes. LWJGL introduced autosized bindings because the NIO API promoted such implicit attributes. It was a bad idea because important information is hidden, it's confusing, get it wrong and you crash. The JDK is even worse with buffer-consuming methods that bump the current position, even experienced developers struggle with it. At least LWJGL doesn't do that.

So, if with Panama "offset" & "length" attributes are tracked explicitly outside MemorySegment, I believe the LWJGL bindings should expose the native API as is and such attributes should be passed explicitly by the user.

tlf30 commented 1 year ago

So, if with Panama "offset" & "length" attributes are tracked explicitly outside MemorySegment, I believe the LWJGL bindings should expose the native API as is and such attributes should be passed explicitly by the user.

I agree. The JDK's limit and position to this day cause me headache even after several years of working with them. One little slip up can be incredibly difficult to find.

theagentd commented 1 year ago

Hey, Spasi! It's been a while! :) And yep, I agree with everything you said!