Vulkan-Samples icon indicating copy to clipboard operation
Vulkan-Samples copied to clipboard

Fix folder creation on Android

Open jloehr opened this issue 1 year ago • 3 comments
trafficstars

Description

root doesn't has a trailing slash, so on Android a folder named filesoutput/ was created instead of files/output/. This PR adds the missing directory separator when creating folders.

General Checklist:

Please ensure the following points are checked:

  • [x] My code follows the coding style
  • [x] I have reviewed file licenses
  • [ ] I have commented any added functions (in line with Doxygen)
  • [ ] I have commented any code that could be hard to understand
  • [x] My changes do not add any new compiler warnings
  • [x] My changes do not add any new validation layer errors or warnings
  • [ ] I have used existing framework/helper functions where possible
  • [x] My changes do not add any regressions
  • [ ] I have tested every sample to ensure everything runs correctly
  • [x] This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • [x] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • [x] My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

  • [ ] I did a full batch run using the batch command line argument to make sure all samples still work properly

jloehr avatar Sep 20 '24 08:09 jloehr

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 20 '24 08:09 CLAassistant

Under what circumstances do you get root without a trailing slash? Maybe this function should be able handle both cases, root with and without a trailing slash.

asuessenbach avatar Sep 23 '24 09:09 asuessenbach

Under Android fs->external_storage_directory().string() (which is passed into create_path as root) returns something along /data/data/com.khronos.vulkan_samples/files (don't have the exact path at hand now) without the trailing space.

https://github.com/KhronosGroup/Vulkan-Samples/blob/4c911f774c4069cafa215d61bdb9a9a2dfa18b22/components/filesystem/src/legacy.cpp#L74 seems to be the only place where create_path is called. We ran samples with that change on Android, Windows and Linux and didn't encounter any issues.

Maybe we can replace it with std::filesystem::create_directories altogether. Though haven't tested that. We ran with the simple added slash.

jloehr avatar Sep 23 '24 16:09 jloehr

Hi @tomadamatkinson, can you take a quick look here? Looks reasonable, but wanted to double check with you before merging. Thanks

marty-johnson59 avatar Nov 04 '24 18:11 marty-johnson59

Hey @jloehr, this approach likely works, but I was a little unsure if this would affect other platforms.

Maybe we can replace it with std::filesystem::create_directories altogether. Though haven't tested that. We ran with the simple added slash.

I think this is a great suggestion! It also pointed out some bugs that we could have had with vkb::fs in the long run. I also didn't know how the standard library would behave so i added some tests in here too. Please take a look at #1214 as an alternative approach

That said i am happy with either PR to merge :)

tomadamatkinson avatar Nov 04 '24 19:11 tomadamatkinson

Merging w/ Tom's approval - per discussion on Monday's call - LMK if this causes issues and we can revert

marty-johnson59 avatar Nov 06 '24 16:11 marty-johnson59