-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
of::filesystem PR #7110
Merged
Merged
of::filesystem PR #7110
Changes from 79 commits
Commits
Show all changes
102 commits
Select commit
Hold shift + click to select a range
2e03222
not working yet
dimitre 2c9f061
some more
dimitre 910f7d2
ok finally
dimitre 554beb1
update
dimitre 992b2d5
of::filesystem
dimitre b101f89
fix non macos files
dimitre 5b4d74e
now working with make
dimitre b7756d9
fileutils
dimitre 2f338b6
tests
dimitre 405273b
fix other platforms
dimitre 9dcd50e
assimp fix
dimitre 2011155
join fix
dimitre 63a3de1
more updates
dimitre d85de52
more updates
dimitre 768da30
more updates 3
dimitre 87e7ee4
more updates 5
dimitre 2c2397d
more 6
dimitre f99500e
update 7
dimitre 9e580bf
fix 8
dimitre 0cbe497
up 9
dimitre 3011a65
workflows and ofxSvg
dimitre 171310c
up 10
dimitre 0a0c8d7
actions
dimitre a7c70ff
ccache
dimitre 16c25a2
ccache
dimitre 685a2d9
act
dimitre ccc8feb
act
dimitre 6edfd28
act
dimitre d1c0a84
act
dimitre 3135f5b
act
dimitre 7b6ee70
act
dimitre d2d2f04
actions
dimitre 98fcfa5
actions
dimitre 5457d30
actions
dimitre 2b8fa6f
actions
dimitre 4f5ee39
make -j -s
dimitre 469685f
up
dimitre a52a6a9
Merge branch 'openframeworks:master' into lite
dimitre 118955c
reverting some updates to master
dimitre 38aba25
reverting
dimitre 49f20f7
updates
dimitre f7a66d1
updates
dimitre d608ceb
revert files
dimitre 8940614
more
dimitre abe1458
updates
dimitre b12b190
ofdirectoshowplayer
dimitre af277ff
font
dimitre 5eefe56
Merge branch 'master' into filesystem
dimitre b0d4765
fix android
dimitre c5b749c
filesystem spaghetti
dimitre f4716a5
update
dimitre 7db22f0
constants
dimitre 696e8fe
ofSetDataPathRoot
dimitre c62e854
update test
dimitre aa06269
updates
dimitre 9d37db4
getAbsolutePath
dimitre 10740c4
unittest
dimitre ac9478e
assimp
dimitre c9b5f18
fix tests for mingw
dimitre fa47f8a
Merge branch 'master' into filesystem
dimitre 3af37ad
update constants
dimitre 6f85521
reverting more files
dimitre cafcc3e
updates
dimitre 7368359
assimp fix?
dimitre 34a7a6b
constants
dimitre a0d6288
namespace fix
dimitre bca09a0
actions
dimitre 5e7134a
filePath
dimitre 162663d
updates
dimitre 269f768
rm removed files
dimitre 9d96dac
updates
dimitre 67216b3
Merge branch 'openframeworks:master' into filesystem
dimitre 469f5a6
fixing conflicts
dimitre b386f80
assimp
dimitre d647ccf
update filesystem::path
dimitre 2b04fe7
fix assimp textures load, still needs some cleanup
dimitre 464f9fb
update
dimitre 2d3706e
Merge branch 'openframeworks:master' into filesystem
dimitre 0dd7001
fix conflicts
dimitre a5f4285
updates
dimitre 74ac3c0
remove constants
dimitre a279999
comment cleanup
dimitre 5d1adf4
remove .string()
dimitre 4cf8462
Merge branch 'master' into filesystem
dimitre 201e5fb
Merge branch 'filesystem' into updatefs
dimitre 65ed292
Merge pull request #16 from openframeworks/updatefs
dimitre 381013b
cubemap fix
dimitre ee78c03
fix assimp
dimitre 2f0d9d4
cleanup comment
dimitre 252a8a6
uniformity in spaces
dimitre 5113cf0
uniformity in spaces
dimitre 5d47d83
cleanup comments
dimitre 8cdf577
comments cleanup
dimitre c36e856
string alongside path
dimitre a89cd81
back to string on return types
dimitre 80c1299
fixes
dimitre 376211e
fix issues
dimitre d2d874e
more fixes
dimitre bb9558e
tests fix
dimitre 70c2fa9
try catch
dimitre fa895ae
try catch in #else
dimitre 76ef977
typo
dimitre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ | |
#include "tinyxml.h" | ||
#endif | ||
|
||
using namespace std; | ||
|
||
/* | ||
Q: what is the which = 0 argument? | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious - does
std::string path = file.getAbsolutePath();
not work with this change?Or is this just being more proper? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this PR getAbsolutePath now returns of::filesystem::path instead of string, so it is needed to convert to string in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - I think that could break a ton of addons/projects/code etc if it doesn't auto convert.
One thing that might work is to have
std::string
andof::filesystem::path
versions of both with deprecation?Or maybe we could figure out a way to have path auto convert to string with = operator overload?
I think without it we'll break a ton of stuff 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right. I'm good about any of this. maybe keeping std::string for legacy compatibility, I can do that soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot have both functions with same name and different return type. I'm unsure of how to proceed here. I know it is not your style but I would suggest we switch to of::filesystem and live with labor pains.
What is your recommendation on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation behind this PR: uniformize of::filesystem outside of std, instead of try to tuck everything inside std::filesystem (there are different versions of c++11, experimental c++17 and boost filesystem)
And another motivation is try to rely more on filesystem::path instead of converting from string to path and vice versa depending on the functions.
3 . If approved other leftovers can be done in separate PRs easier to read. I can see other places like
ofCairoRenderer::setup(const std::string & _filename
andsetupShaderFromSource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK very clear! i'm not in a position to test things but as you propagated the changes in a good number of components with success it's clearly a positive move forward. my only comment (and maybe that falls in a "style" discussion) it so committing commented unused/updated code -- i understand the value of that while working, being able to quickly refer to the past and toggle "versions", but in committed core code it adds a bit of noise (even more so within large
#ifdef
blocks like in ofConstants.h which are hard to follow). it also provides traction for the of:: namespace.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree @artificiel. I've just cleaned out the comments. Thanks for the reading.
@ofTheo I really think the potential cons are outweighted by pros of this change
And this can be useful for tackle some projectGenerator issues, which uses a mixture of filesystem and Poco::Path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artificiel some more context for the changes are in this issues:
#6585 (comment)
Will add to the main PR description to link the issues.
@dimitre - going to take a look at this today.
Most nervous about changing the return types for the ofFile:: call from string to path. I def agree path is better especially for international support, but thinking about all the things this could break ( similar of a break as going from ofPoint to glm::vec3 ).
Going to see if there is a solution so we can have cake and eat it 🙂
Curious if anyone has creative ideas there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i must confess i'm not sure about what requires the of:filesystem (vs direct std::filesystem) but i'm presuming that has been sorted out and is the right thing to do — it does not change the fact that returning one or the other is different than std::string.
direct implicit conversions such as
std::string p = my_dir.getAbsolutePath();
will fail on windows. but the currentgetAbsolutePath()
method makes use of.string()
to output strings:openFrameworks/libs/openFrameworks/utils/ofFileUtils.cpp
Lines 1207 to 1212 in edb72bd
which means it forces the conversion of windows wstring to string, which is convenient but "incorrect"....
what is going to break globally is things like the usage of strings to construct ad-hoc paths:
but that usage should be definitely be shifted (not advocating this a good path strategy just pointing the "most-direct-adaptation" here):
so these breaking changes are breaking on things that should be fixed.
the glm transition example is interesting, and i remember the moment of that change
OF_USE_LEGACY_VECTOR_MATH
seemed like a good thing, but in truth it's like making a deal with the credit card company: feels like a win today, but in the end you pay more anyhow. (and someone paid for all those #ifdefs's!)(and the case of filesystem::path is even more necessary as it's a conceptual transition upgrading a bare string to a structured object, not a transition between somewhat "equivalent" systems).
one thing though: this means that all the interfaces and all the examples must be adapted — to a learning mind, a stale example is worse than no example... it should not be hard, but it has to be done coherently.
i will allow myself to re-phrase my take on the relationship between to OF source and one's projects:
(i find the OF deprecation strategy very nice -- the old functions can hang in there for ever, and politely point out the better option to the user; unfortunately there is no way to do something like that with return types).