Skip to content

Add new tests for advanced data uploading#474

Merged
adam-sawicki-a merged 2 commits intoGPUOpen-LibrariesAndSDKs:masterfrom
IAmNotHanni:master
Apr 1, 2025
Merged

Add new tests for advanced data uploading#474
adam-sawicki-a merged 2 commits intoGPUOpen-LibrariesAndSDKs:masterfrom
IAmNotHanni:master

Conversation

@IAmNotHanni
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/Tests.cpp Outdated
Comment thread src/Tests.cpp Outdated
Comment thread src/Tests.cpp Outdated
Comment thread src/Tests.cpp Outdated
Comment thread src/Tests.cpp
Comment thread src/Tests.cpp Outdated
@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
Copy Markdown
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
Copy Markdown
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