Fix: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath #7348

Berbe · 2019-03-09T01:41:42Z

⚠Behaviour of IniLoadFile::LoadFromDisk to be confirmed⚠

The following call is done with a full path:

Line 162 in 7e7563f

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 · 2019-03-09T10:09:29Z

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 · 2019-03-09T16:52:30Z

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 · 2019-03-09T16:58:38Z

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?

…ase_set>::AddFile provides fullpath

TrueBrain · 2019-03-09T17:50:07Z

src/base_media_func.h

 	char *path = stredup(filename + basepath_length);
+	ini->LoadFromDisk(path, BASESET_DIR);


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.

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

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

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

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 · 2019-04-09T04:18:11Z

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.

Berbe · 2019-04-10T00:56:05Z

Ping?

LordAro

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 · 2019-04-29T18:12:21Z

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

Berbe · 2019-05-02T15:13:28Z

👍 1st PR ever merged 🎉 🎂 🎈

…e provided full path (OpenTTD#7348)

LordAro previously approved these changes Mar 9, 2019

View reviewed changes

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

Berbe dismissed LordAro’s stale review via f24d41a March 9, 2019 17:34

Berbe force-pushed the base-media branch from 63a907a to f24d41a Compare March 9, 2019 17:34

TrueBrain reviewed Mar 9, 2019

View reviewed changes

stale bot added the stale label Apr 9, 2019

stale bot removed the stale label Apr 10, 2019

LordAro approved these changes Apr 27, 2019

View reviewed changes

PeterN merged commit 04c7435 into OpenTTD:master May 1, 2019

Berbe deleted the base-media branch May 2, 2019 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath #7348

Fix: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath #7348

Berbe commented Mar 9, 2019 •

edited

Loading

PeterN commented Mar 9, 2019 •

edited

Loading

Berbe commented Mar 9, 2019

PeterN commented Mar 9, 2019

TrueBrain Mar 9, 2019

TrueBrain Mar 9, 2019

TrueBrain Mar 9, 2019

Berbe Mar 9, 2019

Berbe Mar 10, 2019 •

edited

Loading

stale bot commented Apr 9, 2019

Berbe commented Apr 10, 2019

LordAro left a comment

PeterN commented Apr 29, 2019

Berbe commented May 2, 2019

		char *path = stredup(filename + basepath_length);
		ini->LoadFromDisk(path, BASESET_DIR);

Fix: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath #7348

Fix: IniLoadFile::LoadFromDisk seems to expect filename, BaseMedia<Tbase_set>::AddFile provides fullpath #7348

Conversation

Berbe commented Mar 9, 2019 • edited Loading

PeterN commented Mar 9, 2019 • edited Loading

Berbe commented Mar 9, 2019

PeterN commented Mar 9, 2019

TrueBrain Mar 9, 2019

Choose a reason for hiding this comment

TrueBrain Mar 9, 2019

Choose a reason for hiding this comment

TrueBrain Mar 9, 2019

Choose a reason for hiding this comment

Berbe Mar 9, 2019

Choose a reason for hiding this comment

Berbe Mar 10, 2019 • edited Loading

Choose a reason for hiding this comment

stale bot commented Apr 9, 2019

Berbe commented Apr 10, 2019

LordAro left a comment

Choose a reason for hiding this comment

PeterN commented Apr 29, 2019

Berbe commented May 2, 2019

Berbe commented Mar 9, 2019 •

edited

Loading

PeterN commented Mar 9, 2019 •

edited

Loading

Berbe Mar 10, 2019 •

edited

Loading