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.83k stars 641 forks source link

Vulkan .pNext #843

Closed tlf30 closed 1 year ago

tlf30 commented 1 year ago

Question

Hello, on VkPhysicalDeviceProperties2 and VkPhysicalDeviceFeatures2 I recently ran into a scenario where I was using the pNext to set a pointer, but because in the code both VkPhysicalDeviceProperties2 and VkPhysicalDeviceFeatures2 overwrite the pNext of the structure being passed into them if the structure is one of the overridden types, it did not work as initially intended. After digging in the code I found where this was happening, and changed my code to pass the address of the struct to the pNext.

Example:

physicalDeviceRayTracingPipelineProperties = VkPhysicalDeviceRayTracingPipelinePropertiesKHR.calloc()
        .sType$Default();
physicalDeviceAccelerationStructureProperties = VkPhysicalDeviceAccelerationStructurePropertiesKHR.calloc()
        .sType$Default()
        .pNext(physicalDeviceRayTracingPipelineProperties.address());
physicalDeviceIDProperties = VkPhysicalDeviceIDProperties.calloc()
        .sType$Default()
        .pNext(physicalDeviceRayTracingPipelineProperties.address());
physicalDeviceProperties2 = VkPhysicalDeviceProperties2.calloc()
        .sType$Default()
        .pNext(physicalDeviceIDProperties);

In this example, only the VkPhysicalDeviceIDProperties struct is actually passes to VkPhysicalDeviceProperties2 because in VkPhysicalDeviceProperties2 the function is overloaded as:

public VkPhysicalDeviceProperties2 pNext(VkPhysicalDeviceIDProperties value) { return this.pNext(value.pNext(this.pNext()).address()); }

Which overwrites the pNext of the VkPhysicalDeviceIDProperties struct.

IIRC the vulkan C++ API does not do this (although I could be wrong). Perhaps if this is additional functionality that is intended, the function could be renamed to pNext$Append

Thank you, Trevor

Spasi commented 1 year ago

Hey @tlf30,

The typed pNext methods are effectively "prepending" to the struct chain / linked-list. The behavior is intended, so that you can do this:

physicalDeviceRayTracingPipelineProperties = VkPhysicalDeviceRayTracingPipelinePropertiesKHR.calloc()
    .sType$Default();
physicalDeviceAccelerationStructureProperties = VkPhysicalDeviceAccelerationStructurePropertiesKHR.calloc()
    .sType$Default();
physicalDeviceIDProperties = VkPhysicalDeviceIDProperties.calloc()
    .sType$Default();
physicalDeviceProperties2 = VkPhysicalDeviceProperties2.calloc()
    .sType$Default()
    .pNext(physicalDeviceRayTracingPipelineProperties)
    .pNext(physicalDeviceAccelerationStructureProperties)
    .pNext(physicalDeviceIDProperties);

Note that the typed overloads only exist on the "parent" struct. The siblings do not (need to) know each other. If this is inconvenient, you should probably use the untyped/pointer pNext methods only, to avoid confusion.

tlf30 commented 1 year ago

Thank you for the clarification @Spasi. Since those overloads provide additional functionality, perhaps it would be best if there was something in the name to indicate that, similar to sType$Default. Perhaps pNext$Append or pNext$Prepend` just to avoid confusion later down the road for others. This particular error took quite a while to find as the structs were simply not being populated, and I was concerned there was additional things I needed to do for raytracing to be enabled.

Spasi commented 1 year ago

It's a bit late for breaking API changes and I'm afraid a new name won't be less confusing. I'm more inclined to remove such helpers in LWJGL 4. It seems that whenever LWJGL tries to be clever, there are always users that stumble on unexpected/non-intuitive behavior.

Spasi commented 1 year ago

Hmm, maybe LWJGL should check that the input struct's pNext is NULL. If not, throw an exception to let the user know they're probably misusing the API?

tlf30 commented 1 year ago

It's a bit late for breaking API changes and I'm afraid a new name won't be less confusing. I'm more inclined to remove such helpers in LWJGL 4. It seems that whenever LWJGL tries to be clever, there are always users that stumble on unexpected/non-intuitive behavior.

Unfortunately, it is a trade off. While having additional functionality is great, having it hidden also makes it hard to know if it is there. In v4 perhaps all additional functionality is denoted some way as to clue in the developer that there may be more going on behind the scenes to investigate.

Hmm, maybe LWJGL should check that the input struct's pNext is NULL. If not, throw an exception to let the user know they're probably misusing the API?

Would it be possible to do this: Check if the parent struct's pNext is not already null, if not null then run down the parent structs pNext chain until a null is found, then append the given pNext to it?

Basically, append what is given to what is already there?

Hmm, maybe LWJGL should check that the input struct's pNext is NULL. If not, throw an exception to let the user know they're probably misusing the API?

If nothing else, this would be great.

ws909 commented 1 year ago

I agree with @tlf30. One of the powers of LWJGL is that it tries to mirror the original API as much as possible, but it's also great that LWJGL does include helpers for some minor functionality. This particular helper functionality has surprised me as well. I suggest renaming it in LWJGL 4 like above, or turning them into static helper methods. I don't really see any reason in removing them, when renaming them is all that's necessary to remove confusion. In fact, I actually wrote a method to replicate that exact behaviour before I noticed what these methods do, so this functionality is needed.

Spasi commented 1 year ago

Hey @tlf30, @ws909,

Check if the parent struct's pNext is not already null, if not null then run down the parent structs pNext chain until a null is found, then append the given pNext to it?

Just implemented this as an experiment. Chain order does not matter, so I figured this would be a compatible change and... it isn't. This example from lwjgl3-demos:

VkPhysicalDeviceAccelerationStructureFeaturesKHR accelerationStructureFeatures = VkPhysicalDeviceAccelerationStructureFeaturesKHR
        .malloc(stack)
        .sType$Default();
VkPhysicalDeviceRayTracingPipelineFeaturesKHR rayTracingPipelineFeatures = VkPhysicalDeviceRayTracingPipelineFeaturesKHR
        .malloc(stack)
        .sType$Default();
VkPhysicalDeviceBufferDeviceAddressFeaturesKHR bufferDeviceAddressFeatures = VkPhysicalDeviceBufferDeviceAddressFeaturesKHR
        .malloc(stack)
        .sType$Default();
vkGetPhysicalDeviceFeatures2(dev, VkPhysicalDeviceFeatures2
        .calloc(stack)
        .sType$Default()
        .pNext(bufferDeviceAddressFeatures)
        .pNext(rayTracingPipelineFeatures)
        .pNext(accelerationStructureFeatures));

crashes with the new implementation. Turns out if we change prepending to appending all structs must initialize their pNext member to NULL. Whereas with prepending, only the parent struct needs to have NULL before the first pNext is called. Btw, another point in favor of prepending is that it's an O(1) operation, whereas appending isn't (though, admittedly, the number of elements in the chain will never be that high).

ws909 commented 1 year ago

@Spasi I believe you should just leave this functionality as-is without further modifications, until LWJGL 4.

rongcuid commented 1 year ago

The identifiers with $ is actually quite awkward to use in Kotlin, because it requires backquote-escaping. Intellij IDEA's autocompletion actually breaks when I try to use .sType$Default. Adding another would probably make things even harder.

ws909 commented 1 year ago

@rongcuid That’s an issue you should report to JetBrains, not LWJGL. IntelliSense/IntelliJ should be able to handle that.

rongcuid commented 1 year ago

@rongcuid That’s an issue you should report to JetBrains, not LWJGL. IntelliSense/IntelliJ should be able to handle that.

Well, the autocomplete problem surely belongs to JetBrains. But the dollar sign still makes it awkward to use in Kotlin:

VkSomeStruct.`sType$Default`()
tlf30 commented 1 year ago

@Spasi I think there is no action to take on this issue unless you would like to implemented the pointer chaining that you proposed. Eitherway, would you like me to close it?

Spasi commented 1 year ago

Hey @tlf30,

Yes, closing this. I realized that my suggestion above about adding a NULL check will also break existing code (for the same reason as the "append" experiment, existing code just mallocs). So, nothing we can do here.

The good news is that the existing javadoc already documents the prepend behavior. Everyone reads javadoc, right? :sweat_smile:

The identifiers with $ is actually quite awkward to use in Kotlin, because it requires backquote-escaping.

My recommendation would be to just not use these methods. It's very likely that I won't implement them in LWJGL 4, might as well be ready for that.

ws909 commented 1 year ago

@Spasi

The good news is that the existing javadoc already documents the prepend behavior. Everyone reads javadoc, right?

Haha, no, I don’t think we read all the JavaDoc. :P We expect all the methods to not do anything special, as long as they’re named the same as the VK struct members, which are documented at the class level JavaDoc. Occassionally we read the JavaDoc for the member functions (struct fields) too, but we don’t expect side-effects there.

My recommendation would be to just not use these methods. It's very likely that I won't implement them in LWJGL 4, might as well be ready for that.

You forgot that’s also the letter used when the native struct has a member named the same as an existing member of the generated Java class, or one of its superclasses. There’s a lot of them in the FT structs.