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.

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

Some fixes for search paths: #17435

Merged
merged 3 commits into from Mar 6, 2017
Merged

Conversation

dumganhar
Copy link

@dumganhar dumganhar commented Mar 3, 2017

  1. Adds ‘_originalSearchPaths’ variable, ’getOriginalSearchPaths’ returns the original values set by ‘setSearchPaths’ or ‘addSearchPath’.
  2. Adds a getter function ‘getDefaultResourceRootPath’.
  3. ‘setDefaultResourceRootPath’ should also update search paths and remove file path cache internally.
  4. ‘setSearchPaths’ supports to pass self (_originalSearchPath), could be used in ‘setDefaultResourceRootPath’ to update the final ’_searchPathArray’ for searching full path.
  5. ‘addSearchPath’ fix, the default resource root path should be the last element in ‘_searchPathArray’.
  6. 'setSearchResolutionsOrder' supports to pass self '_searchResolutionsOrderArray' as parameter.

1. Adds ‘_originalSearchPaths’ variable, ’getSearchPaths’ returns the original values set by ‘setSearchPaths’  or ‘addSearchPath’.
2. Adds a getter function ‘getDefaultResourceRootPath’.
3. ‘setDefaultResourceRootPath’ should also update search paths and remove file path cache internally.
4. ‘setSearchPaths’  supports to pass self (_originalSearchPath), could be used in ‘setDefaultResourceRootPath’ to update the final ’_searchPathArray’ for searching full path.
5. ‘addSearchPath’ fix, the default resource root path should be the last element in ‘_searchPathArray’.
_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.

@@ -874,6 +879,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'

@dumganhar
Copy link
Author

@kpkhxlgy0, any thoughts about this pull request since is inspired by your PR (#11195).

@minggo minggo merged commit bdcacd0 into cocos2d:v3 Mar 6, 2017
@minggo minggo added this to the 3.15 milestone Mar 6, 2017
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Mar 9, 2017
…futurePRs

* commit '5c0c6d2e1e4b1772efc239d23f8cdaf8ce79a898':
  More powerful Uri class, adds unit test for Uri class. Refactors some code in SocketIO & Websocket. (cocos2d#17472)
  Prevent signed/unsigned mismatch (cocos2d#17468)
  fix the issue that onEnterTransitionDidFinish() is not invoked at first scene change (cocos2d#17466)
  csbload error bug (cocos2d#17465)
  *Fix: GLThread and other threads access sDownloaderMap in a thread-unsafe way, which will result in crash in some occasion on Android. (cocos2d#16326)
  fixed cocos2d#17416: Wrong default port of SocketIO connection. (cocos2d#17459)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17458)
  fixed cocos2d#17427: lws_parse_url has wrong behavior, it parses ’ws://domain.com/abc/d’ to ‘path: abc/d’ rather than ‘path: /adb/d’. (cocos2d#17455)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17456)
  Prevent unexpected calls to unscheduled selector in long updates. (cocos2d#17431)
  fix the broken of lua-tests after PR cocos2d#17445 was merged. (cocos2d#17454)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17453)
  Solve the error logic in spine runtime. (cocos2d#17448)
  cocos2d::Sequence::isDone() checks that the last action is actually done. (cocos2d#17437)
  Websocket bug fix after PR#17440 was merged. (cocos2d#17450)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17449)
  Some fixes for search paths: (cocos2d#17435)
  update external version (cocos2d#17446)
  fixed cocos2d#17433: [webSocket ] webSocket random crash in win32 (cocos2d#17440)
  Fix some local variable names in tests (cocos2d#17445)

# Conflicts:
#	web
@sbrednikhin
Copy link
Contributor

Adding FileUtils::getOriginalSearchPaths is good thing, but what about people who rely on old resource path order?
Making original search paths last in the list looks strange (and I think might have some additional performance cost).

@balmerdx
Copy link

balmerdx commented Oct 27, 2017

This commit is not good.

  1. Break backward compatibility.
  2. Strange logic. Why one path go to back?????? Confuse interface.
  3. Not add new functionality.

In older code is several variants to implement dumganhar logic without change FileUtils class.

  • addSearchPath(..., front=true)
  • get/setSearchPaths and change path order.


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

@dumganhar dumganhar Oct 27, 2017

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.

dumganhar pushed a commit to dumganhar/cocos2d-x that referenced this pull request Oct 30, 2017
If the last search path is _defaultResRootPath, addSearchPath(path) should not force that the last path is still default resource root path.
I made this mistake and broke the compatibiliy, since I thought resource root path definitely is low priority while search in most case. However, I should not make this assumption.

Anyway, dear developers, thanks for all of you review cocos2d-x source code. I accept any suggestion, any blame. I think everyone makes mistake more or less.
The key point is that `PLEASE DON'T MAKE THE SAME MISTAKE SECOND TIME, THIRD TIME`.
So please don't blame our developers who makes mistakes. We need to work together with all our developers to make our engine better and better.

And in fact, I never think that I deal with cocos2d-x source code as my 'own branch' although I contributed lots of code to it. I didn't push any code to repo directly since I sent Pull Requests. Every one could review code and comment in pull request. We encourage more communication with our developers in github or the forum. You could say GOOD , BAD or SHIT, but please do your best to respect every developer who made a contribution.

Thanks all of you. Good luck!
minggo pushed a commit that referenced this pull request Nov 6, 2017
If the last search path is _defaultResRootPath, addSearchPath(path) should not force that the last path is still default resource root path.
I made this mistake and broke the compatibiliy, since I thought resource root path definitely is low priority while search in most case. However, I should not make this assumption.

Anyway, dear developers, thanks for all of you review cocos2d-x source code. I accept any suggestion, any blame. I think everyone makes mistake more or less.
The key point is that `PLEASE DON'T MAKE THE SAME MISTAKE SECOND TIME, THIRD TIME`.
So please don't blame our developers who makes mistakes. We need to work together with all our developers to make our engine better and better.

And in fact, I never think that I deal with cocos2d-x source code as my 'own branch' although I contributed lots of code to it. I didn't push any code to repo directly since I sent Pull Requests. Every one could review code and comment in pull request. We encourage more communication with our developers in github or the forum. You could say GOOD , BAD or SHIT, but please do your best to respect every developer who made a contribution.

Thanks all of you. Good luck!
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Nov 19, 2017
* commit 'bceb242ebd9c0bf60fdf1cfcf671efc47f07e1ee': (28 commits)
  no message (cocos2d#18460)
  Added missing #include statement (cocos2d#18452)
  Fix cocos2d::log va_list re-use bug (cocos2d#18426)
  Improve StringUtils::format implementation. (cocos2d#18425)
  RichText improvements (cocos2d#18447)
  Update CCTableView.cpp (cocos2d#18451)
  Update UIWebViewImpl-ios.mm (cocos2d#18448)
  Reverts add search path logic in PR cocos2d#17435 . (cocos2d#18419)
  Fix 60 fps for android (cocos2d#18445)
  set texture anti-alias by deault in RenderTexture (cocos2d#18442)
  fix typos (cocos2d#18444)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18440)
  Spine update (cocos2d#18438)
  remove vs2013 libs (cocos2d#18439)
  Revert cocos2d#18327 (cocos2d#18436)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18435)
  update glfw to 3.2.1 (cocos2d#18434)
  Editbox init member variable (cocos2d#18432)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18431)
  Update spine (cocos2d#18427)
  ...

# Conflicts:
#	build/cocos2d_libs.xcodeproj/project.pbxproj
#	cocos/Android.mk
#	cocos/base/CCConsole.cpp
#	cocos/editor-support/spine/Android.mk
#	cocos/editor-support/spine/Animation.c
#	cocos/editor-support/spine/Animation.h
#	cocos/editor-support/spine/AnimationState.c
#	cocos/editor-support/spine/AnimationState.h
#	cocos/editor-support/spine/AnimationStateData.h
#	cocos/editor-support/spine/Atlas.c
#	cocos/editor-support/spine/Atlas.h
#	cocos/editor-support/spine/AtlasAttachmentLoader.c
#	cocos/editor-support/spine/AtlasAttachmentLoader.h
#	cocos/editor-support/spine/Attachment.h
#	cocos/editor-support/spine/AttachmentLoader.h
#	cocos/editor-support/spine/Bone.c
#	cocos/editor-support/spine/Bone.h
#	cocos/editor-support/spine/BoneData.h
#	cocos/editor-support/spine/BoundingBoxAttachment.c
#	cocos/editor-support/spine/BoundingBoxAttachment.h
#	cocos/editor-support/spine/CMakeLists.txt
#	cocos/editor-support/spine/Event.h
#	cocos/editor-support/spine/EventData.h
#	cocos/editor-support/spine/IkConstraint.c
#	cocos/editor-support/spine/IkConstraint.h
#	cocos/editor-support/spine/IkConstraintData.h
#	cocos/editor-support/spine/MeshAttachment.c
#	cocos/editor-support/spine/MeshAttachment.h
#	cocos/editor-support/spine/PathAttachment.c
#	cocos/editor-support/spine/PathAttachment.h
#	cocos/editor-support/spine/PathConstraint.c
#	cocos/editor-support/spine/PathConstraint.h
#	cocos/editor-support/spine/PathConstraintData.h
#	cocos/editor-support/spine/RegionAttachment.c
#	cocos/editor-support/spine/RegionAttachment.h
#	cocos/editor-support/spine/Skeleton.c
#	cocos/editor-support/spine/Skeleton.h
#	cocos/editor-support/spine/SkeletonAnimation.cpp
#	cocos/editor-support/spine/SkeletonAnimation.h
#	cocos/editor-support/spine/SkeletonBatch.cpp
#	cocos/editor-support/spine/SkeletonBatch.h
#	cocos/editor-support/spine/SkeletonBinary.c
#	cocos/editor-support/spine/SkeletonBinary.h
#	cocos/editor-support/spine/SkeletonBounds.c
#	cocos/editor-support/spine/SkeletonBounds.h
#	cocos/editor-support/spine/SkeletonData.h
#	cocos/editor-support/spine/SkeletonJson.c
#	cocos/editor-support/spine/SkeletonJson.h
#	cocos/editor-support/spine/SkeletonRenderer.cpp
#	cocos/editor-support/spine/SkeletonRenderer.h
#	cocos/editor-support/spine/Skin.h
#	cocos/editor-support/spine/Slot.c
#	cocos/editor-support/spine/Slot.h
#	cocos/editor-support/spine/SlotData.c
#	cocos/editor-support/spine/SlotData.h
#	cocos/editor-support/spine/TransformConstraint.c
#	cocos/editor-support/spine/TransformConstraint.h
#	cocos/editor-support/spine/TransformConstraintData.h
#	cocos/editor-support/spine/VertexAttachment.c
#	cocos/editor-support/spine/VertexAttachment.h
#	cocos/editor-support/spine/extension.c
#	cocos/editor-support/spine/extension.h
#	cocos/editor-support/spine/kvec.h
#	cocos/editor-support/spine/proj.win32/libSpine.vcxproj
#	cocos/editor-support/spine/proj.win32/libSpine.vcxproj.filters
#	cocos/editor-support/spine/spine.h
#	cocos/ui/UIRichText.h
#	plugin
#	tools/cocos2d-console
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Nov 21, 2017
steve: got rid of spine folder as well

* commit 'db1102492452faea78746da7e3cf29fa22c69c2a':
  Reverts add search path logic in PR cocos2d#17435 . (cocos2d#18419)
  Fix 60 fps for android (cocos2d#18445)
  set texture anti-alias by deault in RenderTexture (cocos2d#18442)
  fix typos (cocos2d#18444)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18440)
  Spine update (cocos2d#18438)
  remove vs2013 libs (cocos2d#18439)
  Revert cocos2d#18327 (cocos2d#18436)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18435)
  update glfw to 3.2.1 (cocos2d#18434)
  Editbox init member variable (cocos2d#18432)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18431)

# Conflicts:
#	cocos/editor-support/spine/SkeletonJson.c
#	cocos/ui/UIRichText.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants