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 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.

LordAro
LordAro previously approved these changes Mar 9, 2019
@PeterN
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
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
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
char *path = stredup(filename + basepath_length);
ini->LoadFromDisk(path, BASESET_DIR);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@Berbe Berbe Mar 10, 2019

Choose a reason for hiding this comment

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

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
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 Stale issues label Apr 9, 2019
@Berbe
Copy link
Contributor Author

Berbe commented Apr 10, 2019

Ping?

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

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

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
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
@Berbe Berbe deleted the base-media branch May 2, 2019 15:10
@Berbe
Copy link
Contributor Author

Berbe commented May 2, 2019

👍 1st PR ever merged 🎉 🎂 🎈

douiwby pushed a commit to douiwby/OpenTTD that referenced this pull request Apr 16, 2020
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