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

[Idea] Pre-fill sType() for Vulkan structs #537

Closed
octylFractal opened this issue Mar 1, 2020 · 12 comments
Closed

[Idea] Pre-fill sType() for Vulkan structs #537

octylFractal opened this issue Mar 1, 2020 · 12 comments

Comments

@octylFractal
Copy link
Contributor

Environment

  • LWJGL version: 3.2.3
  • LWJGL build #: 13
  • Java version: 13
  • Platform: Linux
  • Module: vulkan

Description

It would be nice if the Vulkan struct classes could have some option to pre-fill or set sType automatically, as it's literally based on the type of the struct, rather than needing to type it out. Even just being able to do something like VkPipelineShaderStageCreateInfo.callocStack().sType(VkPipelineShaderStageCreateInfo.S_TYPE) would be nicer than having to import the class and a constant.

I do understand that this takes it away from looking like the C code, but arguably omitting the setters for the length properties already does that, and this would be a similar convenience feature.

@SWinxy
Copy link
Contributor

SWinxy commented Sep 22, 2020

AFAIK @Spasi uses this to generate the Vulkan bindings. I'm not sure if that would be able to generate an auto-filled sType.

@Spasi
Copy link
Member

Spasi commented Sep 17, 2021

I'm working on this issue and could use some feedback.

Currently trying to find an alternative to the VkPipelineShaderStageCreateInfo.callocStack().sType(VkPipelineShaderStageCreateInfo.S_TYPE) suggestion, because:

  • There is already an STYPE static field in Vulkan struct classes, it represents the byte offset of the sType member.
  • It is too much typing/autocompleting.
  • It looks ugly.

It would be better to simply overload the sType setter, however a no-param sType() setter method would conflict with the getter method. So, unless someone has a better idea, the new setter must include a suffix. For example:

/** Sets the specified value to the {@link #sType} field. */
public VkBufferMemoryBarrier sType(@NativeType("VkStructureType") int value) { nsType(address(), value); return this; }
/** Sets the default value to the {@link #sType} field. */
public VkBufferMemoryBarrier sType$Default() { return sType(VK10.VK_STRUCTURE_TYPE_BUFFER_MEMORY_BARRIER); }

and the call site:

VkBufferMemoryBarrier.malloc(stack)
    .sType$Default()
    .pNext(NULL)
    // more members here
;

Some ideas for the suffix:

sTypeDefault()
sType$Default()
sTypeAuto()
sType$Auto()

Other than aesthetics, the problem with an arbitrary suffix is that the new name may end up matching exactly another struct member. It won't happen in the case of Vulkan structs, but this is going to be a generic feature of the LWJGL code generator and a naming scheme that minimizes that risk would be welcome.

@Spasi
Copy link
Member

Spasi commented Sep 17, 2021

I should also mention that there's another, source-incompatible option. The sType getter could be removed and the sType setter changed to a no-arg method that sets the default value. It would look like this:

VkBufferMemoryBarrier.malloc(stack)
    .sType() // sets default and returns the VkBufferMemoryBarrier instance, not the value of sType
    .pNext(NULL)
    // more members here
;

This would be the cleanest option and the static unsafe accessors (nsType) would still be available as a workaround for unusual cases.

Note that the next LWJGL 3 release is a major one (3.3.0) and breaking source compatibility is an option we do have.

@octylFractal
Copy link
Contributor Author

I'm not sure if this fits with the generic nature of the feature, but I think sType$Inferred is another viable candidate. If that doesn't work, then my preference is sType$Default. Both are unlikely to clash because of the $, which I understand is a problematic character for an identifier in C/C++.

I don't think removing the getter is a good idea, it will be very weird.

I also thought of an esoteric suggestion:

// Singleton instance
public enum Default { DEFAULT }

public class VkBufferMemoryBarrier [...] {
    // [...]
    /** Sets the default value to the {@link #sType} field. */
    public VkBufferMemoryBarrier sType(Default setDefault) { return sType(VK10.VK_STRUCTURE_TYPE_BUFFER_MEMORY_BARRIER); }
    // [...]
}

// Usage
VkBufferMemoryBarrier.malloc(stack)
    .sType(DEFAULT) // sets default and returns the VkBufferMemoryBarrier instance
    .pNext(NULL)
    // more members here
;

This is source compatible, nice to read, and easy to use. It does require another import [which I mentioned as a downside in the original issue], but that import is shared for all structs, so it is still much reduced.

@httpdigest
Copy link
Member

I do kinda like the special and typesafe enum overload. Would allow us to also add more enum literals (other than DEFAULT) in the future, should the need arise. And it indeed does read nicer than any suffix of the method name.

@SWinxy
Copy link
Contributor

SWinxy commented Sep 17, 2021

I envisioned the idea being to fill the structure type on allocation (ie during calloc(MemoryStack)) with the .sType(int) overriding the default and for backwards compatibility.

@lwjglgamedev
Copy link

I also like the SWinxy appoach (if possible).

@Spasi
Copy link
Member

Spasi commented Sep 19, 2021

Automatically filling in sType on malloc/calloc was the first option I considered. It's the obvious and simplest choice. However, it's implicit behavior and I'm trying to avoid that in LWJGL as much as possible. The recent deprecation of the auto-TL-lookup mallocStack()/callocStack() methods is a recent example of bad API that had to be fixed. There's no way to understand from the API that sType is populated automatically, users would have to read the javadoc to know that it's happening and we all know that users don't read documentation. There are also performance implications, especially in the case of struct arrays. It's a lot of cache-lines to be touching 2 times (or 3 in the case of calloc(n): first memory is zeroed-out, then LWJGL sets sType for all structs in the array, then the user sets the other members).

I initially liked the enum Default idea but, after playing with it, the extra static import felt awkward. It's extra ceremony that shouldn't be needed and I couldn't decide on a good place to put the enum class (in the prototype it was inside the Struct class, so you had to import static org.lwjgl.system.Struct.Default.*;). Also, it felt like an alien concept, I've never encountered an API doing something like that in Java.

So, an extra suffixed method may be the least bad solution. It's explicit and easily discoverable, has dedicated javadoc, does not affect performance and no extra import is required. It currently looks like this:

sType$Default

I'm still open to naming suggestions or other clever ideas though.

@Spasi Spasi closed this as completed in 639b142 Sep 25, 2021
@SWinxy
Copy link
Contributor

SWinxy commented Sep 26, 2021

.sType$Default() it is, then. I didn't know that was a valid character to put in methods.

@tlf30
Copy link
Contributor

tlf30 commented Oct 3, 2021

I guess I am jumping into this conversation a bit late. I too was not aware that a dollar sign could be in a method name (old dogs new tricks).

As for sType$Default() I have to say that im not too fond of it yet. I agree that it would be great to not specify the sType explicitly as it is inherent to the struct. I personally would like just a noarg sType call better to set the default. Just my two cents.

@Spasi
Copy link
Member

Spasi commented Oct 11, 2021

The default sType setters are now available in 3.3.0 snapshot 15. Sorry for the delay. Please give it a try and I'll be happy to hear any feedback, still open to better naming suggestions.

@Spasi
Copy link
Member

Spasi commented Oct 19, 2021

If you're evaluating this change, please upgrade to 3.3.0 snapshot 16 to also test the new type-safe pNext setters.

Auto-complete example:

pNext_example

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

6 participants