add support for subfolders to cfile #4771
Conversation
|
This has yet to be tested on Windows and may have unintended side effects for FRED2 as well! Leaving as draft until sufficient testing and feedback has been done. To take advantage of this feature you just need to put files in folders. Any subfolder of an existing cfile path type is treated as if it was directly in that base path. For instance:
could instead be organized like:
or like:
and everything would just work. Models, maps, sounds, tables, scripts, ... can all be organized the same way: however you like! Scripts also now support requiring other scripts directly out of subfolders too. This allows you to have scripts with the same name, organized in different subfolders, and load the specific script that you want. For instance, you have a
and use |
|
Known quirks/issues:
For the sort order issue, an example would be where you have a Essentially, this file structure as seen in a directory listing:
will actually be indexed in the following order: |
|
Testing and feedback would be appreciated from @Goober5000, @EatThePath, and @wookieejedi |
notimaginative
added
the
enhancement
They aren't directly referenced anywhere by pathtype and the new subfolder code will continue to index and load files from them
An alternative is left in place for old fonts, as that was the only place it appeared to be used. This method of file localization conflicts with subfolder support however so if it's needed in the future then changes to the subfolder code will be necessary.
|
Hopefully will be able to test soon, but some comments from reading the description: Just to check my understanding... In that last case, would both |
|
For your first example, that's correct. Right now the list functions have no concept of subfolders so as far as that code goes those two files are just the same thing in different roots and the second one is filtered out. You would have the same problem if they were in the same mod too, although the indexing order would come in to play then. The plan is to modify those list functions to allow something like looking for For your second example, that's also correct. You would be able to directly reference So you wouldn't want to use the same file names for texture maps or effects just because they are in subfolders, for example. When opening files like that it only searches for the file name and doesn't take folders into account. So having |
|
sounds fairly good all around. It might be useful for me to have the lua behavior apply to config files too, but not crucial. Being able to list with a subfolder path might be more important, but the lack of it wouldn't be a merge blocker to my mind. Some of the motivation behind #3391 is me chafing under the 31 character limit, and if I'm making a lot of individual tables I get the desire to not type those four obligatory characters every time. But those are relatively minor things, and the bigger organizational goals of that FR would be served by this arrangement. I wouldn't object to that issue being closed with this. I'll try to give this some practical testing. Now that I've refreshed my memory I definitely have some use cases to try. |
|
Both If it doesn't work let me know and I'll make sure to get it fixed. Subfolders in file lists is just a convoluted process but I do intend to get it working before this is merged. For other files like textures, effects, and sounds, that code does technically support subfolders but there isn't really a good way to modify the code to use them. I suppose that including a path with the filename in a table entry or model to load textures/effects might work, but I haven't tried it. I should also note that in all of this new code I'm removing the 31 character limit as I go. VP files will still have the limit, and I can't fix that without a new archive format, but at the cfile level at least the limit is largely gone. We just need to start converting other references in the code to use SCP_string instead of a fixed length. |
Handy! To be specific about my goals. I want to be able to get a list of list all of the config files in
Good to hear! |
Gotcha. That should be doable once I get subfolders working in the list functions (probably next weekend). |
|
Got it partially working with lists now and I'll finish it up over the next week as time allows. It has three different search modes:
|
|
those search methods seem to cover the bases at a glance, if I think of any issues I'll let you know. I've reorganized dev warmachine to heavily use this and it works best I can tell. I'm around 10 fps on the machine I currently have access to, so I can't stresstest it much, but it launches without evident issue. One thought: It seems extra easy now to accidentally duplicate a filename in this system. It would probably be useful to have a warning if a file 'shadows' another file in the same root. |
Yeah, that's an issue I haven't figured out a good way to deal with yet. It's a primary issue with file lists as a list with both I'll be thinking on it this week, so maybe I'll have a solution by the weekend. |
|
I was thinking more like your example of a |
|
I'm hesitant to do that since the performance hit could vary quite wildly and we'd surely get complaints. But it is possible and we could limit it to just debug builds. Maybe I'll try adding something and we can just experiment with it a bit. As an alternative, what do you think about a comprehensive check via cmdline option? Sort of like |
|
I've gotten most of the kinks worked out on lists and so far all of my tests are getting predictable results. I'm going to sit on it another day or two before pushing just in case additional tweaks are required. It's going to take someone else having a go at it to give things a proper test. The two versions of The big problem is shadowing, and I'm just not sure how to deal with it. For testing I have a mod with a VP containing But as that test shows, warning about shadowed files will need to take all roots into account to really be effective. |
|
I'll try to get more testing of a reorganized mod in this weekend, the situation keeping me from it is now resolved. When it comes to warnings, I think it's probably something that this feature can be merged without, we can always work more warnings in later and I think the game is better off with subfolders and no new warnings than it is without subfolders. |
|
I have worked up a simple shadow detection check which might do the trick. Two issues are that it can't work as a It's easy enough to just print it to the debug log to deal with issue one. That will make the log a bit messy though. The windowing issue is that, during this stage of the init process, creating a Warning() dialog appears to prevent the main game window from being created. At least on Linux. I'm a little concerned with the false positives though. It triggers on possible shadowing, but it's the actual usage which determines whether it's a problem. So having this check default to enabled may be a huge nuisance if there are more than a couple of shadowed files. |
|
Test for shadowed files added with a warning to the debug log. Give it a try and let me know if you think that is good enough or not. But again, the false positives once the subfolder setup is fully in use may just annoy people. I made sure that it's easy enough to disable though. |
|
I pulled that and gave it a spin and my dev copy of warmachine has 270 shadowing warnings. Looks like about half of them are because BP actually has a subfolder in their voice/personas folder with duplicated files, so those are "real" shadowings, but the rest look to be intentional/desirable shadowings. |
|
Hit this one by going to the tech room mission simulator. ASSERTION: "rc == Num_campaigns" at missioncampaign.cpp:312 Assert: "rc == Num_campaigns" ntdll.dll! NtWaitForSingleObject + 20 bytes |
Also fixes discrepancy in older cf_get_file_list() when a sub path is specified (should be used in all cases, not just glob).
|
Nothing to be done about the BP issue from a code perspective. And I think there are some similar cases with other mods as well. It's just going to have to be dealt with on the content side. Sim room bug should be fixed now. A misguided attempt to optimize things messed it up. |
|
I haven't found any other issues in my own testing, or further areas of improvement. Setting this as ready for review and hopefully some additional testing will be done by others as well. |
BMagnu
left a comment
BMagnu
left a comment
There was a problem hiding this comment.
Looks mostly good, especially with all the apparent testing that's already happened.
Some questions and comments
|
I need to circle back to this and retest the newest version, I've been wrapped up in my own branches a lot.
I agree with that for sure. So long as it's a log print and not a full Warning, the shadowing messages seem appropriate. I'm tempted to make a new default-on "load order" filter category, but that can be a followup if it feels appropriate once people get some experince with the messages. |
|
Per Discord Discussion: |
4 participants
Allow indexing of files in subfolders of known cfile path types as if the files in those subfolders were directly part of the known path. Essentially a cosmetic change allowing mod makers to organize their files in a better way without requiring any direct content changes or special code handling to take advantage of the feature.