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

apply functions added in 3.2.3 break existing Kotlin code #554

Closed
Hugobros3 opened this issue May 2, 2020 · 7 comments
Closed

apply functions added in 3.2.3 break existing Kotlin code #554

Hugobros3 opened this issue May 2, 2020 · 7 comments

Comments

@Hugobros3
Copy link

Release 3.2.3 brought a new apply method in StructBuffer and thus clashes with the built-in apply extension function available in Kotlin code. That apply function appears to do the same job but has a different calling convention from the Kotlin one (Kotlin apply takes a lambda with this renamed to the object it's called on, while StructBuffer's takes a SAM ).

The result of this is large portions of Kotlin code that use StructBuffers are now broken (example here) and need tedious manual working around the issue. I have to say it's a very unfortunate oversight to have implemented any functionality that clashes with basic Kotlin stdlib functionality, especially given LWJGL itself uses Kotlin for various things internally.

I'm not sure how to fix this, I admit it's been a while since I worked on my Kotlin game engine project. I'd say roll back the changes, but this already shipped so it's a matter of choosing whose code you'll break now. The middle ground might just be to rename the function ?

@Hugobros3 Hugobros3 changed the title apply functions added in 3.2.3 breaks tons of existing Vulkan Kotlin code apply functions added in 3.2.3 break existing Vulkan Kotlin code May 2, 2020
@Hugobros3 Hugobros3 changed the title apply functions added in 3.2.3 break existing Vulkan Kotlin code apply functions added in 3.2.3 break existing Kotlin code May 2, 2020
@Hugobros3
Copy link
Author

Hugobros3 commented May 2, 2020

(I don't know about tons, it's just mine afaik. But apply is a staple of idiomatic Kt code)

@octylFractal
Copy link
Contributor

I don't think this does the same job -- here apply works on a relative element of the buffer (and increments the position!), whereas Kotlin's apply would be targeting the buffer itself. Your code only worked because the buffer also has methods to target the currently pointed-to element of the buffer, not because Kotlin's apply was extracting the element from the buffer.

I do think that it may be slightly misleading for them to have the same name and similar signatures, but do very different things.

@XenoAmess
Copy link

So can it build with the latest version of kotlin?
Or just compile error?

@Hugobros3
Copy link
Author

Your question is weirdly phrased. This has nothing to do with changes on the Kotlin side, the latest version of LWJGL just breaks my code in plenty of places.

I do very liberal amounts of stackAlloc(1) to get singleton array of structs to pass into Vulkan in plenty of places where the API expects an array, since LWJGL doesn't offer overloads for the non-buffer variants. Using Kotlin's apply there just makes sense and yields the most readable code, which is why I had settled on that pattern.

@XenoAmess
Copy link

XenoAmess commented May 2, 2020

Your question is weirdly phrased. This has nothing to do with changes on the Kotlin side, the latest version of LWJGL just breaks my code in plenty of places.

I do very liberal amounts of stackAlloc(1) to get singleton array of structs to pass into Vulkan in plenty of places where the API expects an array, since LWJGL doesn't offer overloads for the non-buffer variants. Using Kotlin's apply there just makes sense and yields the most readable code, which is why I had settled on that pattern.

Oh. I see.
This is why I hate kotlin.
Such clash seems will never happen if using java instead.

@Spasi
Copy link
Member

Spasi commented Mar 16, 2021

Hey @Hugobros3, thanks for opening this issue. Adding these methods to StructBuffer was meant to make it easier for Java code to consume the Vulkan API, but it indeed had this unfortunate side-effect when working in Kotlin. I would probably take it back (or made different) if this was reported earlier, but I'm afraid it's too late now.

However, I would like to point out that there's a ton of flexibility in Kotlin and there are trivial workarounds that don't sacrifice readability. As an example, I refactored some of the code in the posted screenshot and this is how I would have written it:

val colorAttachmentReference = VkAttachmentReference.callocStack(outputsDeclaration.outputs.size)
colorAttachmentReference.forEachIndexed { index, ref -> ref
    .attachment(index)
    .layout(VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL)
}

val subpassDescription = VkSubpassDescription.callocStack(1)
    .pipelineBindPoint(VK_PIPELINE_BIND_POINT_GRAPHICS)
    .pColorAttachments(colorAttachmentReference)
    .colorAttachmentCount(colorAttachmentReference.capacity())
    .pDepthStencilAttachment(if (!depth.enabled) null else VkAttachmentReference.callocStack()
        .attachment(attachmentDescription.capacity() - 1)
        .layout(
            if (depth.write)
                VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL
            else
                VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL
        )
    )

This is shorter and I would argue cleaner than the original code.

@Spasi Spasi closed this as completed Mar 16, 2021
@Hugobros3
Copy link
Author

Well it was reported a year ago, it looks like the issue slipped under the cracks. It doesn't really matter too much, as you say, it's too late now, and personally I have moved in other directions.

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