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

Adding documentation to the allocated class hierarchy, base classes edition #1

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

jherico
Copy link

@jherico jherico commented Nov 1, 2024

Here's a first pass at adding more developer comments to the allocated base classes. I haven't done the derived classes yet, but I wanted to get a sense of whether this was what was being looked for.

Prompted by KhronosGroup#1193

@jherico
Copy link
Author

jherico commented Nov 1, 2024

Tagging @SaschaWillems

@SaschaWillems
Copy link

SaschaWillems commented Nov 2, 2024

Thanks a lot for this. Makes it a lot easier to follow and understand the code. Very much appreciated 👍🏻

Copy link
Owner

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great documentation!

Just a couple of typos.

framework/builder_base.h Outdated Show resolved Hide resolved
framework/builder_base.h Outdated Show resolved Hide resolved
framework/builder_base.h Outdated Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
*/
void destroy_image(ImageType image);
/**
* @brief Clears the internal state. Can be overridden by derived classes to perform additional cleanup of members.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be overridden? It's not marked as virtual.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a need for this to be overriden.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so either. So please remove that comment.

framework/core/allocated.h Outdated Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
Copy link
Owner

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I like the fact that the AllocatedBase class is gone.

Just a couple of typos and maybe some whitespace adjustments.

protected:
/**
* @brief The VMA-specific constructor for new objects. This should only be visible to derived classes.
* @tparam Args Additional constructor arguments needed for the derived class. Typically a `VkImageCreateInfo` or `VkBufferCreateInfo` struct.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: @tparam -> @param

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an error or typo. @tparam is specifically for template parameters.

framework/core/allocated.h Outdated Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
framework/core/buffer.h Show resolved Hide resolved
framework/core/allocated.h Outdated Show resolved Hide resolved
@jherico
Copy link
Author

jherico commented Nov 10, 2024

let me know if there's anything else you want in terms of documentation. I could try to add some notes and links to the VMA docs at the appropriate places for the builder classes, but I'd rather do that in a distinct PR. if you're happy with these changes so far let me know and I'll rebase the "fixup" commits into a single commit so you can merge it without the mess. I only kept it as fixups so you could see the incremental changes for each one.

Copy link
Owner

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it's perfect now! Thanks a lot.

@asuessenbach asuessenbach merged commit c768e60 into asuessenbach:unify_Allocated Nov 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants