vsg::Path development

[Robert Osfield]

Hi All,

Next up on my TODO list for VulkanSceneGraph-1.0 is to implement a vsg::Path class that works in a similar way to std::filesystem::path, but without the older compiler support problems that this has.

My plan since 2018 when I first put in the "using Path = std::string;;" in include/vsg/io/FileSystem.h was that later I'd come back and implement a vsg::Path class that supports wchar_t filenames under Windows, with utf8 char filenames on unix variants - broadly similar to what std::filesystem::path does.

My first step has been to implement a vsg::Path class that "has a" std::string, implements all the std::string methods that the VSG, vsgXchange, vsgGIS and vsgExamples use.  This first step is intended to keep things compiling and working as before, and giving me an idea of the features that will be needed in the final incarnation of vsg::Path.

These first set of changes are now checked into Path branches:

    https://github.com/vsg-dev/VulkanSceneGraph/tree/Path

    https://github.com/vsg-dev/vsgXchange/tree/Path

    https://github.com/vsg-dev/vsgGIS

I may also need to update the other VSG libraries but the above are the changes I needed to make to get things to compile and run cleanly after changes to VSG.

I have also created a vsgpath example to use initially as testbed, but this should evolve into an example:

    https://github.com/vsg-dev/vsgExamples/tree/Path

The changes to vsgXchange and vsgGIS work with VSG master, mostly they are just fixes where std::string usage slipped into the code rather than using vsg::Path. 

In the new inlude/vsg/io/Path.h header I have a define:

#define NEW_PATH_DEFINED 1

That used to decide where to use the new vsg::Path class when the value is 1, and to use std::string for vsg::Path when it's 0.  This allows me to toggle between the old and new implementations for testing purposes, so if I get a regression I can more easily hunt the cause down.

Once vsg::Path is complete and debugged I'll remove this #define NEW_PATH_DEFINED and all the associated #if statements around the code.

--

It's end of the working day now so I won't do any further coding, tomorrow my plan is to look at std::wstring support and mapping between std::wstring and std::string with the latter assumed to use UTF8.

I don't have a Windows dev system to test against, and don't know anything about the Windows file system functions - the ones in the VSG I didn't write, so will need members of the VSG community who are familiar with Windows development to help guide and test the move across to fully supporting wchar_t variants of the file system calls.

My aim with this work is provide a relatively seamless way to handle filenames across OS's using a consistent API across platforms with platform specific differences minimized - or at least minimized with a modest amount of code on our side as I'm aiming to avoid the task spirally out to include lots of filesystem related functionality.

I don't know yet how exactly it'll look in the end, or how much of std::filesystem::path is appropriate to replicate or attempt to work like. I'll get more familiar with std::filesystem::path as this work progresses, and am keen to take suggestions from the community on what bits are really helpful and what parts you've never used.

The general ethos I'm going for is minimal and complete enough for basic cross platform filename handling.

Cheers,

Robert.

[Robert Osfield]

I have steadily been building out the new vsg::Path functionality, and updated vsgGIS, vsgXchange and vsgExamples to utilize the capabilities provided by vsg::Path.  Thanks to the cross platform automated builds on github I've been able to make sure the Win32 calls that I've changed compile on Windows, though as I don't have a Windows dev system have no idea if they run correctly :-)

While the work isn't completed yet I think there is enough for others to begin to try compiling and running that apps against the Path branches of VSG, vsgGIS, vsgXchange and vsgExamples.  I'm hoping that things will just work as they did before and we won't see any regressions.  Taking std::filesystem::path as inspiration, on POSIX systems (Linux, macOS, Android and iOS) vsg::Path uses std::string internally, while on Win32 std::wstring is used internally.   This means on the Windows front in theory you should now be able to set the vsg::Path using wchar_t* and wstring strings and have them passed through automatically to the i/ofstream constructors as vsg::Path provides a cast operator to wstring and string on Windows and POSIX systems respectively.

I also created a vsg::fopen(const Path& path, const char* mode) methods that uses the _wfopen_s method so hopefully this will allow you to use wstring filenames safely with both C++ and C IO.

I have also add automatic mapping between char*/std::string and wchat_t*/std::wstring so that you can use either when setting paths and calling various Path::find/replace/compare etc. so that making code portable between Windows and POSIX systems be a little easier. 

Where possible I have updated codes to support both std::wstring/wchar_t* and std::string/char* filenames, but not all 3rd party libraries support this, so for GDAL and freetype I've just head to convert to std::string and then pass the c_str(). 

To handle working with either filename variants I have written a number of copy functions for copying from and to different string combinations, mostly then are just repackaging but the vsg::copy(const std::wstring& src, std::string& dst) and vsg::copy(const std::string& src, std::wstring& dst) versions will convert from utf16 to utf8 and utf8 to utf16 respectively.   These two functions are just placeholders for now than just copy the characters across without handling the UTF* encodings, tomorrow I'll implement these.  These methods will be essentially for handling conversion between wide and narrow strings, and to enable support for wide strings encoded as utf8 when reading/writing to VSG native ascii and binary formats.

Once I've finished the UTF conversion code I'll review the API of vsg::Path as it kinda sits between the functionality provided by std::string and the what is provided by std::filesytem::path, it's ended up like this as originally vsg::Path was just a typedef to std::string so all code using vsg::Path basically treated it as a std::string.  Those codes could probably be refined by added some more std::filesystem::path style functionality into vsg::Path.  Happy to take suggestions on what you like/dislike about std::string and std::filesytem::path and what would be helpful to adopt or ignore.

While there is some loose ends I think the work is far enough along for others to testing out the build and runtime on their systems.  Windows users testing things out are especially sought as I can only test the build remotely using the github automated builds, so have no ability to test the runtime, and I also have no expertise on the Win32 file API calls that I've used - I've just done online searches and reading of MS docs and done what I "think" is probably roughly appropriate, but I could easily be off the mark.

Cheers,

Robert.

[Rainer Gericke]

Hi Robert,

I tested the new vsg::Path. It builds on the Mac, vsgpath prompts:

vsgpath 

c_string c

w_string w

MyPath(string) string 1

print(wchar_t) 0x16bb47518

On windows the build of VulkanSceneGraph is broken due to typos. I couldn't create a pull request, so I send the changed file to you.

After applying these changes I got this answer on windows:

vsgpath
c_string char
w_string wchar_t
MyPath(string) string 1
print(wchar_t) 000000A918AFF6B0

[Robert Osfield]

HI Rainer,

On Fri, 6 May 2022 at 08:35, Rainer Gericke <dr.ratz...@gmail.com> wrote:

I tested the new vsg::Path. It builds on the Mac, vsgpath prompts:

vsgpath 

c_string c

w_string w

MyPath(string) string 1

print(wchar_t) 0x16bb47518

On windows the build of VulkanSceneGraph is broken due to typos. I couldn't create a pull request, so I send the changed file to you.

After applying these changes I got this answer on windows:

vsgpath
c_string char
w_string wchar_t
MyPath(string) string 1
print(wchar_t) 000000A918AFF6B0

Thanks for the testing.  Interesting that the typenames are different.  The vsgpath example itself doesn't do anything interesting w.r.t the current vsg::Path, the code in vsgpath.cpp was a quick test of the casting required to automatically map the std::wstring and std::string to the appropriate method in i/ofsteam constructors.

I'll be working on the UTF8 <-> UTF16 mapping today so will need to test this, so fleshing out vsgpath will probably be an appropriate place to use as a test bed for this.

I've merged the build fix, thanks for that.

Cheers,

Robert.

[Robert Osfield]

Now that vsg::Path is further along I have removed all the #if def's that were sprinkled through the VSG codebase to copy with vsg::Path no longer being a std::string.  This cleans up the code substantially.  I have also moved all the string <-> wstring conversion code into a dedicated include/vsg/io/convert_utf.h and convert_utf.cpp, though I still have to write the decoding/encoding code for this.

I have also add some file path tests to vsgpath example in the Path branch with illustrating of how vsg::fileExtension(), vsg::simpleFilename(), vsg::filePath() and vsg::removeExtension() work.  I've put in tests for a range of different file path combinations and while the most common patterns are handled correctly a number of patterns aren't handled correctly by all these functions so I will need to rewrite these functions to address these problems. 

These problems will have existed prior to the vsg::Path work so it's not a regression, rather just having a new set of tests to hammer existing functions.  While it's good to catch these prior to 1.0 it does mean it'll take me a bit longer to wrap up the vsg::Path work.

[Robert Osfield]

On Fri, 6 May 2022 at 12:40, Robert Osfield <robert....@gmail.com> wrote:

These problems will have existed prior to the vsg::Path work so it's not a regression, rather just having a new set of tests to hammer existing functions.  While it's good to catch these prior to 1.0 it does mean it'll take me a bit longer to wrap up the vsg::Path work.

To resolve these problems I've introduced a new function to handle the problem trialing . .. \. \.. /. /.. endings:

    bool vsg::trailingRelativePath(const vsg::Path& path)

I've deployed this in the other path processing functions to catch these previously probematiccases.  These changes are now checked into VSG Path branch.

[Robert Osfield]

Hi All,

I have written a UTF-8 encoding and decoder so that we can now use st::wstring with vsg::Path and be able to read/write these safely with the .vsgt and .vsgb formats with the reading/writing automatically converting the wstrings to UTF0-8 and back.  These changes are now checked into the PATH branch of VulkanSceneGraph.

On POSIX systems vsg::Path defaults to use the std::string so this step won't be required, on Windows it defaults to vsg::Path with wstring support so will do this conversion automatically, so in theory writing files under Windows and then using under POSIX systems and visa-versa should work fine.  I only have Linux system so can't test this roundtrip, so I would appreciate users who have access to both types of system to test things out.

I have also added some std::wstring <-> std::string and vsg::Path tests to the vsgpath example in the Path branch of vsgExamples.

I haven't written a UTF-16 encoder yet, and I'm in two minds whether this is required right now. UTF-8 is the one that is critical to handle encoding file names strings across platforms.

I now completed the key parts of the vsg::Path functionality that I was aiming for.  There are a few additions and cleans ups I could to bring vsg::Path closer to std::filesystem::path like the / operator.  I'll do a code review tomorrow and add anything that looks obviously missing/useful to end user developers.

I would appreciate testing/feedback from the community as I've been mostly writing stuff based on what I think is required but without a Windows system I'm working a little blind, vsg::Path exists primarily to try and smooth over platform specific hacks required to provide full Windows filename support.

Cheers,

Robert.

[Robert Osfield]

I have checked vsg::Path::concat(), + and += operators that directly join two paths without adding a file separate, vsg::Path::append(), / and /= operators that join two paths with a native file separator between them.  This mirrors the mathods and conventions found in std::filesystem::path:

    https://github.com/vsg-dev/VulkanSceneGraph/commit/40e7deae1348d7e255c16e6262973c34ff2993ac

I have remove the previous vsg::concatPaths() function as this providing similar functionality to std::filesystem::path::append() rather than std::fileystem::path::concat() so I felt keeping it for backwards compatibility would just provide opportunities for confusion, forcing users to update their code and make a decision whether they want + and / concatenation seems sensible. 

This change also makes the resulting path handling code more expression i.e.

  

    auto complete_path = directory / filename + extension;

The / operator checks if the directory is empty and doesn't add a /, so you don't get /filename.extension, instead you just get filename.extension.

The changes to vsgXchange illustrate how the replacement of vsg::concatPaths() looks in action:

    https://github.com/vsg-dev/vsgXchange/commit/e3ed034cda5d1d00466dff1d67cdfc9e87b7533a

I have also added to the vsgpath example some file concatenation tests that illustrate usage:

    https://github.com/vsg-dev/vsgExamples/commit/3fdc5690d27f87f96da4d8590bcf32ce1d5e0f61

vsg::Path and std::filesystem::path are now broadly similar, but aren't identical.  I have pulled across the essential and most obviously useful features into vsg::Path, and left a few std::string style features.  Potentially I could add more methods found in std::filesystem::path, though these additions can be added later as we need them - the bulk of the vsg::Path functionality is now in place.

I will do some code reviews this afternoon and more testing, and would appreciate testing out on other platforms, especially Windows - does everything compile and run correctly?  

If this all goes well I'll merge the respective Path branches with master this afternoon.

Cheers,

Robert.

[Robert Osfield]

I have updated the VSG version to 0.3.1 to signify the new vsg::Path functionality, so you can use this in your cmake find_package call i,e,

     find_package(vsg 0.3.1 REQUIRED)

I have done my code review and am now focused on final testing of the Path branch functionality before I go ahead and merged with VSG, vsgGIS, vsgXchange and vsgExample. master.

[Robert Osfield]

Hi All,

When testing the conversion of an OSG paged database to VSG equivalent using vsgconv I found an issue with writing of vsg::Path to a binary file, and a bug in the OSG PagedLOD conversion code.  I've fixed both these issues, they require commits to VSG and vsgXchange respectively.

It;s now past the end of the working day here so I'll leave the VSG Path branches as is overnight so they can be tested by the community. If there are now issues found overnight I'll merge the Path branches with respective masters.

Thanks for testing,

Robert.

[Rainer Gericke]

Hi Robert,

I tested the actual vsgPath related changes on Mac and Windows 10 (VS2022) again. On Windows I got tons of build errors, fortunately with 2 changes (#include <sstream>) I was able to fix it.

The vsgpath example runs into an infinite loop. sizeof(wchar_t) == 2 on windows, but sizeof(wchar_t) == 4 on the Mac (guess same as Linux). I changed it, now the Mac and Windows example run. That are the good news.

Under Windows I wasn't able to build vsgXchange and I don't know what to do. Do you have an idea?

Best

Rainer

[Robert Osfield]

To make vsg::Path usage a little more expressive and streamlined I've added a Path bool operator so you can now replace:

   if (!filename.empty()) ..

with:

    if (filename) ....

The changes to the VSG are:

   https://github.com/vsg-dev/VulkanSceneGraph/commit/d68c80cbd8559bd186ab91a5646ef1bcb3413a53

To see the effect on code that uses paths see the changes to vsgXchange and vsgExamples:

    https://github.com/vsg-dev/vsgXchange/commit/a02c0136fb2331b9f6b82c0a3c7abb3a5e9e14b5

    https://github.com/vsg-dev/vsgExamples/commit/e4fabf574e1805ff63eaae3c47d73490ee075a86

Cheers,

Robert.

[Robert Osfield]

HI Rainer,

On Wed, 11 May 2022 at 08:36, Rainer Gericke <dr.ratz...@gmail.com> wrote:

I tested the actual vsgPath related changes on Mac and Windows 10 (VS2022) again. On Windows I got tons of build errors, fortunately with 2 changes (#include <sstream>) I was able to fix it.

Why is it needed in Data.h and Node.h, they don't have anything to do with vsg::Path.  The automated Windows build on githhub is building fine without any issues.

Could you post the compile errors?

The vsgpath example runs into an infinite loop. sizeof(wchar_t) == 2 on windows, but sizeof(wchar_t) == 4 on the Mac (guess same as Linux). I changed it, now the Mac and Windows example run. That are the good news.

The infinite loop with the vsgpath under Windows is a bit odd, suggests the cast between 100000 and wchar_t is an issue.  What is the max value of wchar_t under Windows? 

Perhaps just reducing the max loop value will resolve the problem.

 

Under Windows I wasn't able to build vsgXchange and I don't know what to do. Do you have an idea?

This line in assimp.cpp

   if (const auto ext = vsg::lowerCaseFileExtension(filename); importer.IsExtensionSupported(ext))

Is probably attempting to map vsg::Path to a std::string, but under Windows the default is std::wstring, so my guess is Assimp requires std::string.

Could you try adding a .string() to force a case of the wstring to utft std::string:

   if (const auto ext = vsg::lowerCaseFileExtension(filename).string(); importer.IsExtensionSupported(ext))

The assimp.cpp line:

    if (auto scene = importer.ReadFile(filenameToUse, flags); scene)

Is probably failing for the same reason.  Would could do the same cast trick, but perhaps a better approach would be for the assimp.cpp to open a ofstream and pass this to Assimp, or perhaps there is wstring support in Assimp.

I'll be away from my dev system for an hour, on my return I can look into these issues further.

Thanks for the testing,

Robert.

[Rainer Gericke]

Hi Robert,

I rolled back the changes to Data.h and Node.h and I got the appended build log.

[Robert Osfield]

Hi Rainer,

On Wed, 11 May 2022 at 09:33, Rainer Gericke <dr.ratz...@gmail.com> wrote:

I rolled back the changes to Data.h and Node.h and I got the appended build log.

Thanks.  The errors are pretty odd, complaining about errors in files that aren't directly related to Path.h, so I can only presume that Path.h is being included indirectly.

Even more odd, the github automated builds are reporting that VisualStudio 2017 looks to build the VSG's Path branch without any warnings or errors.

 

What led you to add the #include <sstream> to the Node.h and Data.h headers?  Did you try adding #include <sstream> to Path.h?

I'm now wondering whether the ostream and istream operators at the bottom of Path.h are requiring the sstream include.  I'll experiment with putting these operators along with the other stream operators into the include/vsg/io/stream.h.

Cheers,

Robert.

[Robert Osfield]

Hi Rainer,

On Wed, 11 May 2022 at 10:57, Robert Osfield <robert....@gmail.com> wrote:

I'm now wondering whether the ostream and istream operators at the bottom of Path.h are requiring the sstream include.  I'll experiment with putting these operators along with the other stream operators into the include/vsg/io/stream.h.

I have moved the vsg::Path stream operators to include/vsg/io/stream.h and checked these changes into the VSG and vsgXchange Path branches.

Could you please test the build out on your system?

Thanks,

Robert

 

 

[Robert Osfield]

Hi Rainer et. al,

I have reduced the wchar_t -> utf8 string test loop bounds to 32768 which should be safely within the bounds of wchar_t under Windows.  This fix is checked in vsgExamples Path branch.

I will now wait for feedback on build and runtime if that looks OK will go ahead and merge the respective Path branches with master.

Thanks for any testing you can do,

Robert.

[Rainer Gericke]

Hi Robert,

we have come ahead. vsg and vsgExamples build on Windows 10 now. The problem with the missing headers is fixed, I followed your hint with the .string() missing in vs2022. It also worked. The build of vsgExamples is broken now (see attachment).

Best

Rainer

[Robert Osfield]

HI Rainer,

On Wed, 11 May 2022 at 12:40, Rainer Gericke <dr.ratz...@gmail.com> wrote:

we have come ahead. vsg and vsgExamples build on Windows 10 now. The problem with the missing headers is fixed, I followed your hint with the .string() missing in vs2022. It also worked. The build of vsgExamples is broken now (see attachment).

I have checked in what I think are appropriate fixes - mostly just changing std::string for vsg::Path usage.  Fixes checked in to vsgExamples path.

I will now review Assimp loader to see what best there.

Thanks for your help with testing,

Robert.

 

[Robert Osfield]

HI Rainer et. al,

On Wed, 11 May 2022 at 13:00, Robert Osfield <robert....@gmail.com> wrote:

I will now review Assimp loader to see what best there.

I have changed the vsg::Options::extensionHint to be vsg::Path, and changed the vsgXchange::Assimp  loader to use a .string() cast when passing the extension to Assimp.  These changes are checked into VSG and vsgXchange Path branches respectively.

I'll now cross all my fingers and toes hoping that everything will now build cleanly under Windows.

Please test :-)

Thanks,

Robert.

[Robert Osfield]

As there has been a few Windows issues flagged today, which may now be fixed, but until we have confirmation that the build and runtime under Windows is work properly with the Path branches of VSG, vsgXchange and vsgExamples I can't go ahead with merging with the respective master.  I'll leave the mege till tomorrow to allow time for further testing.

[Rainer Gericke]

Hi Robert,

sorry for the dely, I wasn't at home. The last result of my testing on Windows is an error in vsgExchange. The changed file is attached.

Thanks

Rainer

[Robert Osfield]

Hi  Rainer,

On Thu, 12 May 2022 at 07:29, Rainer Gericke <dr.ratz...@gmail.com> wrote:

sorry for the dely, I wasn't at home. The last result of my testing on Windows is an error in vsgExchange. The changed file is attached.

No problem, thanks for doing the testing and providing a fix.

I was hoping that we might be able to pass a whar_t* filename to Assimp but it looks like we are limited to just char* filenames.  I will look into whether Assimp will handle the utf8 encoding that vsg::Path provides when using Path::string() one Windows.

Failing that perhaps it would be best to open a ifstream using vsg::Path and then reading the file into memory and then passing this to Assimp.

Cheers,

Robert.

 

[Robert Osfield]

On Thu, 12 May 2022 at 08:04, Robert Osfield <robert....@gmail.com> wrote:

I was hoping that we might be able to pass a whar_t* filename to Assimp but it looks like we are limited to just char* filenames.  I will look into whether Assimp will handle the utf8 encoding that vsg::Path provides when using Path::string() one Windows.

It looks like Assimp assumes filenames are UTF-8 under windows and does a conversion from UTF-8 to std::wstring, so it looks like we are safe to cast to std::string using vsg::Path::string().  I have merged this fix and pushed it to vsgXchange Path.

With this merged I now believe we are good to go for merging Path branches with their respective master :-)

[Rainer Gericke]

Good news: after pulling the last commits everything can be built and the examples run well.

Thanks

[Robert Osfield]

On Thu, 12 May 2022 at 08:40, Rainer Gericke <dr.ratz...@gmail.com> wrote:

Good news: after pulling the last commits everything can be built and the examples run well.

Phew, that's a great relief.  Thanks for your help with testing.

 

[Robert Osfield]

Hi All,

I have wrapped up the vsg::Path work and have merged the Path branches with respective masters.

Changes to VulkanSceneGraph:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/442

Changes to vsgGIS:

    https://github.com/vsg-dev/vsgGIS/pull/20

Changes to vsgXchange:

   https://github.com/vsg-dev/vsgXchange/pull/98

Changes to vsgExamples:

    https://github.com/vsg-dev/vsgExamples/pull/148

Thanks to those that helped with testing. 

Robert.