ofToDataPath simplification

[dimitre]

ofToDataPath can now be hugely simplified using filesystem functions.

[artificiel]

my angle on ofToDataPath() is that it should support searching in multiple places. right now the logic is to locate a dataPathRoot() (which wraps defaultDataPath()) and then construct file paths based on that unique root.

why multiple roots? the case of the .app bundle is one (that could be solved in different ways) but I see value for a system that searches for resources in different places, either automatically (OS-sanctioned areas, ofxiOSGetDocumentsDirectory(), shared directories, etc) or manually (ofSetDataPathRoot(path) becomes ofAddDataPathRoot(path)). the MaxMSP search paths are an inspiration for that, allowing dynamic (runtime) "pointing" to resources in addition to the "by default" adjacent space. this opens up another conversation about what constitutes a resource ("required" assets, shaders, etc vs My movie (1).mov), but in all cases I will advocate for anything that enhances malleability for unexpected future usage (moreover if we open up the mind to apps distributed at large to "users").

however to do that it means that ofToDataPath() moves from a path-building function to something that actually looks in the filesystem to find things (to an extent, it already does to find data/), and returns not only "legal paths" (from the point of view of std::filesystem::path) but paths actually backed by a file on the filesystem (overlapping with ofFile::exists()).

there is already a bit of confusion, for instance:

openFrameworks/addons/ofxSvg/src/ofxSvg.cpp

Lines 32 to 52 in f71884f

	std::string file = ofToDataPath(fileName);
	// FIXME: I think this is the equivalent of .empty() which is simpler.
	// maybe use file exists to check instead?
	// if(fileName.compare("") == 0){
	// ofLogError("ofxSVG") << "load(): path does not exist: \"" << fileName << "\"";
	// return;
	// }
	// ofBuffer buffer = ofBufferFromFile(fileName);
	// loadFromString(buffer.getText(), fileName);
	if(file.compare("") == 0){
	ofLogError("ofxSVG") << "load(): path does not exist: \"" << file << "\"";
	return;
	}
	ofBuffer buffer = ofBufferFromFile(file);
	loadFromString(buffer.getText(), file);

• indeed, .empty() should be used instead of .compare("") or =="" as it is guaranteed to be as efficient as possible, and also in line with the STL containers and most modern C++ libs. (related topic: initialization of empty strings should be done with {} and not by assigning an empty string ="" — that includes method parameters that default to empty string. 2 low-hanging PR's there...)

• that being said, ofToDataPath() does not validate the existence of the underlying file, so this condition is not really useful (it will branch only of the user does .load("")).

• (on top, there is error handling within loadFromString() that would catch a bad path, so redundancy)

where I'm heading is that ofToDataPath() should gain the responsibility of validating paths and return a valid path, or nullopt (yes I'm sneaking in std::optional here), since it changes the return type, maybe a new method ofConcreteDataPath(), so the above code could become:

if (auto concrete_file = ofConcreteDataPath(fileName)) {
    auto buffer = ofBufferFromFile(concrete_file);
    if (loadFromString(buffer.getText(), concrete_file)) isLoaded = true;
    // NB optional adopts pointer operator so perhaps needed here or adapted further in
} 
//else {
//  fileName was not found in any dataPaths...
//  but local code does not need to deal with path existence (good separation!)
//  (a "file not found" warning would already be emitted within ofConcreteDataPath())
//  nevertheless, if something meaningful has to happen here the nullopt `else` branch works
//}

note that that's actually independent of searching into multiple places... sorry for not being totally structured! so (a) make ofToDataPath poly-directory and (b) make ofToDataPath confirm paths.

and note that @arturoc mentions overhauling return types here #4998 (comment) where std:expected would work fine, so maybe we skip optional altogether (it's so pre-pandemic anyway) and head straight into expected? (am I scaring you enough yet?)

(note that ofBufferFromFile() is incorrectly named and should be ofBufferFromPath() (as it actually takes a path and constructs an ofFile within))

[dimitre]

I totally agree with your view. maybe a vector of fs::path to look for, for all platforms, and macos has two paths already initialized.
Thinking of this PR as a midway to get there, do you think its logic makes sense?
most functions from ofFilePath can be substituted easily for of::filesystem ones,
we don't need addtrailingslash anymore too.
if we don't use declared separators as '/' we dont need to use path::make_preferred

[artificiel]

I will not be able to read carefully today, but one issue that has to be addressed is what happens when you construct a path for writing (as ofSaveImage(pix, "myFutureImage.png") calls ofToDataPath, and if the path is relative, build one into data().

So there needs to be a "deterministic" dataPath that is used to write to. If this idea of multidatapath is to go somewhere, maybe the data paths need to be qualified in some way? brainstorm: OF_DATA_INTERNAL, OF_DATA_USER, OF_DATA_GENERATED, so when asking for a write-destined dataPath the developer can "hint" which one. In the case the macOS bundle issue, it definitely makes no sense to write user-data inside the app bundle, so it's a case where the app must find at runtime things within the bundle, and write outside the bundle. I don't know what are the best practices in iOS OF but in my case I'm using ofxiOSGetDocumentsDirectory() with absolute paths do download ressources and basically skip ofToDataPath(). I don't find that "platform exception" to be a good thing. So basically unwrap the function of ofxiOSGetDocumentsDirectory() in a generalized pattern.

also I had this on my reading list specifically around #1523 (comment) as evidently considerable work is into ofToDataPath et al. and whatever "upgrade" to the interface has to be carefully designed.

and as far as the error checking, I think it is important to clearly define which step is responsible for the file existence or writability (I argue it is not the app or addon writer's responsibly), and from there look a a few concrete scenarios to test properly how a design would work in actual use. (I was a bit kidding about std::expected as a "valid path or nullopt" is exactly a use case for std::optional). [edit to add]: and in the case of an unfound file it is more a matter of handling a minor "disappointment" than an actuall "error" and should be processed as such.

[dimitre]

Thanks. I'll be reading carefully your comments later.

Just a quick note: I don't think Poco::path should be used now we have fs.
The only place I've seen is in projectGenerator and it needs badly some love.

I appreciate you think some steps ahead. Anyway just to think of "here and now" I would like your opinion on this PR as it is, compared to the master code, if you see any downside on this change.

[artificiel]

oh no don't worry I'm not advocating poco at all! (that thread is from 2012) just pointing out the specific comment indicating there are tricky aspects to "low level" aspect dataPathRoot() and the evolution of the method should be reviewed in order not to miss anything that might have been solved before (in terms of design, not implementation)

[artificiel]

I was starting to go through the specific changes and was quickly "drifting out" towards "thinking steps ahead"...

ex: the section about finding the data/ folder in bundled macOS apps, I'm not sure which should be the default: checking first ../../.. means a user placing a bundled macOS app besides a folder name data/ (accidentally or not) will not get access to it's bundled ressources. perhaps the default should be bundled so it has precedence? but then ofSaveImage(ofToDataPath("screen.png")) ends up inside the app... this immediately leads to the idea of supporting more than one path, especially considering we need an implicit path for bundled ressources, and another one for user-produced stuff.

so what I mean is as far as "review" goes it helps to have specific goal or problems to check, so:

• for the reduction of complexity of the try/catch stuff, if it is effectively resolved by the std::filesystem without changing the behaviour (I presume the unit tests are already setup for spotting that kind of thing? although the windows file dialog ) it's a no-brainer!

• for the handling of macOS bundled data, as I mentioned in macOS: search for data within .app bundle if not found besides app #7361 an imperfect solution is better than none, but there is some urgency to think about what we want in terms of design, and while the specific problem stems from macOS (and iOS to an extent) it should prompt a wider look (especially considering "users" of "apps" (vs developers compiling along their data/). also I think we should pass ${UNLOCALIZED_RESOURCES_FOLDER_PATH} from Xcode as "../Resources" comes from there and it's were Xcode will actually copy data (we take it for granted but something might change it) -- but I don't know the correct way to do that (it's an Xcode template project thing). so maybe start by using a #define MACOS_UNLOCALIZED_RESOURCES_FOLDER_PATH "../Resources" and work from there?

also line 386 of ofCubemap.cpp, going from:

std::string encFolder = data->settings.cacheDirectory;

to

of::filesystem::path encFolder = data->settings.cacheDirectory;

is the perfect example of why auto is a better idiom...!

[dimitre]

Great, thanks @Artificel
Now I see why testing both paths could lead to unexpected results.
Maybe we should left as it is default (../../../data in macOS) and the user has to define this change.
A helper function can be useful too, like

ofSetDataPathToBundle(); // horrible name

[dimitre]

about the tests of this PR.
I've noticed some tests fails on windows because it is expecting a trailing slash, but it is not needed in fs::path,
so to fix this we have to update the tests.
what do you think @ofTheo and @artificiel?

[artificiel]

ofSetDataPathToBundle(); // horrible name

this approach is not ideal as the fact that the app is bundled might be a "later" decision. if you write shader.load("dude") you want it in ../../../data until you enable OF_BUNDLE_DATA_FOLDER = 1 then it should mean ${UNLOCALIZED_RESOURCES_FOLDER_PATH}/data BUT ofSaveImage("capture.png") should never save an image within the app bundle. and as an app writer you certainly don't want if (appIsBundled()) { } else { } at user-level code.

hence the discussion about multiple dataPaths and ways of hinting/distinguishing them. maybe multiple data paths as free "search paths" is not the correct idea, and something like resourcesDataPath() vs userDataPath(), which by default could both overlap in ../../../data like currently (but userDataPath = ofxiOSGetDocumentsDirectory() in iOS, and the search strategy of macOS), and then the app or addon writer gets to decide which one to use at all times / all platforms? (I switch the same apps from Mac an linux and would hate to have platform-specific stuff like that to handle (I already dislike ofxiOSGetDocumentsDirectory() but iOS apps are inherently different it does not really count as "cross platform"). We can presume the code author knows if the ressource is an internal one like a font or shader, or a "media" that the user reads/writes.

(I would also jot here not to forget the case of a multi-user linux system with a common /opt/openFrameworks install, the "data" could also be separated between the common path (resources for the examples, whatever in apps/) and something in the user's directory if that common place is not writable (ex: ~/openFrameworks/$appname/data). I have often written tools that could be used by many users, and it's always tricky (of course simply compiling is tricky in a shared env, but I'm thinking towards people running a compiled binary that could be centrally installed)).

(and while we're taking about dataPath I cannot also omit my reflection of making the function a validator of the given path, hence std::optional<of::filesystem::path> return -- which has deeper ramifications);

[artificiel]

to complete the above:

shader.load("dude"); // must be found in bundle
ofLoadImage("test_pattern.png"); // must be found in bundle
ofSaveImage("capture.png"); // cannot save within bundle -- automatic deduction of writable place?
ofLoadImage("capture.png"); // must be found either in or out of bundle! 
                            // hence my initial "search path" idea" which is the transparentest

or:

shader.load(ofResourceData("dude"));
ofSaveImage(ofUserData("capture.png"));
ofLoadImage(ofUserData("capture.png"));
ofLoadImage("capture.png"); // defaults to user-space

ofLoadImage("relative/path/i.png") would default to be placed within ofUserData() which defaults to ../../../data (and is less specialized than ofResourceData(), which would be required by bundled apps authors (or addons authors providing assets))

[dimitre]

Lots of deep insights here. Thanks @Artificel.
I now understand what you mean and agree there are lots of subleties to be taken care of.
I linked this discussion here #7042
so it doesn't get lost

Do you think code in this PR has equivalent functionality of openframeworks:master?
In the case we don't close this PR, should it keep functionality of this PR? checking second folder? #7361
or maybe postponing this decision after a more detailed discussion?

[artificiel]

yes! for the macOS bundle part, this is a good first step (i.e. it's not perfect but you can now bundle an app and have it running (combined with #7358 you can even copy it around with custom dylibs -- I'm doing just that)).

for the general dataPath simplification, if the tests are good I don't see a problem but I'll let some other eyes with more experience with that code confirm.

[dimitre]

Great! I'm hoping #7358 get merged soon and I love the idea of separating the script

[ofTheo]

Might be an edge case here if you pass in an absolute path and makeAbsolute is true.
ie: path is /Users/me/Desktop/image.jpg if makeAbsolute is true and the path is already absolute then it would prepend dataPath ie: /Users/me/Downloads/OF/apps/myApp/bin/data/Users/Theo/Desktop/image.jpg

does this edge case exist in the original code? looks like it might?

[dimitre]

Great, I'll be investigating this better now

[ofTheo]

these should prob be in another PR but fine for this one.

Maybe we should start using auto instead of of::filesystem::path for local vars going forward as it will make the code more adaptable in the future.

[dimitre]

Yes +1 for auto.

[dimitre]

I think this specific one should not be auto, because data->settings.cacheDirectory is a string, so I'm building an of::filesystem::path from that.

[ofTheo]

@dimitre Seems like a few of the tests are failing on Linux too:

I think to merge this we should try and get it to pass the tests as is, as some of the small differences could cause real world issues.

[dimitre]

Yes, I think there is more to it, and I am investigating the issue now