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: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath #7348

Merged
merged 1 commit into from May 1, 2019

Conversation

@Berbe
Copy link
Contributor

Berbe commented Mar 9, 2019

⚠Behaviour of IniLoadFile::LoadFromDisk to be confirmed⚠

The following call is done with a full path:

ini->LoadFromDisk(filename, BASESET_DIR);

Since that function adds search paths to the provided filename, it results in unecessary failed file searches, eventually falling back to the original, which is then found and processed, as seen in the attached filtered output.
That behaviour wastes processing time/disk filesystem queries and could potentially hurt file discovery.

Fix merely moves up an existing instruction already transforming a full path in a file name.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Mar 9, 2019

I'm confused by the commit title and the commit content being seemingly unrelated.

What are you fixing here? From the title it suggests you are fixing BaseMedia::AddFile, but from the commit you are fixing the ini file loader.

@Berbe

This comment has been minimized.

Copy link
Contributor Author

Berbe commented Mar 9, 2019

I am actually confused you are confused...

My PR/commit title references BaseMedia<Tbase_set>::AddFile, I quoted a line of BaseMedia<Tbase_set>::AddFile, and the commit shows a line of BaseMedia<Tbase_set>::AddFile, modifying a call to LoadFromDisk.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Mar 9, 2019

So is it fixing that AddFile searches for full paths instead of files (i.e. changes it search for files), or is the fix to make it search for full paths instead of files?

@Berbe Berbe changed the title Fix: BaseMedia<Tbase_set>::AddFile searches for fullpaths instead of files Fix: BaseMedia<Tbase_set>::AddFile wrongly searches for fullpath instead of filename Mar 9, 2019
@Berbe Berbe changed the title Fix: BaseMedia<Tbase_set>::AddFile wrongly searches for fullpath instead of filename Fix: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath Mar 9, 2019
…ase_set>::AddFile provides fullpath
@Berbe Berbe force-pushed the Berbe:base-media branch from 63a907a to f24d41a Mar 9, 2019
char *path = stredup(filename + basepath_length);
ini->LoadFromDisk(path, BASESET_DIR);

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Mar 9, 2019

Member

I think the old place was correct, but it should be NO_DIRECTORY as second argument. But .. this code is difficult to follow, and it seems a hack on a hack.

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Mar 9, 2019

Member

From what I can see, ScanPath is ran on BASESET_DIR. Every hit it calls AddFile, with 'filename' the full path, and 'basepath_length' with the offset of the searchpath found. Next LoadFromDisk is called on the filename, and using BASESET_DIR here causes it to search the searchpaths again. But AddFile already knows where the hit is. At least .. this is my (limited) understanding :D

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Mar 9, 2019

Member

Another look might suggest that my suggestion kills TAR support. Not sure. Code is weird.

This comment has been minimized.

Copy link
@Berbe

Berbe Mar 9, 2019

Author Contributor

Using NO_DIRECTORY there breaks another file discovery:

[5528] dbg: [console] /home/<user>/source/src/fileio.cpp:1378 ScanPath Valid file: /home/<user>/.openttd/content_download/baseset/
[5528] dbg: [grf] /home/<user>/source/src/base_media_func.h:158 AddFile Checking opengfx-0.5.2/opengfx.obg for base graphics set
[5528] dbg: [console] /home/<user>/source/src/fileio.cpp:421 FioFOpenFileSp Trying to load opengfx-0.5.2/opengfx.obg
[5528] dbg: [console] /home/<user>/source/src/base_media_func.h:165 AddFile path = opengfx-0.5.2/opengfx.obg
[5528] dbg: [console] /home/<user>/source/src/base_media_func.h:172 AddFile path = opengfx-0.5.2/
[5528] dbg: [grf] /home/<user>/source/src/base_media_func.h:46 FillSetDetails Base graphicsset detail loading: name field missing.
[5528] dbg: [grf] /home/<user>/source/src/base_media_func.h:46 FillSetDetails   Is opengfx-0.5.2/opengfx.obg readable for the user running OpenTTD?

Normal behaviour (trunk) is:

[5781] dbg: [console] /home/<user>/source/src/fileio.cpp:1378 ScanPath Valid file: /home/<user>/.openttd/content_download/baseset/
[5781] dbg: [grf] /home/<user>/source/src/base_media_func.h:158 AddFile Checking opengfx-0.5.2/opengfx.obg for base graphics set
[5781] dbg: [console] /home/<user>/source/src/fileio.cpp:421 FioFOpenFileSp Trying to load /home/<user>/source/baseset/opengfx-0.5.2/opengfx.obg
[5781] dbg: [console] /home/<user>/source/src/fileio.cpp:421 FioFOpenFileSp Trying to load /home/<user>/.openttd/baseset/opengfx-0.5.2/opengfx.obg
[5781] dbg: [console] /home/<user>/source/src/fileio.cpp:421 FioFOpenFileSp Trying to load /home/<user>/source/bin/baseset/opengfx-0.5.2/opengfx.obg
[5781] dbg: [console] /home/<user>/source/src/fileio.cpp:421 FioFOpenFileSp Trying to load /usr/local/share/games/openttd/baseset/opengfx-0.5.2/opengfx.obg
[5781] dbg: [console] /home/<user>/source/src/fileio.cpp:421 FioFOpenFileSp Trying to load /home/<user>/.openttd/content_download/baseset/opengfx-0.5.2/opengfx.obg
[5781] dbg: [console] /home/<user>/source/src/base_media_func.h:165 AddFile path = opengfx-0.5.2/opengfx.obg
[5781] dbg: [console] /home/<user>/source/src/base_media_func.h:172 AddFile path = opengfx-0.5.2/

What's bothering with that AddFile behaviour is that, even though a specific file is found in ScanPath, it triggers yet another file discovery in all of the basesets directories instead of merely loading from a subdirectory of the found file (for cases like this one where the file is an archive, seen as a subdirectory by the loading system).

However, all that seems to go beyong that quick & dirty fix, calling for a global rewrite...

This comment has been minimized.

Copy link
@Berbe

Berbe Mar 10, 2019

Author Contributor

IniLoadFile::LoadFromDisk then calls IniFile::OpenFile, which is a mere proxy to FioFOpenFile.
This last function seems thus to be central in all file loading operations as it returns a file pointer on discovery success.

While this function handles cases were full paths are provided to it, as I pointed out it was working before (look towards its end), it's definitely inefficient to do so.

Might even be a good idea to rewrite this function so that full paths are detected early in it?
In doing so, rather than blindly attempting to load resources, checking if the beginning of a filename matches any of the registered search paths might trigger a direct attempt to load & resource by fullpath?

And of course, I am not even talking about the quality of the code in base_media_func.h, which is more of your league.

@stale

This comment has been minimized.

Copy link

stale bot commented Apr 9, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Apr 9, 2019
@Berbe

This comment has been minimized.

Copy link
Contributor Author

Berbe commented Apr 10, 2019

Ping?

@stale stale bot removed the stale label Apr 10, 2019
Copy link
Member

LordAro left a comment

I'm pretty sure this is correct behaviour - all the paths that are passed in as absolute paths are those outside tar files, and they have basepath_length set correctly. Other paths (those inside tar files) are already relative, with basepath_length set to 0.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Apr 29, 2019

This PR/commit title seems to suggest the opposite of the change?

@PeterN PeterN merged commit 04c7435 into OpenTTD:master May 1, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190309.16 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@Berbe Berbe deleted the Berbe:base-media branch May 2, 2019
@Berbe

This comment has been minimized.

Copy link
Contributor Author

Berbe commented May 2, 2019

👍 1st PR ever merged 🎉 🎂 🎈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.