Improve documentation for ash::Device and ash::Instance #592

hoj-senna · 2022-02-28T16:28:55Z

Closes #576.

MarijnS95

Thanks for contributing this PR to self-resolve your reported issue, but I'm not really fond of the way the text is written.

For starters, render the docs yourself and make sure all intradocs-links are correct and consistent (don't repeat link text, backticks go in the link name, more [currently monospaced] words can be converted to links) and then we can commence the review from there.

Perhaps it's easier if I respin this PR instead of requesting a change on every line, let me know what you prefer 😉

hoj-senna · 2022-03-17T17:27:09Z

Thank you!

I have attempted to improve the links.

In some places I have not turned monospaced words into links, because I don't think that, for example, the text explaining ash::Device should have (self-referential) links to ash::Device.

If respinning the PR is easier/faster for you, please go ahead with that.

MarijnS95 · 2022-03-22T11:06:43Z

Seems like we're getting somewhere.

In some places I have not turned monospaced words into links, because I don't think that, for example, the text explaining ash::Device should have (self-referential) links to ash::Device.

I'd rather have links here like [`Device`][Self] than even mentioning the ash:: module prefix since you're already reading the documentation inside that module (in other words, this is an unnecessary module qualification), or rewrite the docs to refer to "this" / "the current type being documented" instead of spelling out its name.

MarijnS95 · 2022-03-22T11:18:31Z

ash/src/device.rs

+/// It is not identical to Vulkan's device object (see [vk::Device]).
+/// The latter, however, can be accessed via the [`handle()`][Self::handle()] method.
+/// In this sense, `ash::Device` is a Vulkan device handle together with all its associated function pointers.


Instead of saying what it's not, how about saying what it is? (and don't forget backicks inside [] links).

Suggested change

/// It is not identical to Vulkan's device object (see [vk::Device]).

/// The latter, however, can be accessed via the [`handle()`][Self::handle()] method.

/// In this sense, `ash::Device` is a Vulkan device handle together with all its associated function pointers.

/// Holds a Vulkan [`vk::Device`] handle together with Vulkan 1.0 - 1.3 device function pointers loaded from it, providing convenient wrappers to call these functions.

///

/// The internal handle is available through [`handle()`][Self::handle()] if needed externally.

MarijnS95 · 2022-03-22T11:21:41Z

ash/src/device.rs

@@ -6,7 +6,19 @@ use std::mem;
 use std::os::raw::c_void;
 use std::ptr;

-/// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkDevice.html>
+/// This is a dispatchable object, whose main purpose is to provide access to Vulkan's functions.


I don't really like the "whose main purpose is" bit here. The fact that it provides access to Vulkan functions is already described in the next paragraph, and what's the other "non main" purpose you're hinting at here?

Apparrently, there is no other purpose (which I did not know before your question).

MarijnS95 · 2022-03-22T11:25:36Z

ash/src/device.rs

+/// All functions from the Vulkan API (except those from extensions) which have a [`VkDevice`] as their first argument, have become methods of `ash::Device`.
+/// Their [`VkDevice`] argument is always passed implicitly.


This is not entirely correct. First argument can also be a derivative from the current device (ie. associated with one unique device) such as a CommandBuffer, PhysicalDevice, Queue etc. I'd argue just having the paragraph above with the device function pointers loaded from it mention is enough, unless you feel like linking to the Vulkan object model docs and mentioning this device relationship is necessary.

Also not sure if referencing VkDevice from the Vulkan docs is very helpful, pointing to the Rust version here is more type-correct (only reference Vulkan docs if there's something actually useful to be read there, and be explicit about it providing additional information).

And in any case, since this first argument is passed implicitly internally I don't think we should bother the user with that detail - but if you feel like this being necessary, expand on that in the providing convenient wrappers to call these functions sentence.

This is not entirely correct. First argument can also be a derivative from the current device

I'd say that does not contradict the sentence which only talks about the functions with VkDevice as first argument. (It does make my description incomplete; and I have removed this sentence.)

About "bothering the user": You can see this as an internal implementation detail of ash — or as one of the most striking differences between ash and the Vulkan docs to which we often refer, which could (should?) be made explicit somewhere.

MarijnS95 · 2022-03-22T11:31:00Z

ash/src/device.rs

+/// All functions from the Vulkan API (except those from extensions) which have a [`VkDevice`] as their first argument, have become methods of `ash::Device`.
+/// Their [`VkDevice`] argument is always passed implicitly.
+///
+/// Also, all `vkCmd*` functions are accessed as methods of `ash::Device`.


They are device functions, no need to mention them explicitly? Otherwise you should also make an exception for all other functions not taking Device but a "derivative" of Device as first argument.

MarijnS95 · 2022-03-22T11:31:26Z

ash/src/instance.rs

+/// Their [`VkInstance`] argument is always passed implicitly.
+///
+/// [`VkInstance`]: https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkInstance.html
+


Suggested change

MarijnS95 · 2022-03-22T11:31:38Z

ash/src/instance.rs

+/// `ash::Instance` contains a [vk::Instance] (accessible via [`handle()`][Self::handle()]) and all associated function pointers.
+///
+/// All functions from the Vulkan API (except those from extensions) which have a [`VkInstance`] as their first argument have become methods of `ash::Instance`.
+/// Their [`VkInstance`] argument is always passed implicitly.
+///
+/// [`VkInstance`]: https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkInstance.html


Same comments here as above.

hoj-senna · 2022-04-04T19:57:33Z

I'd rather have links here like [`Device`][Self] than even mentioning the ash:: module prefix since you're already reading the documentation inside that module (in other words, this is an unnecessary module qualification)

My perspective here was that I wanted to provide a comparison (or "translation guide") between the Vulkan API docs (and Vulkan tutorials etc.) and ash. Since most of ash's documentation (rightly) consists in links to the Vulkan specification and ash::Device is one of the few "additional" things, it seemed to make sense. And in this context, it also seemed reasonable to spell out where I'm talking about ash::Device and where about the VkDevice of the (C) Vulkan API. So, I did not find the module qualification unnecessary.

In this sense, I also disagree with (the second part of) this:

Also not sure if referencing VkDevice from the Vulkan docs is very helpful, pointing to the Rust version here is more type-correct

The objects I was talking about (or at least: wanted to be talking about) where those from the Vulkan docs, not their Rusty ash counterparts.
But the links are not really helpful, that's true.

I have now changed the documentation of Device according to your suggestion (although I have changed "device function pointers" to "device-level function pointers", because I did not find "device function" by searching in the Vulkan docs, and there is at least an entry "Device-Level Command" in the Glossary);

I have chosen a version that expands on "providing convenient wrappers to call these functions" in the documentation of Instance. (The point of "First argument can also be a derivative" etc. shouldn't apply here.)

hoj-senna added 3 commits February 28, 2022 17:12

Documentation for ash::Device

282ee01

Document ash::Instance

d7d865c

typo

ddbae54

MarijnS95 requested changes Mar 17, 2022

View reviewed changes

hoj-senna added 2 commits March 17, 2022 18:23

improve links

f109b52

improve links

5ad7240

MarijnS95 reviewed Mar 22, 2022

View reviewed changes

hoj-senna added 3 commits April 4, 2022 21:29

Merge branch 'MaikKlein:master' into master

429c3ed

rewrite documentation of ash::Device

10bb499

expanded documentation

3b6e305

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation for ash::Device and ash::Instance #592

Improve documentation for ash::Device and ash::Instance #592

hoj-senna commented Feb 28, 2022

MarijnS95 left a comment

hoj-senna commented Mar 17, 2022

MarijnS95 commented Mar 22, 2022

MarijnS95 Mar 22, 2022

MarijnS95 Mar 22, 2022

hoj-senna Apr 4, 2022

MarijnS95 Mar 22, 2022

MarijnS95 Mar 22, 2022

hoj-senna Apr 4, 2022 •

edited

Loading

MarijnS95 Mar 22, 2022

MarijnS95 Mar 22, 2022

MarijnS95 Mar 22, 2022

hoj-senna commented Apr 4, 2022

		/// All functions from the Vulkan API (except those from extensions) which have a [`VkDevice`] as their first argument, have become methods of `ash::Device`.
		/// Their [`VkDevice`] argument is always passed implicitly.

Improve documentation for ash::Device and ash::Instance #592

Are you sure you want to change the base?

Improve documentation for ash::Device and ash::Instance #592

Conversation

hoj-senna commented Feb 28, 2022

MarijnS95 left a comment

Choose a reason for hiding this comment

hoj-senna commented Mar 17, 2022

MarijnS95 commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoj-senna Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoj-senna commented Apr 4, 2022

hoj-senna Apr 4, 2022 •

edited

Loading