Fix abs paths for GetResourceStream

[DanRStevens]

Noticed a few issues with OP2MapImager related to other recent changes. I believe this sorts out those problems.

[Brett208]

What is the use case for passing an absolute path into the ResourceManager? Wouldn't someone just access the file directly instead of passing it into the resource manager if I know where it is at? In the case of an absolute directory we are bypassing searching through the archive files anyways.

I had no plan to support relative directories within archive files. There are implications for testing this feature with OP2Archive.

If you pass in a file with a relative directory, for example "test/eden01.map", which volume is searched? Does it look in test relative directory first and then search archives in test and then search archives in the root resource directory? Does the archive have to contain test/eden01.map as the filename to find it, or does it strip it to eden01.map?

I'm having trouble understanding the end goal here in archive and resource manager behaviour.

When you say issues, were there bugs when trying to find map files or tilesets for rendering?

-Brett

[Brett208]

I think we need a precendence listing for the ResourceManager if we want to expand scope to include directories in archives and subdirectories.

[DanRStevens]

Looking back at this, there are two changes in this changeset. One is being able to handle absolute paths, the other is not truncating paths before doing the lookup in the archives. I believe it's the later that actually bothers you.

---

The current bug I've encountered with OP2MapImager is that is passes the path given on the command line directly to ResourceManager. If that path is an absolute path, then this change allows ResourceManager to deal with that.

A second change, which I plan to make, is modifying OP2MapImager so it breaks the path into the resource folder, and the map/saved game filename separately, making the map/saved game filename relative. That would eliminate the immediate need for supporting absolute paths here, though also doesn't change the correctness of that part either. The absolute path code only affects raw file access, not archive file access.

---

The reason for supporting absolute paths though ResourceManager is having a single code path. That way client code doesn't need to check for absolute versus relative paths before deciding if it should open it directly, or pass it to ResourceManager to open it.

If we wish to disallow absolute paths, then we should probably start raising an exception if absolute paths are given.

---

The removal of path stripping is I think the more contentious part.

Within an archive, a file may actually be named "test/eden01.map". There is no separate "test/" directory component, it's all filename. Any concept of directories within an archive is an artificial construct. Remember, these are just strings. They are simple identifiers as far as the archives are concerned. They could have been hash strings, it doesn't really matter, so long as the strings form a unique id.

Granted, the strings do have some relation to an actual filesystem, since that's where the names come from, and where they may be extracted to. Hence, they look like filesystem filenames, but within the archive, those strings have simpler meaning. The implications of this are largely only relevant when generating an identifier string during packing of a VOL file, and when generating a filename during extraction from a VOL file.

In terms of where "test/eden01.map" is looked up, all loaded archives are searched for a file with that exact name string.

---

I had no plan to support relative directories within archive files.

This is actually something I've long considered. For the most part, it should just work by not getting in the way by stripping path components from filenames. One additional small requirement for extracting from archives to disk, is to pre-create the output path before opening the file for writing. That would mean calling GetDirectory on the filename from the archive, and ensuring that string of possibly nested directories is created first in the actual filesystem.

Archive files only support relative filenames. There is no concept of an absolute path to an archive file. At least none that we currently support. I suppose you could interpret a path such as "C:/Path/Archive.vol/InternalFile.ext" as an absolute path to an archive file, but we don't support that. We only support "InternalFile.ext", which is a relative path.

---

What I propose, is that we may additionally support a relative name with embedded slashes, such as "SomeString/InternalFile.ext". We currently don't support that, as the name gets stripped to "InternalFile.ext". Thus both "InternalFile.ext" and "SomeString/InternalFile.ext" will both try to open the same internal file "InternalFile.ext". I think this is undesired behavior.

Effectively what my change implies, is being more strict about what strings the archive search accepts. If they want access to "InternalFile.ext", then they must pass exactly that, and only that, to the search routine.

If code follows this more strict definition of what names ResourceManager accepts, then as the archives currently don't contain any subfolders, this change should have no real impact on existing code. However, I suspect that line was added to the code to work around some problems in the calling code that didn't adhere to such a strict definition of what names ResourceManager should accept. By removing that line, we expose those points of call and can address what they are doing.

---

In hindsight, perhaps the two changes should have been split into 2 pull requests.

[DanRStevens]

I'm reverting the change that strips the path before doing an archive search for now. Due to impact on other code, it's probably better to handle that in a separate branch.

[DanRStevens]

Ok, changed direction on this one a bit. I've decided to fix the path issue as an application level bug in OP2MapImager itself. I think OP2Utility should instead just outright reject absolute paths (or rather, non-fully relative paths).

I've added tests to ensure absolute paths are rejected. This uncovered a small header include bug.

[DanRStevens]

Upon further examination, with recently changes, only plain filenames with no path components should ever be fed in to ResourceManager::GetResourceStream.

The open pull request for OP2MapImager has updates to ensure only plain filenames are passed to GetResourceStream (directly or indirectly). This was part of the bug fix done over there. Once that is incorporated into master, OP2MapImager should not be impacted by changes to stripping of path components from filenames within the archives, as it will never pass a filename with path components.

In OPArchive, there are no calls to GetResourceStream (either directly or indirectly). So again, there is no impact for not stripping path components there.

Not stripping the path is a safe change, that should have no current impact.

---

In terms of what OP2Archive would do if it ever encountered an archive with path components built into the filename, that may be more of an application level concern, which so far has been left undefined. I don't think we need to worry about that at this time.

[DanRStevens]

After much consideration, I've decided to include the removal of path stripping change, which I had reverted earlier. It should have no impact on OP2Archive, and no impact on OP2MapImager after inclusion of recent changes. It should prevent incorrect/unexpected operation in some strange corner cases.

At this time, we still don't support subdirectories in archives. In particular, I have not examined or addressed packing/unpacking code. However, as a side effect of the more strict name checking, I believe in-memory stream access to files in subfolders within an archive would now work as expected (even if we don't support it). If we create such an archive with another external tool, or a future version of this tool, it should be usable in-memory with the current code base.

[DanRStevens]

Ok, I've tried to flesh out future plans for ResourceManager in Issue #265.