Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulkan .pNext #843

Closed
tlf30 opened this issue Jan 5, 2023 · 14 comments
Closed

Vulkan .pNext #843

tlf30 opened this issue Jan 5, 2023 · 14 comments

Comments

@tlf30
Copy link
Contributor

tlf30 commented Jan 5, 2023

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
Copy link
Member

Spasi commented Jan 8, 2023

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
Copy link
Contributor Author

tlf30 commented Jan 9, 2023

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
Copy link
Member

Spasi commented Jan 9, 2023

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
Copy link
Member

Spasi commented Jan 9, 2023

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
Copy link
Contributor Author

tlf30 commented Jan 10, 2023

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
Copy link

ws909 commented Jan 10, 2023

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
Copy link
Member

Spasi commented Jan 10, 2023

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
Copy link

ws909 commented Jan 10, 2023

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

@rongcuid
Copy link

rongcuid commented Jan 10, 2023

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
Copy link

ws909 commented Jan 10, 2023

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

@rongcuid
Copy link

@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
Copy link
Contributor Author

tlf30 commented Jan 25, 2023

@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
Copy link
Member

Spasi commented Jan 31, 2023

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? 😅

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.

@Spasi Spasi closed this as completed Jan 31, 2023
@ws909
Copy link

ws909 commented Jan 31, 2023

@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.

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

No branches or pull requests

4 participants