feat(filesystem): Implement file and directory ignore functionality for local and archive file systems - #3019
feat(filesystem): Implement file and directory ignore functionality for local and archive file systems#3019xezon wants to merge 10 commits into
Conversation
825df11 to
1f10476
Compare
|
@greptileai review this |
|
| Filename | Overview |
|---|---|
| Core/GameEngineDevice/Source/StdDevice/Common/StdLocalFileSystem.cpp | Applies ignore checks to standard filesystem operations and safely advances directory iterators through ignored entries. |
| Core/GameEngineDevice/Source/Win32Device/Common/Win32LocalFileSystem.cpp | Applies ignore checks to Win32 filesystem operations and safely advances FindNextFile through ignored entries. |
| Core/GameEngine/Source/Common/System/AsciiString.cpp | Corrects the nextToken alias assertion and clears the output token when input is empty. |
| Core/GameEngine/Source/Common/System/UnicodeString.cpp | Corrects the nextToken alias assertion and simplifies delimiter handling without breaking identified callers. |
| Core/GameEngine/Source/Common/System/ArchiveFile.cpp | Adds path-indexed archive lookup and recursive ignore state for archived files and directories. |
| Core/GameEngine/Source/Common/INI/INIFileSystem.cpp | Parses FileSystemIgnore definitions and dispatches file or directory ignores to the filesystem layer. |
| GeneralsMD/Code/GameEngine/Source/Common/GameEngine.cpp | Loads the optional filesystem-ignore configuration early in engine initialization. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
INI[Data/INI/FileSystem configuration] --> Parser[FileSystemIgnore parser]
Parser --> FS[FileSystem ignore API]
FS --> Local[Local file system]
FS --> Archive[Archive file system]
Local --> LocalOps[Open / existence / metadata / enumeration]
Archive --> ArchiveOps[Archive lookup / metadata / enumeration]
LocalOps --> Hidden[Ignored entries are hidden]
ArchiveOps --> Hidden
Reviews (3): Last reviewed commit: "feat(filesystem): Implement file and dir..." | Re-trigger Greptile
1f10476 to
7c8ee53
Compare
…tring::nextToken() (#3019)
…hen no file was loaded (#3019)
…ileSystem, StdLocalFileSystem (#3019)
…m::openArchiveFile() (#3019)
…o, ignore case & slash in FileSystem::FileExistMap (#3019)
…, StdBIGFile::getFileInfo() (#3019)
…or the local and archive file systems (#3019) A new optional Data/INI/FileSystem.ini is loaded early and can be configured with FileSystemIgnore definitions: FileSystemIgnore Name="Directory/File.ext" Type=FILE ; or DIR End
7c8ee53 to
4f62dc9
Compare
| FastCriticalSectionClass::LockClass lock(m_ignoreFileMutex); | ||
| m_ignoreFileHashMap.insert(std::make_pair(filename, IgnoreFileData())); | ||
| } | ||
| else if (!m_ignoreFileHashMap.empty()) |
There was a problem hiding this comment.
The empty check is currently not mutexed
| while (it != m_ignoreFileHashMap.end()) | ||
| { | ||
| IgnoreFileHashMap::const_iterator curr = it++; | ||
| if (startsWithPath(curr->first.c_str(), directory.str())) |
There was a problem hiding this comment.
What happens when two files or directories start with the same name, is there a risk that it will match the wrong item?
There was a problem hiding this comment.
Can you give example? The paths given are expected to be full paths from game root directory. I can document it.
There was a problem hiding this comment.
I haven't tested it, but for example these directories:
MyGameData/Foo
MyGameData/FooBar
There was a problem hiding this comment.
Good point. I have not considered this. I will fix it.
|
If a mod relies on FileSystem.ini to mask files, running that mod on an unmodified/legacy engine executable will silently break because the legacy client ignores FileSystem.ini and loads the masked files anyway. Instead of introducing custom C++ filesystem-ignore logic, wouldn't it be much cleaner to handle this in the BIG tooling / build pipeline? |
| ca = toLowerAscii(ca); | ||
| cb = toLowerAscii(cb); | ||
|
|
||
| if (ca != cb) |
There was a problem hiding this comment.
rts::less_than_path is not a strict weak ordering, because comparePath compares a raw separator against a non-separator instead of a canonicalized one.
comparePath("a/b", "a1b") < 0 // '/' 0x2F vs '1' 0x31
comparePath("a1b", "a\b") < 0 // '1' 0x31 vs '' 0x5C
comparePath("a/b", "a\b") == 0 // both separators, skipped
So X < Y < Z while X == Z. The same holds for trailing separators: "a" < "a1" < "a" while "a" == "a".
The only user is FilenameList, which is a std::set, so this is undefined behavior rather than just an odd sort order. It is also a regression, as the previous less_than_nocase was a valid ordering. Windows builds are unlikely to hit it since local and archive paths both use , but StdLocalFileSystem produces / on other platforms, so a merged list can hold both forms.
Canonicalizing each separator before the character comparison fixes the first case but not the second, since the trailing-separator skip only runs after the loop exits. Both need comparePath to be a real normalize-then-compare: strip trailing separators up front to get end pointers, as hashPath already does, compare canonicalized characters over those ranges, then compare the remaining lengths.
hash_path and equal_to_path are unaffected — path equality is already transitive. Only the ordering needs to change.
There was a problem hiding this comment.
What is the suggested fix from the LLM?
There was a problem hiding this comment.
Also can someone translate the LLM speak to human speak? This error description is very confusing. Unclear to me if it hallucinates. Assuming it is LLM generated.
There was a problem hiding this comment.
Yeah it's not an easy one to make sound human, it's in the weeds. It's not a hallucination though.
slash and backslash are treated as equal when compared directly, but they receive different ordering positions when compared against another character. That makes the result inconsistent.
I verified with std::set: inserting a/b, a1b, and a\b gives either 2 or 3 elements depending on insertion order.
The fix is to strip trailing separators first, normalize every remaining / or \ to the same character, and then compare the normalized ranges lexicographically (who doesn't use this word regularly?).
The technical fix is to replace only comparePath() in Core/Libraries/Include/Lib/PathUtil.h:
inline int comparePath(const char* a, const char* b)
{
- while (*a && *b)
+ const char* endA = a + strlen(a);
+ const char* endB = b + strlen(b);
+
+ // Ignore trailing separators before comparing.
+ while (endA > a && isPathSeparator(static_cast<unsigned char>(endA[-1])))
{
- unsigned char ca = static_cast<unsigned char>(*a);
- unsigned char cb = static_cast<unsigned char>(*b);
+ --endA;
+ }
- if (isPathSeparator(ca) && isPathSeparator(cb))
- {
- ++a;
- ++b;
- continue;
- }
+ while (endB > b && isPathSeparator(static_cast<unsigned char>(endB[-1])))
+ {
+ --endB;
+ }
- ca = toLowerAscii(ca);
- cb = toLowerAscii(cb);
+ while (a != endA && b != endB)
+ {
+ unsigned char ca = static_cast<unsigned char>(*a);
+ unsigned char cb = static_cast<unsigned char>(*b);
+
+ if (isPathSeparator(ca))
+ {
+ ca = '\\';
+ }
+ else
+ {
+ ca = toLowerAscii(ca);
+ }
+
+ if (isPathSeparator(cb))
+ {
+ cb = '\\';
+ }
+ else
+ {
+ cb = toLowerAscii(cb);
+ }
if (ca != cb)
+ {
return ca - cb;
+ }
++a;
++b;
}
- // Ignore trailing separators
- while (isPathSeparator(*a))
- ++a;
- while (isPathSeparator(*b))
- ++b;
+ if (a != endA)
+ {
+ return 1;
+ }
- return static_cast<unsigned char>(*a) - static_cast<unsigned char>(*b);
+ if (b != endB)
+ {
+ return -1;
+ }
+
+ return 0;
}
| } | ||
|
|
||
| dirInfo->m_files[fileInfo->m_filename] = *fileInfo; | ||
| typedef std::pair<ArchivedFileInfoMap::iterator, bool> Result; |
There was a problem hiding this comment.
This changes duplicate archive paths from last-entry-wins (operator[] assignment) to first-entry-wins (insert). Even if duplicate paths are malformed or unusual, this changes which entry is opened.
Should the previous behavior be preserved, or should duplicates be explicitly detected and rejected/logged?
There was a problem hiding this comment.
I expect it never adds a duplicate entry to begin with. I can restore the old behavior but it is probably inconsequential in practice.
|
I agree with @githubawn here, this seems like needless complication. Instead if you want to control load order of big files and exclude some, implement a manifest mode for loading big files where a mod supplied a config file that states explicitly which archive files to load. Later SAGE games implement this functionality. To prevent loading files from other archives, you could supply a blank file in a higher precedence big file to effectively dummy it out. |
|
Moving toward an explicit archive manifest system like Tiberium Wars is definitely the cleaner long-term path. However, I think the key is using external sidecar .manifest files placed next to the archives on disk. The main benefit is legacy mod compatibility. Many classic Zero Hour mods exist as standalone .big archives whose original authors are gone. External .manifest files allow us to define load orders, metadata, override rules, and even automated extraction instructions for installer .exes (like Shockwave) for those classic archives without modifying the original .big file. |
Yes the ignore solution is indeed not so great. I already agreed with Stubbjax on another solution where we allow to specifiy the INI load path. Then we do not need to ignore files. |
Merge with Rebase
This change implements a new feature that allows to ignore files and directories for the local and archive file systems. This pull also contains a number of commits that refactor, simplify and fix relevant code to reach that goal.
This feature is useful for Mods that need to ignore specific game files and/or directories, for example prevent some INI files from loading. It is needed for GeneralsGamePatch2.
On Engine init, a new optional
Data/INI/FileSystem.ini(and/or INI files insideData/INI/FileSystem) is loaded early and can be configured withFileSystemIgnoredefinitions:It is technically also possible to lift file and directory ignore on runtime, but that remains unused.
The file ignore implementation has been optimized for efficiency and simplicity.
Further thoughts
If we could rely on user supplied command line arguments, then simpler implementations would be possible, such as preventing the Archive File System from loading an archive file or contents inside of it. But because we want to load the ignore configuration from inside an INI file, which can also be inside an Archive, it needs to load the Archive File System normally.
TODO