Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Some fixes for search paths: #17435

Merged
merged 3 commits into from
Mar 6, 2017
Merged

Some fixes for search paths: #17435

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
961e195
Some fixes for search paths:
Mar 3, 2017
035e52f
Checks whether the parameter ‘searchResolutionOrder’ passed in is the…
Mar 6, 2017
9334eec
Don’t break the compatibility, add a new method called ‘FileUtils::ge…
Mar 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 41 additions & 10 deletions cocos/platform/CCFileUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,9 +860,11 @@ std::string FileUtils::fullPathFromRelativeFile(const std::string &filename, con
void FileUtils::setSearchResolutionsOrder(const std::vector<std::string>& searchResolutionsOrder)
{
bool existDefault = false;
std::vector<std::string> searchResolutionOrderCopied = searchResolutionsOrder;

_fullPathCache.clear();
_searchResolutionsOrderArray.clear();
for(const auto& iter : searchResolutionsOrder)
for(const auto& iter : searchResolutionOrderCopied)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need to copy searchResolutionsOrder ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose this invocation:

fileUtils->setSearchResolutionsOrder(_searchResolutionsOrderArray);

_searchResolutionsOrderArray will be cleared before the for loop.
The same situation could happen in setSearchPaths.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

fileUtils->setSearchResolutionsOrder(fileUtils->getSearchResolutionsOrder());
fileUtils->setSearchPaths(fileUtils->getSearchPaths());

will aways fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you are right. But i think it is better to check it at first like

if (_searchResolutionsOrderArray == searchResolutionsOrder)
    return;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For setSearchResolutionsOrders, we could.
But for setSearchPaths, we couldn't.
Because setSearchPaths will update _searchPathArray which will be affected by _defaultResourceRootPath.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, i just mean searchResolutionsOrder.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minggo , updated setSearchResolutionsOrder.

{
std::string resolutionDirectory = iter;
if (!existDefault && resolutionDirectory == "")
Expand Down Expand Up @@ -904,44 +906,62 @@ const std::vector<std::string>& FileUtils::getSearchResolutionsOrder() const

const std::vector<std::string>& FileUtils::getSearchPaths() const
{
return _searchPathArray;
return _originalSearchPaths;
}

void FileUtils::setWritablePath(const std::string& writablePath)
{
_writablePath = writablePath;
}

const std::string& FileUtils::getDefaultResourceRootPath() const
{
return _defaultResRootPath;
}

void FileUtils::setDefaultResourceRootPath(const std::string& path)
{
_defaultResRootPath = path;
if (_defaultResRootPath != path)
{
_fullPathCache.clear();
_defaultResRootPath = path;
if (!_defaultResRootPath.empty() && _defaultResRootPath[_defaultResRootPath.length()-1] != '/')
{
_defaultResRootPath += '/';
}

// Updates search paths
setSearchPaths(_originalSearchPaths);
}
}

void FileUtils::setSearchPaths(const std::vector<std::string>& searchPaths)
{
bool existDefaultRootPath = false;
_originalSearchPaths = searchPaths;

_fullPathCache.clear();
_searchPathArray.clear();
for (const auto& iter : searchPaths)

for (const auto& path : _originalSearchPaths)
{
std::string prefix;
std::string path;
std::string fullPath;

if (!isAbsolutePath(iter))
if (!isAbsolutePath(path))
{ // Not an absolute path
prefix = _defaultResRootPath;
}
path = prefix + (iter);
fullPath = prefix + path;
if (!path.empty() && path[path.length()-1] != '/')
{
path += "/";
fullPath += "/";
}
if (!existDefaultRootPath && path == _defaultResRootPath)
{
existDefaultRootPath = true;
}
_searchPathArray.push_back(path);
_searchPathArray.push_back(fullPath);
}

if (!existDefaultRootPath)
Expand All @@ -962,10 +982,21 @@ void FileUtils::addSearchPath(const std::string &searchpath,const bool front)
{
path += "/";
}

if (front) {
_originalSearchPaths.insert(_originalSearchPaths.begin(), searchpath);
_searchPathArray.insert(_searchPathArray.begin(), path);
} else {
_searchPathArray.push_back(path);
_originalSearchPaths.push_back(searchpath);

if (!_searchPathArray.empty() && _searchPathArray[_searchPathArray.size()-1] == _defaultResRootPath)
{
_searchPathArray.insert(_searchPathArray.begin() + _searchPathArray.size() -1, path);
Copy link
Author

@dumganhar dumganhar Oct 27, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbrednikhin, @balmerdx
So the key point is that this line push the default resource path to the end, right?
Yes, I know I broke the compatibility here since I think the search path of default resource should always be at the end. I don't know what kinds of situation you will do in your game. Perhaps, more code snippets and more detail are welcome.

Anyway, if you think I was wrong, feel free to submit a new or better pull request to correct my mistake. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care where default resource path is, actually. My only point is that if you breaking regular code behavior, causing people to fix their code (which must rely on some order of paths), you should have some good reason for that. Not just thinking that it is better.
Since at least some people don't see any obvious reason for that change, you could point on it in comment or something.
This project is open source common code - not your own branch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK，you're totally right, thanks for reminding me. I will revert that line. I was wrong, I used it as 'my own my private branch', good luck guys, thank you guys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, for understanding. We are very appreciate your work.

}
else
{
_searchPathArray.push_back(path);
}
}
}

Expand Down
10 changes: 10 additions & 0 deletions cocos/platform/CCFileUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,11 @@ class CC_DLL FileUtils
*/
virtual void setSearchPaths(const std::vector<std::string>& searchPaths);

/**
* Get default resource root path.
*/
const std::string& getDefaultResourceRootPath() const;

/**
* Set default resource root path.
*/
Expand Down Expand Up @@ -873,6 +878,11 @@ class CC_DLL FileUtils
*/
std::vector<std::string> _searchPathArray;

/**
* The search paths which was set by 'setSearchPaths' / 'addSearchPath'.
*/
std::vector<std::string> _originalSearchPaths;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need _originalSearchPaths

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileUtils uses _searchPathArray for searching full path internally.
setSearchPaths will modify _searchPathArray variable, it will be connected by _defaultResourceRootPath.
_originalSearchPaths saves the original parameter setSearchPaths passes in, and getSearchPaths returns _originalSearchPath will be more reasonable than returning _searchPathArray. It's the rule of getter returns what user has set.

If getSearchPaths returns _searchPathArray, you could suppose the following code will always not work.

If current search path is bbb/, ccc/

auto oldDefaultResourceRoot = fileUtils->getDefaultResourceRootPath(); // Suppose it return "" which is an empty string.
auto oldSearchPaths = fileUtils->getSearchPaths(); // oldSearchPaths is : ["bbb/", "ccc/"]
fileUtils->setDefaultResouceRootPath("aaa"); // oldSearchPath is : ["aaa/bbb/", "aaa/ccc"]
// oldSearchPaths will be modified by adding a prefix `aaa` since `_searchPathArray` was modified. 

... ...

// reset resource root
fileUtils->setDefaultResourceRootPath(oldDefaultResourceRoot); 
auto newSearchPath = fileUtils->getSearchPaths(); // newSearchPath is incorrect: ["aaa/bbb", "aaa/ccc"] since it returns `_searchPathArray` which was added by prefix 'aaa'


/**
* The default root path of resources.
* If the default root path of resources needs to be changed, do it in the `init` method of FileUtils's subclass.
Expand Down
21 changes: 20 additions & 1 deletion tests/cpp-tests/Classes/FileUtilsTest/FileUtilsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,25 @@ void TestSearchPath::onEnter()
fclose(fp);
}
}

// Save old resource root path
std::string oldDefaultRootPath = sharedFileUtils->getDefaultResourceRootPath();
sharedFileUtils->setDefaultResourceRootPath("extensions");
auto sp1 = Sprite::create("orange_edit.png");
sp1->setPosition(VisibleRect::center());
addChild(sp1);

// Recover resource root path
sharedFileUtils->setDefaultResourceRootPath(oldDefaultRootPath);

auto oldSearchPaths = sharedFileUtils->getSearchPaths();
sharedFileUtils->addSearchPath("Images");
auto sp2 = Sprite::create("btn-about-normal.png");
sp2->setPosition(VisibleRect::center() + Vec2(0, -50));
addChild(sp2);

// Recover old search paths
sharedFileUtils->setSearchPaths(oldSearchPaths);
}

void TestSearchPath::onExit()
Expand All @@ -155,7 +174,7 @@ std::string TestSearchPath::title() const

std::string TestSearchPath::subtitle() const
{
return "See the console";
return "See the console, can see a orange box and a 'about' picture";
}

// TestFilenameLookup
Expand Down