EFF subdirectories #449
EFF subdirectories #449
Conversation
MageKing17
added
enhancement
There was a problem hiding this comment.
This macro is getting a bit too big, how about converting it to a static function?
There was a problem hiding this comment.
IIRC I didn't because it was the more complicated route and I didn't want to bundle potentially error-prone refactoring like that with the feature. It could certainly be done separately.
ln-zookeeper
force-pushed
the
eff_subdirs
branch
2 times, most recently
from
612a4d0 to
c7eeec0
Compare
|
The above feedback is now addressed as best I could. I ended up keeping the default value as an empty string literal instead of NULL because the latter would have then required adding a whole lot of NULL checks all over the place and that would have gotten messy and probably more bug-prone too. In the past year the added APNG support may have rendered this maybe not quite as vital as before, but I think it's still a worthwhile feature since it can be used with any image format and allows existing animations to be compartmentalized more easily than converting them to APNG. |
ln-zookeeper
force-pushed
the
eff_subdirs
branch
from
c7eeec0 to
c8fba6a
Compare
code/bmpman/bmpman.cpp
Outdated
| } | ||
|
|
||
| if (optional_string( "$Subdir:" )) { | ||
| stuff_boolean(&subdir);} |
There was a problem hiding this comment.
This brace is not aligned correctly.
code/bmpman/bmpman.cpp
Outdated
|
|
||
| #ifndef NDEBUG | ||
| if (strlen(fullpath) < strlen(fullrootpath)) { | ||
| mprintf(("BMPMAN: Path to %s was shorter than the root path, programmer thought it could never happen!\n%s\n%s\n", filename, fullpath, fullrootpath)); |
There was a problem hiding this comment.
If this should never happen then an Assertion might be the better choice here.
| } | ||
|
|
||
| // Init to an empty string to avoid having to test for NULL everywhere | ||
| Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path = vm_strdup(""); |
There was a problem hiding this comment.
This memory is leaked. This can be seen in the valgrind results of the unit tests.
code/bmpman/bmpman.cpp
Outdated
| * Example: if called with "debris.eff" and that file is in data/maps, then Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path | ||
| * will be stuffed with data/maps/debris, allowing data/maps/debris/debris_0000.dds (etc) to be found | ||
| */ | ||
| bool set_temp_subdir_pathtype(const char *filename) { |
There was a problem hiding this comment.
Could this function be expanded so that it would allow using a subdirectory in an arbitrary path type? This is currently tied to a specific file which means that it's very limited in what it can do.
There was a problem hiding this comment.
I'm not sure what exactly you're suggesting; as far as I can tell, there's not really any expanding to do and this should work as-is for any other files. Of course it will still be limited to anchoring the subdirectory to an existing filename (effects/foo.eff <-> effects/foo/) but that's not something I think I could get rid of.
If for some reason one needed to use it for non-bitmaps, it would be easy at that point to move the function to cfile and remove the references to BMPMAN from the output strings. However, to me that seems like a rather unlikely scenario.
There was a problem hiding this comment.
This was more of a "nice-to-have" request so it's not very important. However, I just noticed that this function is defined in bmpman.cpp and not in the CFile system. The functionality of this function should be present in the CFile system and not in bmpman since it changes stuff inside the CFile system.
|
Also, unit tests that check if this is actually working correctly would be nice. |
code/bmpman/bmpman.cpp
Outdated
|
|
||
| Bm_ignore_load_count = 0; | ||
|
|
||
| vm_free(Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path); |
There was a problem hiding this comment.
Shouldn't this be in cfile_close() since the initial vm_strdup() call is in cfile_init()?
There was a problem hiding this comment.
Also, an internal CFile field should not be manipulated in the bmpman code.
ln-zookeeper
force-pushed
the
eff_subdirs
branch
2 times, most recently
from
aae2020 to
780c605
Compare
|
Latest comments should again be addressed now. Except, of course, no unit tests since I have absolutely zero experience and knowledge about them. |
|
I can write the unit tests, it should be relatively easy to do. Have you tested this with VPs? I know that the cfile code does some indexing when loading a VP but I don't know if that includes all subdirectories or only the ones that are in the pathtypes. |
|
I have added unit tests for testing the function: ln-zookeeper#3 However, I discovered that the function does not support VP files properly. The reason for this is that |
ln-zookeeper
force-pushed
the
eff_subdirs
branch
2 times, most recently
from
27ea4ed to
624a4a7
Compare
|
Files from VP's should work correctly now. |
| SCP_string subdir, finalpath; | ||
|
|
||
| subdir = SCP_string(filename); | ||
| subdir = subdir.substr(0, subdir.find_last_of(".")); |
There was a problem hiding this comment.
If filename doesn't contain an extension then will find_last_of will return a value that is not valid for substr.
There was a problem hiding this comment.
I didn't specifically test it, but according to documentation it'll return string::npos meaning that substr will just return the complete string.
code/cfile/cfile.cpp
Outdated
| // (without extension) to it, giving us a relative path that can be temporarily | ||
| // stuffed into Pathtypes | ||
| for (size_t i=CF_TYPE_ROOT; i<CF_MAX_PATH_TYPES; i++) { | ||
| size_t size, offset; // Needed for cf_find_file_location, but not used |
There was a problem hiding this comment.
Er... if you don't need the size or the offset, you can just pass null pointers, like you already are for pack_filename.
There was a problem hiding this comment.
You're right. No idea why I had originally done it that way.
ln-zookeeper
force-pushed
the
eff_subdirs
branch
from
624a4a7 to
6a502eb
Compare
This allows animation frames of eff animations to be located in a subdirectory. This is useful for de-cluttering the effects directory in particular.
ln-zookeeper
force-pushed
the
eff_subdirs
branch
from
6a502eb to
62f1d5c
Compare
|
Given that we have APNGs, should this PR be closed? |
I would say yes? |
|
Are there not still performance advantages to using EFFs of DDS files vs APNGs? As long as there are reasons to still use EFFs going forward, and therefore a use for this feature, it would still be nice to see merged at some point, I would think. |
Ah that's a good point, especially since DDS images can be mipmapped and loaded faster. |
|
Closing now that #4771 is merged. |
5 participants
This allows animation frames of eff animations to be located in a subdirectory. This is useful for de-cluttering the effects directory in particular.
Usage and limitations as per http://www.hard-light.net/forums/index.php?topic=88139.0
Before merging, this needs testing in different mod environments: VP's, multiple mods, replacing anims from retail or other mods, and so on. I've pretty much only tested this in a development setting, as in a single mod as a bare directory, no VP's.
Notes:
I'm not sure about the original rationale behind the changes to the bm_unlock()/bm_unload() calls in bmpman.cpp; I'm sure they were intended as bugfixes, but I don't recall the specifics. I included them now because they've been like this in FotG for a long time without problems, but I'll have to split those into a separate commit or remove entirely before merging.