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

fix for loading cached dts from zip archives #1466

Closed
wants to merge 3 commits into from
Closed

fix for loading cached dts from zip archives #1466

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2015

This fixes a bug where if you attempted to load cached dts files from a zip archive it would fail. This was because it was attempting to extract the file time information from non-existent files outside of the archive. Thus, canLoadCachedDTS would return false.

I added a line to disable the file change notification errors for directories that don't exist or are in zip archive.

Additionally, I added *.pak to the search pattern for archives.

@crabmusket crabmusket added the Bug label Nov 18, 2015
@crabmusket
Copy link
Contributor

This seems like a good idea! This is looking for existing cached DTS files in zips, correct? Cache files don't get created inside zips if there aren't any when the zip is created?

@ghost
Copy link
Author

ghost commented Nov 18, 2015

Correct. It checks for the cached dts.

@ghost
Copy link
Author

ghost commented Nov 20, 2015

I'll need to add this back in, no doubt.

@rasteron
Copy link
Contributor

Hey guys, got interested also in seeing this to work as it completes the process of packaging dts files in zip format. However, after patching a recent build with the files, I did notice that it does not cover the proper reading of the equivalent materials.cs file or particularly the equivalent compiled materials.cs.dso.

Thanks.

FileTime cachedModifyTime;
if (Platform::getFileTimes(cachedPath.getFullPath(), NULL, &cachedModifyTime))
{
bool forceLoadDAE = Con::getBoolVariable("$collada::forceLoadDAE", false);
Copy link

Choose a reason for hiding this comment

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

Hello! Your PR is good! Thanks for contribution.
You may want to restore this flag as it's used in the engine and as well from the editor to force the load of *.dae instead of cached *.dts.

Like:
if( Con::getBoolVariable("$collada::forceLoadDAE", false) )
return false;

Then we can merge.
Thanks.

@ghost
Copy link
Author

ghost commented Feb 27, 2016

@Lopuska didn't mean to leave that out. All good now.

@ghost
Copy link
Author

ghost commented Feb 27, 2016

@rasteron I haven't noticed any of those issues.

@ghost ghost added the Final review label Feb 27, 2016
@ghost ghost added this to the 3.9 milestone Feb 27, 2016
@ghost ghost self-assigned this Feb 27, 2016
@Alex-doc Alex-doc mentioned this pull request Mar 4, 2016
@ghost
Copy link
Author

ghost commented Mar 4, 2016

Dear @blackwc,
Me and @Alex-doc have decided to improve this in a different and more proper way:
Check #1540 and feel free to comment and give your opinion on that.

Closing this as we have a more consistent version.
Thanks for your help and contribution.

@ghost ghost closed this Mar 4, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants