search

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Have ResourceManager append directory #235

Closed Brett208 closed 5 years ago

Brett208 commented 5 years ago

Resolves #232

Removing some of the functions might be a bit contentious. I think the goal is to not extract files with the ResourceManager. I think its goal is to locate files inside resources and provide memory streams to them.

-Brett

DanRStevens commented 5 years ago

I think you're right about having ResourceManager provide access to memory streams, and leaving extraction to files on disk as an external concern. When I stop and think about it, only an archive utility really needs to extract files, so maybe that part doesn't need to be in ResourceManager, or even OP2Utility. In particular, there is already sufficient support in terms of the Stream library component to easily extract a memory stream and dump its contents to a file on disk. It's basically a 1-2 line operation for a library user.

Brett208 commented 5 years ago

My experience seems to back up what you are saying. I originally used these 2 functions to extract resources in both OP2Archive and OP2MapImager. They were written before the stream code became robust. As the stream code matured, I switched away from using the 2 functions. So they were not used in either project anymore.

Brett208 commented 5 years ago

I'm ready to merge these changes. I know the use of XFile::ReplaceFilename isn't and ideal way to append a filename but I am content not putting the work in now to expand XFile.

DanRStevens commented 5 years ago

Ok, XFile can now Append paths. The second path is error checked to make sure it has no root components, which would not be properly recognized after a string concatenation.

DanRStevens commented 5 years ago

I checked into uses of FindContainingArchiveFile. It is only used in OP2Archive, and only to display the found path to the user. I think it's fine if this shows up as an absolute path. It won't break anything.

If we decide the output is too much, we can try to output a relative path instead. Such a path should be relative from the resourceRootDir. It should contain all path elements from that point onwards.

I think this is good to merge now.

DanRStevens commented 5 years ago

I just noticed this is still open. Not sure if you had a chance to review my changes, though I think this is good to merge now.

Brett208 commented 5 years ago

I finally got around to testing with OP2MapImager. There are some minor changes required in OP2MapImager to make it fully compatible, but the results seemed good.

I'm ready for this to get merged as well.

-Brett

Brett208 commented 5 years ago

I wanted to merge the master into this branch to incorporate other changes that would affect the tests.

I forgot to update the master before merging, so I merged only some of the new changes initially. I think that is what caused it. Sorry for the mess it made

Brett