KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.81k stars 468 forks source link

Suggestion to replace the actual value into 'sType is the type of this structure'. #1921

Open eloj opened 2 years ago

eloj commented 2 years ago

I'm copying this one from twitter because I think this seems like a worthwhile idea.

"Document pages for structs do not tell the correct sType immediately. You have to scroll down to the valid usage section, which is often several pages down and takes time to find. Would be better to tell the correct sType immediately." -- https://twitter.com/SebAaltonen/status/1562022771428888576

aa-sType

sjfricke commented 2 years ago

I also would be :100: % in favor of this too now thinking about it!

@oddhack I put together the following quick

diff --git a/chapters/fxvertex.txt b/chapters/fxvertex.txt
index aa6ab32b..0efae67c 100644
--- a/chapters/fxvertex.txt
+++ b/chapters/fxvertex.txt
@@ -284,9 +284,6 @@ The sname:VkPipelineVertexInputStateCreateInfo structure is defined as:

 include::{generated}/api/structs/VkPipelineVertexInputStateCreateInfo.txt[]

-  * pname:sType is the type of this structure.
-  * pname:pNext is `NULL` or a pointer to a structure extending this
-    structure.
   * pname:flags is reserved for future use.
   * pname:vertexBindingDescriptionCount is the number of vertex binding
     descriptions provided in pname:pVertexBindingDescriptions.
diff --git a/scripts/docgenerator.py b/scripts/docgenerator.py
index 741f01f3..1ae30188 100644
--- a/scripts/docgenerator.py
+++ b/scripts/docgenerator.py
@@ -228,7 +228,7 @@ class DocOutputGenerator(OutputGenerator):
             # No API dictionary available, return nothing
             return ''

-    def writeInclude(self, directory, basename, contents):
+    def writeInclude(self, directory, basename, contents, afterContent = ''):
         """Generate an include file.

         - directory - subdirectory to put file in
@@ -262,6 +262,7 @@ class DocOutputGenerator(OutputGenerator):
         write('----', file=fp)
         write(contents, file=fp)
         write('----', file=fp)
+        write(afterContent, file=fp)
         fp.close()

         if self.genOpts.secondaryInclude:
@@ -277,6 +278,7 @@ class DocOutputGenerator(OutputGenerator):
             write('----', file=fp)
             write(contents, file=fp)
             write('----', file=fp)
+            write(afterContent, file=fp)
             fp.close()

     def writeEnumTable(self, basename, values):
@@ -391,6 +393,7 @@ class DocOutputGenerator(OutputGenerator):
         OutputGenerator.genStruct(self, typeinfo, typeName, alias)

         body = self.genRequirements(typeName)
+        afterContent = ''
         if alias:
             if self.conventions.duplicate_aliased_structs:
                 # TODO maybe move this outside the conditional? This would be a visual change.
@@ -401,8 +404,14 @@ class DocOutputGenerator(OutputGenerator):
             body += 'typedef ' + alias + ' ' + typeName + ';\n'
         else:
             body += self.genStructBody(typeinfo, typeName)
-
-        self.writeInclude('structs', typeName, body)
+            for member in typeinfo.elem.findall('.//member'):
+                memberName = member.find('name')
+                # checking member.get('values') will catch cases such as VkBaseInStructure
+                if (memberName is not None and memberName.text == 'sType' and member.get('values') != None):
+                    afterContent = '\n'
+                    afterContent +='  * pname:sType is the type of this structure, ename:{}.\n'.format(member.get('values'))
+                    afterContent +='  * pname:pNext is `NULL` or a pointer to an extension-specific structure.'
+        self.writeInclude('structs', typeName, body, afterContent)

     def genEnumTable(self, groupinfo, groupName):
         """Generate tables of enumerant values and short descriptions from

and now it looks like

image

Honestly, I don't see why not just generate the sType and pNext descriptions as they are all copied-and-pasted to my knowledge anyway

(Note this would require someone going and removing the sType/pNext everywhere now, happy to do it if you and the WG agree with this change)

oddhack commented 2 years ago

I don't think that will work in general. When a structure is promoted, you get two includes of the two different structure fragments, so the result would be like

...defined as

<structure declaration one>

sType / pNext boilerplate

or the equivalent

<structure declaration two>

duplicate sType / pNext boilerplate

If we do decide to do this, we could add it as a comment before the sType member declaration in the fragment, instead.

oddhack commented 2 years ago

As a general comment on this sort of thing, we try very hard not to duplicate information. The structure API gets described first, then all the semantics around the API. In this specific case there's no risk of divergence since the VU and the hypothetical autogenerated sType comment are both generated, but we've found that whenever you have two pieces of language describing the same thing, and one of them is hand-written, they are very likely to eventually diverge. Whether this specific case is justified is a decision for the working group to make, we will bring it up next week.

sjfricke commented 2 years ago

but we've found that whenever you have two pieces of language describing the same thing, and one of them is hand-written, they are very likely to eventually diverge

I agree with this also... maybe taking a step back and looking at the core issue, getting the correct sType is slightly annoying, but I personally think the way we do it in the Validation Layers is one of the best way to handle it where you just go

    auto vertex_input_state = LvlInitStruct<VkPipelineVertexInputStateCreateInfo>();

and in your code (where it actually matters) you don't need to go looking it up in the spec in the first place.

<thinking_out_loud> Maybe the real issue is people are not easily able to make use of the python library to generate the code, I realize there is no "Khronos stamped examples" how to take the XML generation python files and do things like generating your own sType mapping file... curious if we had a either included files like vk_typemap_helper.cpp in the Vulkan Headers or at least showed how to do it somewhere like the Vulkan Guide if that would solve issues like this where it is less of "documentation" and more of "I need to look up generated things in the spec instead of trying to hack around with a generation script" </thinking_out_loud>

oddhack commented 2 years ago

We have had some discussions about how to use the XML to generate header information that can be consumed by IDEs, which have more or less ground to a halt with a combination of lack of engagement from the VAP members, and lack of understanding of what the different IDEs can make use of. Whether feeding a required sType value through to the IDEs is possible, I have no idea, though I suspect at best there's no single method of doing that for the different IDEs commonly in use.

Edit: what's the real use case - is this for someone writing everything by hand? Do they want to be able generate a template for filling a structure inside their IDE? Write a library for initializing structures, like your example? Something else? I take the tweet author's point that it's friction to have to look up that value, but equally isn't it friction to look up what structure types are allowed in the pNext chain, or what the value of a flags field which is 'reserved' must be?

oddhack commented 2 years ago

I have been surprised by the number of Vulkan language binding projects which felt that it was a good idea to write yet another XML parser. Maybe that's mostly because they want to write their binding generator in the same language?

eloj commented 2 years ago

I don't really have a horse in this race, nor care that deeply about this, but here's what I'll say...

What separates sType from the other struct members mentioned is that you need this value in EVERY CASE and there is only ever 1 valid value. The same is not strictly true for pNext or flags; it is well understood that these will often be NULL and 0.

I can't speak for Mr. Aaltonen, but it seems pretty clear that the use-case is someone copying from the documentation into their code, and filling out structs 'manually'.

Often when I'm experimenting with something, I just want to do things in the simplest most familiar and manual way possible, and not get bogged down in tooling etc.

For this reason, I personally don't really care to be told off with "productivity" arguments that hinge on me taking a completely different approach to writing code. ''Why don't you just install this random scripting language and bindings generator and change your editor and use this 3rd party editor plugin and configure new shortcuts and learn all those things instead?''... Please. Stop.

I think trying to 'see through' this request to find some bigger underlying issue is to massively miss the point.

"Sometimes, the elegant implementation is just a function. Not a method. Not a class. Not a framework. Just a function." -- John Carmack.

oddhack commented 2 years ago

We discussed in the WG and agreed to put the sType enum in a comment after the sType member declaration in the struct definition. I will work up the generator code for that.