OP2MapImager Development

[Hooman]

Nice job with the documentation.

We'll have to get the custom OP2-format BMP loading code working, as well as loading directly from VOL files. I consider the direct VOL loading to be higher priority, since you'd need to read the VOL files before getting access to the custom format BMP files stored inside. Of course that could just be me procrastinating on touching the messy custom format BMP loading code.

The release build steps seem long and manual. Perhaps we should script that. Maybe we could create a new project in the solution space for building the release package. It could depend on the project, so building it would automatically rebuild the project, and fail if the project failed to build. Maybe throw the commands into the post build step to copy files to the right folder and zip them up. I don't know how you want to handle version numbers, though it would be easy enough to append a date to the archive name.

[Vagabond]

Hooman,

I spent some time learning more about post build events in Visual Studio. Below is the result. Now Visual Studio will check if the code was built in release mode, and if so create a folder called 'OP2MapImager X.X' inside the release folder. Then it populates the new folder with all the files required for the build.

The user will still have to manually change the version number in the code, folder name, and ReadMe before zipping the folder. So, I guess it eliminates some of manual labor.

Thank you for the reply in the other topic on expanding VolDecompress. I haven't had time to implement it yet, but should be next on the list to look at. I would prefer moving away from the WIN API code, but I'm not sure it makes sense for me to just rewrite a smattering of the functions this way and leave the rest with WIN API and COM interoperability.

x86 vs x64 compatibility
I think the WIN API code used in OP2VolDecompress is not compatible with x64 builds. I can only seem to get it to compile in x86.

Code: [Select]

xcopy /y /d "$(ProjectDir)FreeImage\FreeImage.dll" "$(OutDir)"

if $(ConfigurationName) == Release (
    mkdir "$(OutDir)OP2MapImager X.X"
    xcopy /y /d "$(ProjectDir)well00*.bmp" "$(OutDir)OP2MapImager X.X"
    xcopy /y /d "$(ProjectDir)FreeImage license-gplv3.txt" "$(OutDir)OP2MapImager X.X"
    xcopy /y /d "$(ProjectDir)ReadMe.txt" "$(OutDir)OP2MapImager X.X"
    xcopy /y /d "$(OutDir)FreeImage.dll" "$(OutDir)OP2MapImager X.X"
    xcopy /y /d "$(OutDir)$(TargetFileName)" "$(OutDir)OP2MapImager X.X"
)

[Hooman]

For the X.X, perhaps your script can set a shell variable using the "svn info" command. The command on it's own will list a number of values, but you can select one specifically with the --show-item argument. I would recommend experimenting with the "last-changed-revision" value. You could tag each release with the SVN revision number. I believe by setting the path/url to the root of your project, it will include the most recent revision change to that project (ignoring more recent repository changes outside of your project). Of course if you depend on another project, it might be easier to just use "revision" (the global value). Another possibly useful value is "last-changed-date".

Code: [Select]

info: Display information about a local or remote item.
usage: info [TARGET[@REV]...]

  Print information about each TARGET (default: '.').
  TARGET may be either a working-copy path or URL.  If specified, REV
  determines in which revision the target is first looked up.

  With --show-item, print only the value of one item of information
  about TARGET. One of the following items can be selected:
     kind                  the kind of TARGET
     url                   the URL of TARGET in the repository
     relative-url          the repository-relative URL
     repos-root-url        the repository root URL
     repos-uuid            the repository UUID
     revision              the revision of TARGET (defaults to BASE
                           for working copy paths and HEAD for URLs)
     last-changed-revision the most recent revision in which TARGET
                           was changed
     last-changed-date     the date of the last-changed revision
     last-changed-author   the author of the last-changed revision
     wc-root               the root of TARGET's working copy

...

Once you have some kind of version tag, I would also recommend zipping the package as part of the build step.


If the release package preparation is slow, it may be nice to have it separated out. Looks like you're only doing it for release mode, which should be good enough. If later you decide to add testing, which might also be done in release mode, the packaging could be move to a later step. That's why I suggested a separate release package project that depends on the actual project. The actual project would still be the default project for builds, so the release package build would only run when requested, and project dependency rules would ensure the actual build was also up-to-date when it runs. Not sure if this makes sense to do; just wanted to be clear on the idea.


The Win API code does have differences between 32-bit and 64-bit. Mostly though, it uses auto-expanding type names, so they're the appropriate type on each platform. Depending on how things are coded, it can often be source compatible between the two, though not likely to be binary compatible.

You shouldn't need to use the Win API. You can depend on the standard C++ library. It's mostly just file access code that depends on the Win API, which there is reasonably straightforward replacements to in the standard C++ library.

I didn't see an easy way to do binary memory stream in the standard C++ library. It may make sense to still use your own StreamReader classes. A custom FileStreamReader class can just delegate to standard C++ library file access classes. A custom MemorySreamReader class can just be built in raw C++ without any standard library or operating specific code.

The existing code for OP2Editor used the Win API for the FileStreamReader. The MemoryStreamReader was simple C++ code without use of libraries or the Win API. The code was quite simple in both cases.

[Vagabond]

Another Update,

I spent some time learning more about the command prompt and writing batch programs, which is essentially what Visual Studio C++ Post Build Events are.

There is not a built in zip console command in Windows (strange I thought), so I settled on 7-Zip's console application. 7Zip is a fairly well known, free, and easy to use alternative. I placed 7za.exe in the repository, which means that others will be able to run the post build event without having to fetch the application manually (if they are running windows).

I had to download the console application portion of TortoiseSVN. Unfortunately, it does not default to installing the console application when you install TortoiseSVN. So, if others pull the source code, the post build script will blow up until they do the same if not already installed. Perhaps there is a way to place the executable in folder like I did with 7-zip?

The code below is the full post build script. Basically it

• saves the filename and directory to a local variable
• creates a directory for all release files
• copies all required files to the release directory
• zips the release directory using 7-zip (I'm using the standard zip format, not 7-zip's arguably better format)
• deletes the initially created directory

Code: [Select]

xcopy /y /d "$(ProjectDir)FreeImage\FreeImage.dll" "$(OutDir)"

REM SET zipName="OP2MapImager Ver1.0" svn info --show-item revision --no-newline
SET zipName="OP2MapImager Ver1.0"
SET directoryName=$(OutDir)%25zipName%25 

if $(ConfigurationName) == Release (
    mkdir %25directoryName%25
    xcopy /y /d "$(ProjectDir)well00*.bmp" %25directoryName%25
    xcopy /y /d "$(ProjectDir)FreeImage license-gplv3.txt" %25directoryName%25
    xcopy /y /d "$(ProjectDir)ReadMe.txt" %25directoryName%25
    xcopy /y /d "$(OutDir)FreeImage.dll" %25directoryName%25
    xcopy /y /d "$(OutDir)$(TargetFileName)" %25directoryName%25

    7za a -tzip $(OutDir)%25zipName%25.zip %25directoryName%25
    REM DEL /q %25directoryName%25\*.*
    RMDIR %25directoryName%25 /s /q
)

The only piece I need help on is setting the revision number from SVN. I cannot figure out how to set the contents of a local variable as a concatenation of a string and the output of the SVN console command.

Successfully working SVN console command:

Code: [Select]

svn info --show-item revision --no-newline

I want it to read something like:

Code: [Select]

SET zipName="OP2MapImager Ver1.0" svn info --show-item revision --no-newline

@Hooman,

Thanks for the reply in the other thread on vol decompression. Now that I'm done fiddling with the post build event (besides the issue noted), I'll head back over and try to implement the tips you gave.

[Hooman]

This may be relevant:
https://stackoverflow.com/questions/17546016/how-can-you-zip-or-unzip-from-the-command-prompt-using-only-windows-built-in-ca
It seems there is a way to compress files to ZIP using Power Shell with newer versions of Windows. Power Shell can be called from a regular shell or batch file. I noticed an alternate methods towards the end that may work for older versions of Windows.

For the variable, this might help:
https://stackoverflow.com/questions/2323292/windows-batch-assign-output-of-a-program-to-a-variable
Looks a bit sick, but seems to work.


On Linux, you could just use either of the following:

Code: [Select]

export variableName="Some fixed string: `command args`"

Code: [Select]

export variableName="Some fixed string: $(command args)"

[Vagabond]

While testing ways to get the directory name to display correctly with the SVN version number, I managed to set the directory to the current directory and deleted the majority of the OP2MapImager code.

Fortunately, it is all backed up to the repository and was quick to restore. (It may have all been in the recycle bin on the CPU as well, but didn't really check).

After that, I bought a book on the Windows Console and batch scripting. Currently working through the book's content. I figured it was time to actually learn what I am doing instead of hacking it constantly for post build events.

Unfortunately, the project is languishing a little as I could have just spent 2 minutes copying files over for a release instead of all these hours learning a new skill. It is something I've wanted to learn for a while though.

I don't think the subject is too complicated, so hopefully in a couple of days I'll have moved through the book far enough to know how loops, escape characters, etc. actually work and do things right.

[Hooman]

While testing ways to get the directory name to display correctly with the SVN version number, I managed to set the directory to the current directory and deleted the majority of the OP2MapImager code.

Hah!

Unfortunately, the project is languishing a little as I could have just spent 2 minutes copying files over for a release instead of all these hours learning a new skill. It is something I've wanted to learn for a while though.

Welcome to the world of software development. Where people spends hours or days, to save a few minutes or seconds. 

Funny how that works out too. You notice some boring repetitive task that keeps eating away at a few minutes, plus the associated cognitive load. So you figure you'll spend an extra two minutes to automate the two minute task. And it works, mostly, and saves time, but there is one small little largely inconsequential detail, and that's suddenly where you sink all your time and effort to get that last little bit done that keeps eating away at you. And you wonder out loud if it was worth the time, as you've now sunk more time into automating the task than the original task ever would have cost you in the long run. Yet you still feel victorious when you finally accomplish the task, because the hidden cost of that cognitive load is no longer weighing on you. And that makes it all worth it! 

[Vagabond]

I finished the first of 3 books on the Windows command line/batch scripting. I figured out enough to pull in the SVN revision number and append it to the end of the directory name. The author separated his book into 3 volumes about 85 pages each. They sell for $3.00 each on Amazon for an electronic copy. I would have been willing to pay $9.00 for the book all together. https://www.amazon.com/Learn-Command-Line-Batch-Script-ebook/dp/B00V9D3QVM/

New post build script is below. New release workflow at the bottom of the ReadMe is posted below that. 

Code: [Select]

REM Place FreeImage DLL into debug or release directory for running application within Visual Studio.
xcopy /y /d "$(ProjectDir)FreeImage\FreeImage.dll" "$(OutDir)"

REM Pull repository revision number and set into final release directory name.
FOR /f %25%25i IN ('svn info --show-item revision --no-newline') DO SET svnVersion=%25%25i

SET zipNameBegining="OP2MapImager Ver1.0."
SET zipName=%25zipNameBegining%25%25svnVersion%25
SET directoryName=$(OutDir)%25zipName%25 

if $(ConfigurationName) == Release (
    mkdir %25directoryName%25
    xcopy /y /d "$(ProjectDir)well00*.bmp" %25directoryName%25
    xcopy /y /d "$(ProjectDir)FreeImage license-gplv3.txt" %25directoryName%25
    xcopy /y /d "$(ProjectDir)ReadMe.txt" %25directoryName%25
    xcopy /y /d "$(OutDir)FreeImage.dll" %25directoryName%25
    xcopy /y /d "$(OutDir)$(TargetFileName)" %25directoryName%25

    REM Use 7Zip to zip final version
    7za a -tzip $(OutDir)%25zipName%25.zip %25directoryName%25
    
    DEL /q %25directoryName%25\*.*
    RMDIR %25directoryName%25 /s /q
)

Code: [Select]

 1. If changing Major/Minor revision number, set new version number at top of OP2MapImager main.cpp AND in post build event batch script.
 2. Run SVN Commit and then SVN Update to merge committed changes and properly update revision number of repository.
 3. Set Solution Configuration to Release.
 4. Set Solution Platform to x86. (x64 is not currently supported due to some WINAPI code within OP2Utility VolDecompress).
 5. Compile Code. 
 6. The following files will automatically be copied into the zipped directory'OP2MapImager 1.0.XXXX' (XXXX is svn revision number): 
    * OP2MapImager.exe (From Release Directory) 
    * FreeImage.dll (x32 version)
    * Well0000.BMP-Well0012.BMP (Reformated BMPs that a normal image editor may open.)
    * ReadMe.txt (this file)
    * FreeImage liscense-gplv3.txt
 7. Place zip file on the Outpost Universe Website.

[Hooman]

Nicely done. You're really going all in on this.

One small point, the "x.y.z" version numbers are generally referred to as Semantic Versioning. They have implied meanings concerning compatibility between versions. The revision numbers returned by SVN are more just a serial number. It might be better not to mix the two formats. Maybe just tag it as "rev-#". If you want something more human friendly, consider appending the revision date. You could append both a revision number and a revision date.

To get proper semantic version numbers, they would probably need to be manually tagged. One way is to detect a build from a tag branch, and extract the tag name. That only works for tagged releases though. I'd say just stick with revision numbers or dates for now.


I like that you actually bought a book to learn that stuff.
+1

[leeor_net]

Implementation Details
 * Repository Type: Subversion
 * Language: C++
 * Windows IDE: Visual Studio 2017
 * Image manipulation library???

DevIL

Have been using it with great success in Rogue Arena.

[Vagabond]

leeor_net,

We have had pretty good success using FreeImage. At this point, all the image manipulation is working properly, so I don't see any reason to switch, although I appreciate the suggestion. I hadn't updated the initial page to indicate we had went with it.

Hooman,

I read through the article on Semantic Versioning. It makes sense to me. I'll revisit this once I get the archive code to compile again (I'll post the errors in the VolDecompress thread).

I was thinking if there was a way to load the contents of a .txt file into a compile time constant, you could specify the version number in a text file. Then you could have the C++ program load the text file for indicating it's version number. It would be trivial to load the text file into the post build event and indicate it in the zipped filename and directory name. A build number within the version number could even be incremented within the text file via the post build event. Then whenever you were ready to change the version number, you would just need to change the .txt file and it would reflect in both the project and the post build event.

-Brett

[leeor_net]

leeor_net,

We have had pretty good success using FreeImage. At this point, all the image manipulation is working properly, so I don't see any reason to switch, although I appreciate the suggestion. I hadn't updated the initial page to indicate we had went with it.

As always I'm late to the party. FreeImage is a good option too, I just happen to have inherited it with the Rogue Arena project and learned that it's quite good at what it does.

[Vagabond]

As indicated in the release post, I finally finished the OP2MapImager. Check it out here: http://forum.outpost2.net/index.php/topic,5980.0.html

I already released the first patch (ver 1.0.1). When attempting to render all maps in the directory, I realized it was pulling SGAME10.OP2, which isn't a saved game but an internal script. This would of course crash the OP2MapImager. It was also pulling and imaging wellpallet.map even though this really isn't a map.

I added two flags to the ResourceManager class. They both default to false. One allows returning OP2SGAME10.OP2 on a directory wide search and the other allows returning wellpallet.map.

I also stopped the ResourceManager from returning duplicate files if the file exists both loosely and in an archive.

There is probably room for some sort of wildcard/regex/blob support for returning files through the ResourceManager, but for now, it is suiting the OP2MapImager's purpose, so I'm going to leave it alone.

If anyone else runs into bugs, please let me know.

Sorry, it is Windows only at this time (although it might work through Wine). I would like to remove the Windows dependencies within the Archive code someday, but this would require completing a console application that acted on creating/scanning archive files to test changes first.

My knowledge of C++ has grown a lot, although it is still not great. I'm learning C++ is a tough language to learn well.

[leeor_net]

My knowledge of C++ has grown a lot, although it is still not great. I'm learning C++ is a tough language to learn well.

That is an understatement and a half! I love C++ but it's, as you said, difficult to learn to use well especially with it's annoying mix of four languages in one (C, C with Objects, Templates and STL).

[Hooman]

I added two flags to the ResourceManager class. They both default to false. One allows returning OP2SGAME10.OP2 on a directory wide search and the other allows returning wellpallet.map.

I think you might be better served by adding appropriate error handling, rather than flags to avoid special case exceptions.


As for platform compatibility, I noticed one small thing that I submitted a fix for. Use forward slash as your path separator whenever possible. Forward slash is accepted as a path separator by both the Linux kernel and the Windows kernel. In particular, this applies to the #includes.

This might sound confusing, since the Windows shell (CMD.EXE) only accepts the back slash as a path separator. Under the hood though, the Windows kernel accepts either back slash or forward slash. In the case of the C++ compiler the #include leads to a call to open a file, using the Windows kernel. Hence, either separator is acceptable here.

On Linux, only a forward slash is accepted as a path separator. That's true of both the kernel, and the shell.

In short, if you're not writing a batch file, or a command that will be run by the shell, use forward slash. It will work everywhere.


Next task: removing the dependence on windows.h. I think you're not far from that. Perhaps a future pair programming session? Maybe also have Visual Studio generate a Makefile, and then switch over to Linux and see what happens there.

[Vagabond]

I think you might be better served by adding appropriate error handling, rather than flags to avoid special case exceptions.

Perhaps it would be better to add 2 public functions to ResourceManager, one that returns all map files in the directory and the other to return all save files in the directory? These respective functions could then remove wellpallet.map or OP2SGAME10.OP2 before returning their values. Currently ResourceManager allows returning all files with a given extension, which is what is creating the problem. It might not be apparent to the user that they will not get wellpallet.map when trying to get all the files of a given extension if they don't see the flags when scanning the header file.

Another aspect that I wasn't sure about was returning filenames vs StreamReaders. I have almost 100 unique maps in my Outpost 2 copy. It seemed strange to pass out 100 StreamReaders simultaneously, so I elected to pass out filenames if you ask for more than a single file. Maybe hundreds of StreamReaders isn't a big deal though? It could cause problems though if someone in the future is using the ResourceManager to get listings of files in archives for modification I suppose.

So, I could remove the it could look like:

Code: [Select]

SeekableStreamReader* getResourceStream(const string& filename, bool accessArchives = true);

void getAllFilenamesOfType(vector<string>& filenamesOut, const string& directory, const string& extension, bool accessArchives = true);

void getAllSavedGames(vector<string>& filenamesOut, const string& directory);

void getAllMaps(vector<string>& filenamesOut, const string& directory, bool accessArchives = true);

Currently there is a public function in ResourceManager:

Code: [Select]

void getAllStreamsOfFileType(vector<SeekableStreamReader*> seekableStreamReadersOut, const string& directory, const string& extension, bool accessArchives = true);

I think it would be best to just remove this and have the user just get filenames and ask for each stream one file at a time. I'm currently not using the function in the MapImager anyways.

In short, if you're not writing a batch file, or a command that will be run by the shell, use forward slash. It will work everywhere

Thanks for fixing. I'll start to incorporate this into the future.

Perhaps a future pair programming session?

I'm interested, but it will be a month or so before it is something I would be able to dedicate a block of time to.

[Hooman]

Perhaps it would be better to add 2 public functions to ResourceManager, one that returns all map files in the directory and the other to return all save files in the directory?

That seems a bit too specific for ResourceManager. Maybe.

It might be better to offer a shell glob search function from ResourceManager. Then users can search for "*.map", "*.OP2", or even better "SGAME0?.OP2". The last search pattern would save you the trouble of one of those flags.

As for the "wellpallet.map" file, does it actually crash, or just produce an ugly unexpected map? If it crashes, you need better exception handling. I think the map imager should just produce an error line stating a file couldn't be processed, maybe some details about the error, and then continue on.

On another note, "wellpallet.map" should be "wellpalette.map". And if it's not a real map file, and can't be processed as such, maybe it should be renamed to have a different extension. What does it even do anyway? There is already tile group info in the editor, so I assume it's not just duplicating that.

Another aspect that I wasn't sure about was returning filenames vs StreamReaders. I have almost 100 unique maps in my Outpost 2 copy. It seemed strange to pass out 100 StreamReaders simultaneously, so I elected to pass out filenames if you ask for more than a single file. Maybe hundreds of StreamReaders isn't a big deal though? It could cause problems though if someone in the future is using the ResourceManager to get listings of files in archives for modification I suppose.

Open files is a finite system resource. Don't just return hundreds of open files without a very good reason. It might not even work if the file limit is set too low. I'd say dump that code, as programmers should not be encouraged to do things that way.

This limit doesn't so much apply to the memory streams, or archive file contents, since they all share one actual file, but it still seems like a bad pattern.


I'm thinking you should avoid the out parameters. Not sure if this design is about syntax, or efficiency.

Assuming efficiency, you do want to avoid copying large structs and arrays around, but a vector is really just a small data type that references an array. It should be possible to pass the vector type by value, and still get by reference transfer of the underlying array data.

Relevant: Return Value Optimization (RVO)

In regards to syntax, I think it would be more clear to simply return the vector. The function user could then declare a variable and assign it to the result of the function, rather than having to declare the variable up front to be passed in.

Code: [Select]

vector<type> localVar = functionReturningVector();

[Vagabond]

I implemented a function called getAllFilenames that accepts a string that is turned into a regex search for the filename. This gave me fine grained control to avoid the last saved game and anyone in the future to do similar things without bloating the number of functions available. I also deleted the flags for the specific filenames.

I was looking at wellpallet.map, and it isn't being distributed by the base download of the game, so it shouldn't typically be a problem. I'm not sure where/when it got added to my copy of Outpost 2. It does render into an image without throwing an error. I moved the for loop that checks for it and deletes it into the MapImager code so ResourceManager doesn't have to worry about it. Something that didn't really need to be worried about in the first place, but it is taken care of now.

Here are the public members of ResourceManager now:

Code: [Select]

SeekableStreamReader* getResourceStream(const string& filename, bool accessArchives = true);

void getAllFilenames(vector<string>& filenamesOut, const string& directory, const string& filenameRegexStr, bool accessArcives = true);
void getAllFilenamesOfType(vector<string>& filenamesOut, const string& directory, const string& extension, bool accessArchives = true);

// Searches .vol and .clm archives for file and then extracts it. 
// returns true if EITHER the file is extracted OR 
//     if BOTH overwrite == false AND the file already exists in the directory.
bool extractFile(const string& filename, bool overwrite = false);

// Searches all .vol and .clm files and extracts any file with the given extension.
void extractAllOfFileType(const string& directory, const string& extension, bool overwrite = false);

I'm thinking you should avoid the out parameters. Not sure if this design is about syntax, or efficiency.

I am trying to return a reference to the vector since it doesn't make since to me to copy a class in this instance. I could return a pointer, but then have to explicitly delete the object later. This way I just create it upfront and don't have to delete it.

I read up some on the Named Return Value Optimization. So basically C++ will often compile into returning a reference even though the specification is to return a copy of a class from a function? Kind of messed up???

This limit doesn't so much apply to the memory streams, or archive file contents, since they all share one actual file, but it still seems like a bad pattern.

Agreed, I deleted the function.

[Hooman]

it doesn't make since to me to copy a class in this instance

Why?

The copy here should be cheap. If you return an object not by pointer, you don't need to worry about deleting it later.


Seems like getAllFilenamesOfType isn't sufficiently different from getAllFilenames. The difference seems to amount to "type" versus "*.type".

What is the directory parameter for? I find that unintuitive.


As for RVO, how else could a function return an object? How is it transferred between stack frames? It can't return first, and then copy, since the local variables must be destroyed before returning, so there would be nothing left to copy. The only real solution is to construct the object on the caller's stack in the first place, while the callee is still active. That requires a hidden reference pointer to be passed in.

[Vagabond]

Took the time to refactor all the OUT function variable references to instead be function return values in the ResourceManager class, main OP2MapImager file, and XFile class. Everything appears to be working properly.

What is the directory parameter for? I find that unintuitive.

XFile needs to know which directory you want to search for files in given either the file's extension or a REGEX string representing different possible filenames.

I could eliminate the directory parameter from both functions and assume ResourceManager only searches the directory that contains the pre-loaded archive files. I think it makes sense to be able to search other directories for files without having to create a new instance of ResourceManager.

Seems like getAllFilenamesOfType isn't sufficiently different from getAllFilenames. The difference seems to amount to "type" versus "*.type".

I like that you can get all the files of a type without using REGEX.

[Hooman]

I could eliminate the directory parameter from both functions and assume ResourceManager only searches the directory that contains the pre-loaded archive files. I think it makes sense to be able to search other directories for files without having to create a new instance of ResourceManager.

Using the VOL search folder as the base for file searches would be closer to how the game works. It also simplifies use for the caller since they don't need to keep a reference to the install folder around and pass it for every call. If you want to load a file outside of that folder, you can always use a relative path from there. I think that would actually be the recommended way of doing it.

Avoid absolute paths. They're a huge pain if you ever move stuff around, transfer things between systems, or sometimes even just install something to a non-standard place.

I like that you can get all the files of a type without using REGEX.

It feels like you're reinventing the wheel.


Also, "#include <filesystem>" should be fairly cross platform. Probably not much need to guard most of XFile with "#if defined(_WIN32)".

[Vagabond]

Also, "#include <filesystem>" should be fairly cross platform. Probably not much need to guard most of XFile with "#if defined(_WIN32)".

When I chose filesystem, I was hoping it would allow for being cross platform. The problem is that it is still in draft form, so they may change the implementation whenever it is actually released. and I didn't know if compilers on other operating systems support it, so I still pushed all the filesystem specific code into XFile.

From https://docs.microsoft.com/en-us/cpp/standard-library/filesystem

As of the release of Visual Studio 2017, the <experimental/filesystem> header was not yet a C++ standard. Visual C++ 2017 implements the final draft standard, found in ISO/IEC JTC 1/SC 22/WG 21 N4100.

Hooman, which compiler do you typically use on Linux, and do you know if it has implemented this filesystem standard?

Using the VOL search folder as the base for file searches would be closer to how the game works. It also simplifies use for the caller since they don't need to keep a reference to the install folder around and pass it for every call. If you want to load a file outside of that folder, you can always use a relative path from there. I think that would actually be the recommended way of doing it.

Avoid absolute paths. They're a huge pain if you ever move stuff around, transfer things between systems, or sometimes even just install something to a non-standard place.

Currently the default folder is wherever OP2MapImager.exe is located. It does make more sense to switc this so ResourceManager uses the folder where you told it to find the archive files as the default directory.

I need a way to distinguish between the directory, relative or absolute, and the REGEX expression that the user wants to use to search for multiple filenames though right?

-Brett

[Hooman]

When I chose filesystem, I was hoping it would allow for being cross platform. The problem is that it is still in draft form, so they may change the implementation whenever it is actually released. and I didn't know if compilers on other operating systems support it, so I still pushed all the filesystem specific code into XFile.

Using XFile is fine. What I'm saying is that within XFile, you have a lot of "#if defined PLATFORM" style code. A lot of it appears to be preemptive, assuming there will be a problem before it even has a chance. As the calls go to <filesystem>, which itself should be cross platform, you should be able to just assume platform specific differences will have already been taken care of. If they're not, a compile error on that functionality is probably more useful than a generic error message about the platform not being supported. I'd suggest removing the the guards.

I use primarily g++ on Linux, and on occasion clang. I do have <experimental/filesystem> available.

Currently the default folder is wherever OP2MapImager.exe is located. It does make more sense to switc this so ResourceManager uses the folder where you told it to find the archive files as the default directory.

Yes, I think the ResourceManger should store the path it gets passed when finding the VOL and CLM files.

I need a way to distinguish between the directory, relative or absolute, and the REGEX expression that the user wants to use to search for multiple filenames though right?

You can convert relative paths (including regex/glob) to absolute by prepending a root folder. This might be the path to the VOL and CLM files. Paths are specified to the ResourceManager as relative to its own folder, which are made absolute before being passed to the OS. This avoids issues where changes to the current working directory cause unintended side effects.

You can avoid prepending if the path is already absolute. Though whether to accept such paths is another issue. In some cases accessing files outside of some root folder might be a security risk. In such cases, it may make sense to throw an exception rather than process the path.

Code: [Select]

	if (directoryStr == "" || directoryStr == "/" || directoryStr == "\\" || directoryStr == " ")
		directory = current_path();

Here you might consider raising an exception for the "/" and "\\" cases. For the other two cases, the if is possibly meaningless, since the default handling for relative paths is to consider them relative to the current working directory (current_path()).

Also, it's concerning you're checking for " " (a single space). This strikes me as papering over a bug or bad design somewhere else in the code.

[Vagabond]

I use primarily g++ on Linux, and on occasion clang. I do have <experimental/filesystem> available.

Sounds good. I'll take the if Defined statements out then.

You can convert relative paths (including regex/glob) to absolute by prepending a root folder. This might be the path to the VOL and CLM files. Paths are specified to the ResourceManager as relative to its own folder, which are made absolute before being passed to the OS. This avoids issues where changes to the current working directory cause unintended side effects.

You can avoid prepending if the path is already absolute. Though whether to accept such paths is another issue. In some cases accessing files outside of some root folder might be a security risk. In such cases, it may make sense to throw an exception rather than process the path.

I want to ensure the directory does not include any REGEX and allow the filenames to include REGEX. So the user cannot search through multiple directories using REGEX, but they can search for multiple files in a directory. I'm not sure how to definitively separate out relative and or absolute directories from filenames that may include REGEX, so I think it is best to provide the directory and the REGEX defined filenames as separate variables.

I have no problems with the OP2MapImager searching anywhere on a user's computer that they specify for maps, archive files, and well files through either relative or absolute directories. I don't have any problem allowing them to save the renders anywhere they want. If this is a concern though, then I guess we could look into limiting to only saving files in My Documents or something?

Code: [Select]

	if (directoryStr == "" || directoryStr == "/" || directoryStr == "\\" || directoryStr == " ")
		directory = current_path();

Here you might consider raising an exception for the "/" and "\\" cases. For the other two cases, the if is possibly meaningless, since the default handling for relative paths is to consider them relative to the current working directory (current_path()).

Also, it's concerning you're checking for " " (a single space). This strikes me as papering over a bug or bad design somewhere else in the code.

I need a way to receive from the user that they are attempting to search the current directory for maps. If they pass

Code: [Select]

OP2MapImager /
or
OP2MapImager \

it will image all maps in the current directory. I'm not really experienced with the command prompt. If there is a better way to handle, I'd be happy to know about it.

I did just check, and you cannot pass an empty string through a command prompt argument, so an empty string or a single space aren't really an option. I wasn't sure when first writing so I threw them both in.

All of XFile is pretty unpolished right now. I was learning how to use the filesystem library code when writing it and didn't spend a lot of time cleaning it up. Since filesystem should work on both Linux and Windows, and you seem comfortable using it even in draft form, it would be good to spend some time cleaning it up.


Update. I pushed some changes to the code base to try and do some cleaning.

I want to simplify the function getFilesFromDirectory. However, if directory_iterator is used with a path that is empty (""), then it will not iterate over the current directory. I have to set the directory via current_path in order to make it work. That is causing the weird check the path against the directory in the first 3 lines.

Does it make sense to allow iterating over the directory if a path is passed into the function that contains a filename? If no, then I could simplify by just throwing an error if a filename exists at the end of the path.

I can move the line to check if the path string is a forward/backwards slash to the mapImager main.cpp file. Then it would be an implementation detail for how the user tells OP2MapImager how to iterate over the current directory. This might make more sense than placing it in XFile. This would apply to both versions of getFilesFromDirectory.

Code: [Select]

vector<string> XFile::getFilesFromDirectory(const string& pathStr, const string& fileType)
{
	path p(pathStr);
	path directory(p.remove_filename());
	if (p == directory)
		directory = current_path();

	if (pathStr == "/" || pathStr == "\\")
		directory = current_path();

	vector<string> filenames;

	for (auto& directoryEntry : directory_iterator(directory))
	{
		if (directoryEntry.path().extension() == fileType )
			filenames.push_back(directoryEntry.path().string());
	}

	return filenames;
}

[Hooman]

I want to ensure the directory does not include any REGEX and allow the filenames to include REGEX. So the user cannot search through multiple directories using REGEX, but they can search for multiple files in a directory.

Why? I don't see a reason for this restriction.

I'm not sure how to definitively separate out relative and or absolute directories from filenames that may include REGEX, so I think it is best to provide the directory and the REGEX defined filenames as separate variables.

Absolute/relative is an orthogonal concern to fixed/glob/regex.

A relative path is not complete. It can not be used to access files or folders. Either you must prepend a root path, or the OS will prepend the current working directory as the root path, and from there it will match using the absolute path.

You need a fixed path to open a file or folder. If instead you have a glob/regex, it must be used to search for a match. Any matches then become the fixed path to operate on, usually one at a time in a loop.

It makes perfect sense to use a glob or regex or match multiple folders, even if the filename portion ends up being fixed. Ex: "**/makefile" to find any "makefile" in any of the subfolders rooted at the current working directory. The "**" part will match zero or more folders. It's not found in every environment. It's common in the Ruby and Javascript worlds, but doesn't work in the command shell.

I have no problems with the OP2MapImager searching anywhere on a user's computer that they specify for maps, archive files, and well files through either relative or absolute directories. I don't have any problem allowing them to save the renders anywhere they want. If this is a concern though, then I guess we could look into limiting to only saving files in My Documents or something?

Don't worry about it for your map imager, but it may be a concern for a more general utility library. If such a component is used in a game that support multiplayer, and the other computers can influence which files are accessed, that can be a problem. Consider a game that support resource downloading, such as map files. Maybe it takes a map filename such as "custom.map", or "campaign/01.map", which by default will be in the game's "map/" folder. If instead, the path is "/etc/passwd", or (some arbitrary number of) "../../etc/passwd", or "symLinkToRoot/etc/passwd", then you could potentially trick the client into uploading its (Linux) password file. A contrived example here, since the host distributing the map is usually the one choosing the map. You could extend the idea though to the resources accessed by the map. Maybe the map references "/etc/passwd", so when you send the map to other players, their computers may try to access "/etc/passwd", which might get loaded into the game and somehow leaked back to the host.

You can defeat the absolute path attack by unconditionally adding a non-empty prefix. You can defeat the ".." attack by expanding the path to a canonical form, and ensuring the prefix still matches and there are no ".." left (or hack it, and just disallow any ".."). You might also want to turn off following symlinks in case a name links to a file or folder in a parent directory. This is sometimes handled by expanding to a canonical path, which may translate symlinks to their targets.

I need a way to receive from the user that they are attempting to search the current directory for maps.

You could check the count of the number of arguments, and if absent set a default path of "" for the current folder. Failing that, the user could specify:

Code: [Select]

OP2MapImager ""

Or even:

Code: [Select]

OP2MapImager ./

I want to simplify the function getFilesFromDirectory. However, if directory_iterator is used with a path that is empty (""), then it will not iterate over the current directory.

That is indeed unfortunate, and very unexpected. This may warrant further investigation.

Code: [Select]

if (pathStr == "/" || pathStr == "\\")
		directory = current_path();

This is very unusual. Such a path means absolute, not relative, and so it's unexpected that it would end up being based off the current working directory. I may have to take a closer look at this code.

Have you exported a makefile for the relevant projects?