ofToDataPath returning std::filesystem::path

[roymacdonald]

So,
on windows I get an error when trying to automatically convert from std::filesystem::path to string

and I have found it being an issue with ofxCv

@artificiel @ofTheo @dimitre

Although it compiles fine on macos.

[artificiel]

this happens because of an seemingly complicated (according to stack overflow etc) to navigate number of parameters relating to the setup of the IDE project options, encoding, manifest, BOM, filesystem, etc, the implicit conversion done by ofToDataPath's argument (which takes a path) will upgrade to wstring on Windows, even if it's explicitly passed a narrow std::string. (this is yet another side quest (testing) but it seems the OF tests in GitHub are setup in a way that does not trigger this problem, and/or the above test should be added).

so it's trying to convert wstring to a string in order to assign and not finding that converter.

don't have windows here; can you splice this in ofFileUtils.h at 1229 (just after ofToDataPath):

inline of::filesystem::path ofToDataPath2(std::string str, bool absolute = false) {
	return ofToDataPath(of::filesystem::path(std::string(str)), absolute);
}

and try your failure with ofToDataPath2? if that works we can work around by explicitly pushing std::string into filesystem::path. (and a potential "future" OF app that wants to be unicode-windows-friendly will anyway have to carefully use std::wstring or ::path and fall into the non-converted signature).

[NB the last phrase is another argument to "raise awareness of path vs string": if we expect some mac-developed app to compile in windows and support wide unicode, it means std::string is never used as a path container, even though on Mac it does not make an actual difference]

[so to be clear, std::string should never be used on windows for a path containers, but obviously legacy-compatibility requires a solution]

[roymacdonald]

@artificiel
I tested the snippet of code you posted and it does not work. same error.
the problem is implicitly converting from of::filesystem::path to std::string, not the other way around. Actually the problem seems to be that there is no implicit conversion from std::wstring to std::string.

I think that at least in the case of ofToDataPath and any class where the return type of a function which used to be std::string and got changed to of::filesystem::path we should roll back to std::string until we find a better solution as this will break on windows a lot of things. As I said elsewhere, we rollback the master branch and we make a dev branch where we have this changes and we test those.

[artificiel]

@roymacdonald as I mentioned elsewhere as far as management of /master goes i am not going to argue against. I responded to this issue as I was under the impression the above had been tested a while ago within the OF tests. it seems either that basic case (which is obviously required) is not part of the tests, or the tests are setup in such a way that the MSVC behaviour is different — it must be ensured that the case is effectively tested and should have failed the PR. I don't know enough about the setup of these GitHub automated testing tasks to contribute effectively on the topic.

[roymacdonald]

@artificiel I am not sure either, but it should be tested and as it is such a basic thing. and I can tell from a project in which I am working now, which I had to move from macos to windows, that it is actually incredibly annoying to deal with. I will take a closer look at the tests being done. @ofTheo any idea where/which are those tests?

[artificiel]

@roymacdonald the tests are here: https://github.com/openframeworks/openFrameworks/blob/master/tests/utils/fileUtils/src/main.cpp

(where i can't help is how are configured the gtihub CI automated builds, as vs2022-* passes all tests currently)

[danoli3]

Sounds like just need to fix ofxCv

…

On Mon, 14 Oct 2024 at 7:06 am, alexandre burton ***@***.***> wrote: @roymacdonald <https://github.com/roymacdonald> the tests are here: https://github.com/openframeworks/openFrameworks/blob/master/tests/utils/fileUtils/src/main.cpp (where i can't help is how are configured the gtihub CI automated builds, as vs2022-* passes all tests currently) — Reply to this email directly, view it on GitHub <#8143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGK2HALUQWEPXIRJAW2SFLZ3LHCTAVCNFSM6AAAAABPZTWDTOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBZGEYDONBRGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[artificiel]

@danoli3 ofxCv is an important one but there is plenty of code that contains code like std::string s = ofToDataPath(""), or object.open(ofToDataPath("")) where .open() expects a std::string. that will never compile on MSVC as the internal representation of datapath is always wide even if the path does not require it — as far as i can tell, which includes perusing the source of MS's implementation:
https://github.com/microsoft/STL/blob/926d458f82ae1711d4e92c0341f541a520ef6198/stl/inc/filesystem#L633-L635
(i say "never" -- there are numerous complaints around that behaviour that hinders the cross-platform design of std::filesystem::path (as we encounter), but let's not count on that).

perhaps a compatibility mode with a flag such as #if defined(DATAPATH_AS_STRING) && defined(OF_OS_WINDOWS) which would force .string() before returning the path? all functions taking a ::path should be happy with a std::string even on windows. the drawbacks are immaterial to the use case which would be existing code that anyway does not support wide unicode on windows.

[ofTheo]

Edit: nevermind.
Going to look at other solutions...

Below might still be feasible:

This only addresses ofToDataPath - if we wanted to address all functions with return types of of::filesystem::path we would maybe need a custom datatype that could have = operator overloads.

ie: ofFilePath ofToDataPath( of::filesystem::path, bool makeAbsolute );

[ofTheo]

I agree in terms of shorterm fixes this should probably be reverted until we can get a workable solution for Windows.
This would literally break almost every app on Windows at this point.

[roymacdonald]

Edit: nevermind. Going to look at other solutions...

Below might still be feasible:

This only addresses ofToDataPath - if we wanted to address all functions with return types of of::filesystem::path we would maybe need a custom datatype that could have = operator overloads.

ie: ofFilePath ofToDataPath( of::filesystem::path, bool makeAbsolute );

I was thinking about something around the same idea. Since we already are aliasing std::filesystem::path into of::filesystem::path we could just use a preprocessor define so only in windows it returns this new datatype with overloaded = operator, otherwise it is just the alias.

[roymacdonald]

I agree in terms of shorterm fixes this should probably be reverted until we can get a workable solution for Windows. This would literally break almost every app on Windows at this point.

I am currently developing on windows so I can do this changes, test and make a PR

[roymacdonald]

So, I was checking the git commit history and there are a bunch of commits which made the switch to using of::filesystem::path instead of std::string.

What would be the best strategy?
Options:

• reverting those commits:
  • Pros:
    • Easy.
  • Cons:
    * Might mess up things added after those commits.
• making the changes by hand and making a new commit
  • Pros:
    • Should not interfere with what we currently have
  • Cons:
    * more work required.

what do you think @ofTheo @artificiel @danoli3

[ofTheo]

@roymacdonald I think there was a point where the current ofToDataPath with named ofToDataPathFS and then ofToDataPath was:

//--------------------------------------------------
std::string ofToDataPath(const fs::path & path, bool makeAbsolute){
	return ofPathToString(ofToDataPathFS(path, makeAbsolute));
}

I think if we can PR back to that change even as a new commit but with just the rename of ofToDataPath to ofToDataPathFS and the above function - that might be all that is needed.

@dimitre would probably be the best to ask though as he's been working the most on this.

[artificiel]

yes at some point there was 2 versions of each of all these FS-suffxied paths methods and as i understand the process, they propagated and got progressively combined as tests were passing.

changing ofToDataPath to return std::string on windows (or maybe more precisely MSVC) should solve most problems for now.

but, it reallly should be ensured the VS builds are failing (upon std::string a = ofToDataPath("a");) — as of now the tests are not failing which in itself is a failure...

[dimitre]

I can't opinion on reverting because I'm still favoring the idea of moving to fs::path and ofPathToString
And adequating code to use auto so we don't have unintended conversion from wide string to narrow

[artificiel]

@dimitre it is for sure the objective; but right now /master is broken on windows will all legacy external cases of std::string a = ofToDataPath("a"); and the discussion is about the best way to revert to a functioning state so that existing projects do not break.

then we can figure out the best workaround for a graceful transition to path. (i am suggesting a #define based approach to make it easy to toggle in and out of "narrow-forcing" with the same code base)

[ofTheo]

@dimitre I'd definitely love a cross platform FS solution and as soon as we have one we should merge it, but it does feel unfair to leave Windows in a broken state.

I don't have an easy windows setup anymore, but I'll take a look at see if I can get something that returns string or fs depending on the assignment and if not put in a PR for a minimal revert.

[roymacdonald]

@roymacdonald I think there was a point where the current ofToDataPath with named ofToDataPathFS and then ofToDataPath was:

Yes I am reverting into that point.

then we can figure out the best workaround for a graceful transition to path. (i am suggesting a #define based approach to make it easy to toggle in and out of "narrow-forcing" with the same code base)
I think that something like this would be good.

I have these changes almost done. Will PR soon.

[roymacdonald]

@dimitre I feel we are pointing at different objectives. I totally agree that using filesystem::path is a great idea, but we must not make such transition in a rushed manner. Breaking changes are not nice, specially if those come undocumented and without proper mitigation. Argueing that using auto is the soulion might just help US (like literally us, OF's core developers), but that breaks for many projects that are already working fine, and in many case without any added benefit. As well as breaking the whole addon ecosystem. The transition to use filesystem::path will eventually happen, but we must do it in a graceful and useful way.

[ofTheo]

@roymacdonald here is a quick test solution using a proxy class than can be run in ofApp.cpp without core changes.
If you are able to, could you try on Windows?

Its a bit gnarly in terms of needing operators for the things that are common to std::string and fs::path - I am of mixed feeling on it - but if it works on Windows, maybe its worth looking at? ( name is terrible right now :)

#include "ofApp.h"


namespace of{

	class FilepathProxy {
	public:
		// Constructor to initialize the proxy with a path
		FilepathProxy(const of::filesystem::path &path)
			: path_(path) {}

		// Implicit conversion to std::string (default behavior)
		operator std::string() const {
			return ofPathToString(path_);
		}

		// Implicit conversion to of::filesystem::path (only when assigned to a path)
		operator of::filesystem::path() const {
			return path_;
		}

		// Overload operators to behave like std::string
		std::string operator+(const std::string &rhs) const {
			return static_cast<std::string>(*this) + rhs;
		}
		
		// Overload operator+ to concatenate with char*
		std::string operator+(const char* rhs) const {
			return static_cast<std::string>(*this) + std::string(rhs);
		}

		bool operator==(const std::string &rhs) const {
			return static_cast<std::string>(*this) == rhs;
		}

		// Overload / operator to concatenate paths
		of::filesystem::path operator/(const std::string &rhs) const {
			return path_ / rhs;
		}

		// Overload operator<< for std::ostream
		friend std::ostream& operator<<(std::ostream& os, const FilepathProxy& proxy) {
			os << static_cast<std::string>(proxy);  // Print as a string
			return os;
		}

	private:
		of::filesystem::path path_;  // Store the path
	};

};

of::FilepathProxy getDataPath(const of::filesystem::path & path, bool bAbsolute){
	return of::FilepathProxy( ofToDataPath(path, bAbsolute) );
}


//--------------------------------------------------------------
void ofApp::setup(){

	string str1 = getDataPath("test.png", true);
	auto path = getDataPath("test.png", true);
	of::filesystem::path path2 = getDataPath("test.png", true);
	of::filesystem::path path3 = getDataPath("test.png", true) / "test"  ;
	string str2 = getDataPath("test.png", true) + "/test";

	cout << " test 1 " << getDataPath("test.png", true)  << endl;
	cout << " test 2 " << str1 << endl;
	cout << " test 3 " <<  path << endl;
	cout << " test 4 " <<  path2 << endl;
	cout << " test 5 " <<  path3 << endl;
	cout << " test 6 " <<  str2 << endl;

[roymacdonald]

So this is the PR. So far it builds and the few tests I've runned are working #8141

[roymacdonald]

@ofTheo That FilepathProxy class works on windows.

[danoli3]

file::path changes changes actually fixes the issues on Windows / all platforms for supporting Chinese UTF8+ folder paths so, best to keep them in core and have this work around.

I do recall having issues with the Project Generator with compile time issues for MSVC, should have documented the process a bit more, been a bit out of my head of late. About to dive back into it

[ofTheo]

Thanks for the PR @roymacdonald !
And thanks for testing the proxy approach.

The thing that makes me nervous about the proxy approach is that I can imagine it might not catch all the edge cases in how people use ofToDataPath ( and other functions that now return path instead of string ). But I would love to hear from C++ nerds more knowledgable than me :)

Some possible solutions to this all:

1. Use std::filesystem::path for load / open arguments, but anything that used to return a string still returns string and keep the FS suffix if you need the actual path. While a bit clunky it should be backwards compatible and future proof.
2. Use the above proxy approach for anything that returns a filesystem::path - danger is we are introducing a new type and it will be unclear why.
3. Do a Windows specific fix and keep the rest as is ( don't like this for cross platform compatibility ).
4. Do something like we did with GLM where with a #define you can go between std::string and of::filesystem::path ( downside here is a lot of extra code )
5. A hard break ( which is what we have now ).

Thoughts?
adding in @2bbb @NickHardeman @oxillo

Edit: just saw @danoli3's response.
I know international folder support is a big reason for the FS work. Would 1) above be a good compromise solution?

file::path changes changes actually fixes the issues on Windows / all platforms for supporting Chinese UTF8+ folder paths so, best to keep them in core and have this work around.

Is this currently true?
Does it mean users need to use wstring or does of::filesystem::path myPath; myPath.string() work?

[danoli3]

std::filesystem::path generally works with std::wstring because the underlying file system uses UTF-16 (wide characters) for Unicode support.
Meanwhile, in Linux and macOS, std::string (UTF-8 encoded) is commonly used so string will work there
UTF8 encodings for Windows .string() generally will work for most UTF8 supported languages, however can crash if UTF16 chars used as per Windows spec. (As in, even Emojis etc - used a lot these days by next gen for folder names )

The transition to std::filesystem::path in cross-platform projects is meant to improve compatibility, but it can introduce headaches when dealing with different encodings.

In many cases, you will need to call .u8string() or .wstring() on std::filesystem::path for platform-specific situations to be compliant for system language utf support.

I think therefore the proxy suggested above will work for the time being and maybe we can summon @microsoft to help

@StephanTLavavej would you know anyone who can assist in this for Wstring / string compliance. We are using C++23 latest VS2022

TLDR:

• myPath.string() gives std::string (ASCII/UTF-8, platform-dependent).
• myPath.wstring() ensures wide-character string (used on Windows for Unicode paths).

[artificiel]

curious to hear STL thoughts; the standard does not require supporting a non-native conversation operator, so I doubt movement will occur there. the .string() method exists; our problem is about legacy path code that is (from the point of view of std::filesystem::path) ill-formed.

if I may add a layer, there are 2 distinct issues:

1. what to do right now (i.e. tonight) for windows not to break with legacy code (for which I think we're still targeting C++17? this should be stated somewhere)

2. what to do for "future OF" to support wide unicode on windows in a cross-platform manner (with C++23 latest etc.)

std::filesystem::path is fine for (2) — including ::path- and auto- (and maybe u8string-) awareness (more than std::wstring, as it it is not cross platform). with such a clean state so we don't have to invent anything; just evolve the usage of defaulting to std::string for path representation. as such OF core is already ::path aware.

for (1) I find the transition would be covered simply by having an std::filesystem::path-like class that implicitly converts .string if std::string is the destination OR ofToDataPath() has a windows #define switch that calls .string() (making it non-wide, and non-path, but that's not different from the current situation).

in all cases i don't see why the input signatures are reverted to std::string, as ::path implicitly converts an std::string even on windows (going narrow to wide) — the problem is solely on the output.

[roymacdonald]

in all cases i don't see why the input signatures are reverted to std::string, as ::path implicitly converts an std::string even on windows (going narrow to wide) — the problem is solely on the output.

I reverted to the state where it was not breaking, which included having the inputs as std::string. I think it is a tidier git history if from that state we add a different commit to change those input types.

As for the load functions, there is another discussion about it, but the general point is to get it back to a state where it does not break other code/addons.

So, as to what to do right now, we need to merge this PR I added to the master branch, and create another one for development where we can experiment with moving to using ::path.

So, should I just merge this PR ?

[ofTheo]

@roymacdonald I think initially I was imagining the PR just confined to return types as that seems to be all that is breaking Windows right now.

So going back to the point that things that returned of::filesystem::path had a different name like ofToDataPathFS.

If you want to update your PR to reflect that that would be awesome and we could merge it.

Thanks!!

[roymacdonald]

ok, here it is #8149
yet still I think we need to fix the video player load function