Skip to content

Add new tests for advanced data uploading #474

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

Merged
merged 2 commits into from
Apr 1, 2025

Conversation

IAmNotHanni
Copy link
Contributor

@IAmNotHanni IAmNotHanni commented Mar 29, 2025

Closes #433

What do you think? The new tests don't show any validation layer warnigns or errors on my system.
I still need to find a way to link the code in the doxygen documentation, but that's for another pr.

I still have some questions:

  1. In TestDataUploadingWithStagingBuffer, I explicitely check if the memory did not end up in mappable memory with TEST(!(memPropFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT));. Does this make sense like this? Because if this is not true, it would be a bug in vma, right? (The same argument is for TEST(memPropFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT); in TestDataUploadingWithMappedMemory).

  2. Besides vmaFlushAllocation, should we maybe also mention in a comment that the user needs to call vmaInvalidateAllocation if reading from gpu -> cpu is the desired use case?

  3. In theory, we could abstract TestAdvancedDataUploading into a function (as I did with the lambda in the ticket here) so we have less duplicate code. The functions TestDataUploadingWithMappedMemory and TestDataUploadingWithStagingBuffer would then call that function with the flags required. While this leads to less code, I am not sure if it would be easier for beginners to understand.

  4. Also, I think it would be worth mentioning in the docs that both vmaFlushAllocation and vmaInvalidateAllocation check internally if the memory is VK_MEMORY_PROPERTY_HOST_COHERENT_BIT, meaning we don't need to check for manually this when calling these functions

  5. Do we need some additional tests if the data uploading was successful?

Johannes

Copy link
Contributor

@adam-sawicki-a adam-sawicki-a left a comment

Choose a reason for hiding this comment

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

Thank you for adding these tests. I'm happy to merge when the issues I posted as comments are fixed.

@adam-sawicki-a adam-sawicki-a added input needed Waiting for more information quality Code quality improvement e.g. refactoring labels Apr 1, 2025
@IAmNotHanni
Copy link
Contributor Author

IAmNotHanni commented Apr 1, 2025

This pull request was tested successfully on NVidia RTX 3090, Intel Arc A770, and AMD Ryzen™ 9 7950X.
The following issues are still present: #339 and #422 I will continue to look into this.

@adam-sawicki-a
Copy link
Contributor

Thank you for this contribution!

@adam-sawicki-a adam-sawicki-a added next release To be done as soon as possible and removed input needed Waiting for more information labels Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release To be done as soon as possible quality Code quality improvement e.g. refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a new test which covers 'advanced data uploading' from docs
2 participants