-
-
Notifications
You must be signed in to change notification settings - Fork 957
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
Conversation
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. |
I am actually confused you are confused... My PR/commit title references |
So is it fixing that |
…ase_set>::AddFile provides fullpath
char *path = stredup(filename + basepath_length); | ||
ini->LoadFromDisk(path, BASESET_DIR); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
Ping? |
There was a problem hiding this 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.
This PR/commit title seems to suggest the opposite of the change? |
👍 1st PR ever merged 🎉 🎂 🎈 |
…e provided full path (OpenTTD#7348)
⚠Behaviour of
IniLoadFile::LoadFromDisk
to be confirmed⚠The following call is done with a full path:
OpenTTD/src/base_media_func.h
Line 162 in 7e7563f
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.