Author Topic: OP2MapImager Development  (Read 1885 times)

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #50 on: August 04, 2017, 04:59:04 PM »
Code: [Select]
OP2MapImager ./

I played more with filesystem. filesystem::is_directory returns false upon passing a blank string (""). So, based on this, I'm thinking that filesystem is purposely not iterating over the current directory when a blank string is provided as the path. When I use a dot (.) or ./, everything works properly though. Reading the specification document for filesystem would possibly answer the question definitively, but I'm happy just using ./.

I had forgotten that ./ is used for the current relative directory. :-[ I reworked the OP2MapImager to take either . or ./ for indicating the current directory. Not allowing "" for a relative directory will keep someone from iterating over 100 maps by accident just because they left off the filename by accident. It also plays nicer with filesystem.

It didn't occur to me you could actually pass a blank string or a single space as an argument by using quotes. Experimenting with it a little teased out a bug when you pass in an empty string. I've fixed this bug on the development version.

Some point in the near future I'll release a new version of the OP2MapImager with both of these issues fixed.

So, that simplifies getFilesFromDirectory to the code below (minus the 3 commented out lines in the middle)

Code: [Select]
// For current relative directory use . or ./. An empty string is not considered a valid directory.
vector<string> XFile::getFilesFromDirectory(const string& directoryStr, const string& fileType)
{
path directory(path(directoryStr).remove_filename());

// Brett208 4Aug17: creating a path with an empty string will prevent the directory_iterator from finding any files in the current relative path on MSVC.
//if (directoryStr == "")
// directory = path(".");

vector<string> filenames;

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

return filenames;
}

Quote
Have you exported a makefile for the relevant projects?

No, but we will need to decide how to build the project on Linux at some point. I was considering just creating a completely new solution/project in whatever IDE you prefer on Linux and manually configuring required properties. We will have to load in the FreeImage.dll designed for Linux and create a separate post build event that complies with whatever IDE you want to use since I'm doubting it accepts Windows batch scripts. I've read up enough to know sort of what a makefile is and does, but not enough to use in practice. However it is done, I'm sure to learn a lot in the process.

Oh course first the Windows.h header needs to be teased out of the Archive code.

I was thinking to create a Console Application that finished defining the required functions to manipulate archive files first. This way we can test the results of changes to the archive code as windows.h is removed. Then perhaps circle around to the Linux build of everything.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3914
Re: OP2MapImager Development
« Reply #51 on: August 05, 2017, 06:16:38 PM »
MSVC has an option to export a Makefile. It might be easier to use that than generate a new one by hand. Though the NAS2D makefile is pretty generic, so maybe we could just reuse that.


Teasing out the "#include <windows.h>" will be a bit of a pain. It might be easier if we used regular stream code rather than memory mapped files. Perhaps converting that should be the first step.


I played around with the <experimental/filesystem> code today on Linux. Indeed, the directory_iterator does not like empty path strings. It also doesn't like shell globs, and it doesn't like filenames either. It must be a non-empty folder name.


The filesystem library does handle converting paths between various path/string/char* formats. That could allow some slight simplification to the code. For instance, directory_iterator can take any of those 3 types, which means you don't need to convert a string to a path before calling it. You also don't need to explicitly convert a path to a string before adding it to the vector.

After playing around a bit, I built out my example to be similar to your code, minus the filename extension checks. Here's what it looked like:
Code: [Select]
std::vector<std::string> getFilesFromDirectory(const std::string& dir)
{
  auto pathStr = dir.length() > 0 ? dir : "./";

  std::vector<std::string> filenames;
  for (auto& entry : fs::directory_iterator(pathStr))
  {
    filenames.push_back(entry.path());
  }

  return filenames;
}


Oh, and I'd say just assume the folder name passed is an actual folder, and not a file. Trying to strip a filename from the path and use the containing folder is a bit unexpected and could lead to subtle bugs in library use. I'd say just let the path feed through to the directory_iterator, where it will cause an exception if it refers to a file rather than a folder.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 1611
    • LairWorks Entertainment
Re: OP2MapImager Development
« Reply #52 on: August 05, 2017, 07:09:19 PM »
Would it make sense to use something like PhysicsFS to handle the cross-platform file handling? I've used it with great success.

Though it may make more sense to grab the code from TrenchBroom (map editor). It has an abstract filesystem that seems to have everything for Windows/Linux/Mac and it works pretty well once you get the hang of working with it:

https://github.com/kduske/TrenchBroom/blob/master/common/src/IO/FileSystem.h
https://github.com/kduske/TrenchBroom/blob/master/common/src/IO/FileSystem.cpp
- Leeor
LairWorks Entertainment

Titanum UFO's

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3914
Re: OP2MapImager Development
« Reply #53 on: August 05, 2017, 08:02:44 PM »
Upon cursory inspection, it looks like a nice API. Though I tend to shy away from adding external dependencies when they can be avoided. It complicates distribution and compiling.

I suppose that's one of the nice things about the Ruby world, or NodeJS. They have their own language specific packaging and distribution system, and the language core libraries insulate most code from platform considerations, so source is often highly portable between systems. It makes it very easy to incorporate external code, without adding significant difficulty for distribution, compiling, or platform considerations.

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #54 on: August 06, 2017, 03:52:26 AM »
leeor_net,

It looks like a clean library. The goal of using filesystem is that it may eventually become part of the standard library, which means many people would become familiar with it. Of course, since it is experimental right now, maybe using it isn't worth the squeeze.



Hooman,

Are you using an alias of fs for the namespace std::experimental::filesystem? If not, it looks like they named the namespace differently between the two compilers.

Code: [Select]
for (auto& entry : fs::directory_iterator(pathStr))
Produces

Error (active)   E0276   name followed by '::' must be a class or namespace name   



I'm getting E0304 from the line of code below. If I change it to entry.path().string(), then it compiles. So it looks like there are some differences between the two compilers implementation.

Code: [Select]
filenames.push_back(entry.path());
Error (active)   E0304   no instance of overloaded function "std::vector<_Ty, _Alloc>::push_back [with _Ty=std::string, _Alloc=std::allocator<std::string>]" matches the argument list

Here is the function rewritten to work in VS17. Since I have a using statement, declaring experimental::filesystem namespace for directory_iterator isn't necessary, but left it to point out the difference.

Code: [Select]
std::vector<std::string> getFilesFromDirectory(const std::string& dir)
{
auto pathStr = dir.length() > 0 ? dir : "./";

std::vector<std::string> filenames;
for (auto& entry : experimental::filesystem::directory_iterator(pathStr))
{
filenames.push_back(entry.path().string());
}

return filenames;
}

Also, if we want to accept empty strings as meaning the same thing as ./ or ., we should update the following two functions to read this way.

Code: [Select]
bool XFile::isDirectory(const string& pathStr)
{
if (pathStr.length() == 0)
return true;

return is_directory(pathStr);
}

Code: [Select]
bool XFile::pathsAreEqual(string pathStr1, string pathStr2)
{
if (pathStr1.length() == 0)
pathStr1 = "./";

if (pathStr2.length() == 0)
pathStr2 = "./";

path p1(pathStr1);
path p2(pathStr2);

return p1 == p2;
}

If this all looks good, I'll commit the changes.

-Brett
« Last Edit: August 06, 2017, 03:55:22 AM by Vagabond »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3914
Re: OP2MapImager Development
« Reply #55 on: August 06, 2017, 09:51:33 AM »
Ahh yes, I forgot the namespace line:
Code: [Select]
#include <experimental/filesystem>
namespace fs = std::experimental::filesystem;


The blank path was a problem for directory_iterator. I don't know if it's an issue for is_directory or path object comparisons. I had no trouble setting a path object to the empty string. I only had problems when I tried to use an empty path object with directory_iterator.

Maybe check what happens when is_directory is passed an empty string. As for comparing path objects, I'm wondering how robust the comparison is. A simple string compare won't be very effective in some cases, though I don't know if it does a string compare or something more involved. What if you compare "." and "./"? What if you compare "./" and "./subFolder/../"? In some cases it may make sense to do a simple string compare, to see if two paths are the same and in the same form. In other cases, you might want to know if two paths refer to the same thing, even if they are given in different forms.

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #56 on: August 06, 2017, 06:18:00 PM »
Just committed changes to the repo.

There are now 3 versions of getFilesFromDirectory. The one Hooman just wrote, one using regex for the filename, and one that uses the file extension. The regex and file extension overloads first call the generic getFilesFromDirectory and then remove files that do not meet the regex or file extension requirement. This way we do not have to repeat the line of code forcing a blank string into "./" and the directory iterator in each function.

Code: [Select]
vector<string> XFile::getFilesFromDirectory(const string& directory, const regex& filenameRegex)
{
vector<string> filenames = getFilesFromDirectory(directory);

for (int i = filenames.size() - 1; i >= 0; i--)
{
if (!regex_search(filenames[i], filenameRegex))
filenames.erase(filenames.begin() + i);
}

return filenames;
}



I have already tested is_directory with a blank string. It will return false, which makes sense in light of the directory_iterator not working with a blank string.



I played some with comparing paths. Using == against filesystem::path basically compares if the underlying string is the same. So == will return "." and "./" as different (false). However, the filesystem::equivalent function will return "." and "./" as being the same (true).

So, I rewrote the function as pathsAreEquivalent. The areas I was using pathsAreEqual in the code base, I was using it to compare if the filename itself was the same. Now in the code base, I just changed it to compare (==) the raw strings or used strcmp in the archive code since it uses char* instead of std::string.

Code: [Select]
// Will throw an exception if passed only a filename as path1 or path2. Lead filenames with relative or full directory.
bool XFile::pathsAreEquivalent(string path1, string path2)
{
if (path1.length() == 0)
path1 = "./";

if (path2.length() == 0)
path2 = "./";

return equivalent(path(path1), path(path2));
}

Test Data:

Code: [Select]
string d1 = ".";
string d2 = "./";
string d3 = "./Debug/../";
string d4 = "";

bool equal = XFile::pathsAreEquivalent(d1, d2); // TRUE

equal = XFile::pathsAreEquivalent(d1, d3); // TRUE

equal = XFile::pathsAreEquivalent(d2, d3); // TRUE

equal = XFile::pathsAreEquivalent(d1, d4); // TRUE
« Last Edit: August 07, 2017, 04:51:21 AM by Vagabond »

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #57 on: August 07, 2017, 05:03:08 AM »
Just committed code again. Fixed bugs when creating pathsAreEquivalent function. I got bit by trying to use == against 2 char* in the archive code since it uses char* instead of string. Ran out of time to fix when working last time so it was in a bit of an awkard state. Everything is working now.



We need to consider how to handle filenames being the same on different operating systems. For example on Windows tutorial.map is the same as Tutorial.map, but they are different valid filenames on Linux I think? So, simply comparing 2 filenames using == or strcmp may not be appropriate.

I think we want manipulated files to remain compatible on both systems. If on Linux you create tutorial.map and Tutorial.map and then pack them both into maps.vol, Windows will not be able to tell them apart. So really, the code should use the more restrictive option and treat tutorial.map and Tutorial.map as the same to prevent stuff like this from happening. It may not be initially intuitive to a Linux user at first, but we can comment the code to indicate the differences.

I've removed parthsAreEqual from XFile. In retrospect, do we want to re-add the function and use it to force Linux to consider ./tutorial.map as an equal path to ./Tutorial.map?

-Brett

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3914
Re: OP2MapImager Development
« Reply #58 on: August 08, 2017, 05:47:48 AM »
Quote
There are now 3 versions of getFilesFromDirectory. The one Hooman just wrote, one using regex for the filename, and one that uses the file extension. The regex and file extension overloads first call the generic getFilesFromDirectory and then remove files that do not meet the regex or file extension requirement. This way we do not have to repeat the line of code forcing a blank string into "./" and the directory iterator in each function.

Ahh, I wasn't expecting you to snaffle my function. That's fine. Sounds like you've simplified the code. Though I should point out it could be considerably less efficient to build the full file list, and then filter it to remove items you don't want. That is especially true of a large folder with only a few files of interest.

Seems like a template function could be written that takes some sort of filter function. That could eliminate code duplication, runtime costs of building a list and then filtering, and also dispatch cost for the filter function. A default filter function could be set that doesn't remove any filenames. Not quite sure how it would work though.

Code: [Select]
return equivalent(path(path1), path(path2));
What is "equivalent"?

I like how you have test code. Is it wrapped in a test suite? I was thinking having a few unit tests could be good for some of these projects.


The case sensitivity thing is more an issue of the file system than the OS. Think how Linux accesses NTFS or FAT partitions, or what happens when Windows accesses a file over a network share that's hosted on Linux. Sometimes there is a bit of support to paper over differences, sometimes not.

The VOL files are essentially their own file system. Whether they are case sensitive or not is up to the design of VOL files. I suppose I could check the Outpost 2 code to see if it uses case insensitive comparisons. Mostly though I'd suggest not bothering. It's not likely to be an issue in practice, and if it is, the user will probably be the best person to handle it.

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #59 on: August 09, 2017, 04:48:02 AM »
Equivalent means the two supplied paths return the same underlying directory or filename.

I can do generics in C#, but templates in C++ are beyond my current skill without some more studying/research. I'm not opposed to learning it though. I agree that it could be inefficient, but it also isn't noticeably affecting the execution speed of any of the consuming code.

I'll try not to get too wrapped up in case insensitivity then.

For testing the equivalent function, I just stuffed some code in the main function and stepped through it in the debugger to record the BOOL results. I'm not opposed to setting up unit tests, but I think typically they are done through some sort of framework. I have only done a little unit testing and that was all through C#. I guess we would need to discuss the strategy we wanted to use. I like that D has unit test built into the core of the language specification.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3914
Re: OP2MapImager Development
« Reply #60 on: August 09, 2017, 06:37:34 AM »
Quote
Equivalent means the two supplied paths return the same underlying directory or filename.

That feels vague. How is it determined? If "a" is a symlink to "b", does 'equivalent(path("/path/to/a"), path("/path/to/b"))' return true?

You're right about the inefficiency not affecting the program in question. There are bigger fish to fry.

I've never really done much unit testing in C++, at least not what I would call proper unit testing. Some cursory searching reveals there is a Google Test library that seems to be quite popular. It might be good to start developing a culture of unit testing here. Seems recent projects are taking off, and more people are starting to collaborate. Unit testing would be good for that.

Thought to bring this all back from things we could, or maybe should do, what are the specific objectives for the project. Maybe some of this is a bit of a distraction.

It seems the OP2MapImager is a basically working and complete project. Do you want to add more features, continue polishing it, or move on to something else? All are valid options.

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #61 on: August 10, 2017, 04:20:22 AM »
Quote
That feels vague. How is it determined? If "a" is a symlink to "b", does 'equivalent(path("/path/to/a"), path("/path/to/b"))' return true?

I deleted the equivalent function and replaced it with the pathsAreEqual function. I was getting frustrated with the filesystem::equivalent function because it throws errors if the path doesn't actually exist on the hard drive. pathsAreEqual ignores case and treats "./tutorial.map" and "tutorial.map" as equal. So now if you type "./Tutorial.map" it will recognize that you want to render "tutorial.map". It also won't throw an error if you are comparing filenames that only exist in an archive files. I didn't really have any use cases for pathsAreEquivalent anyways.

Quote
Thought to bring this all back from things we could, or maybe should do, what are the specific objectives for the project. Maybe some of this is a bit of a distraction.

Besides re-releasing the map maker with these bug fixes, I just want to get the OP2Utility code to a point where you are happy with its state when we are ready to port it to Linux. OP2Utility is also being used by OP2Archive. Otherwise, I'm finished with the application until someone has a problem with it.

If we are serious about unit testing than we can either use this project as a test case or start by hooking it into OP2Archive (sorry for the pun :) ). I'll check out Google Test in the near future. If it is popular, then it is probably a good choice because it should be supported for a long time, people may already be familiar with it, and learning it may help directly apply to projects outside of Outpost 2.

Leeor,

I don't suppose OutpostHD/NAS2D uses any unit test library???

Also, I'll be uploading my changes shortly to the REPO. I kind of fell behind on this.

-Brett

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 1611
    • LairWorks Entertainment
Re: OP2MapImager Development
« Reply #62 on: August 10, 2017, 01:43:16 PM »
I don't suppose OutpostHD/NAS2D uses any unit test library???

No -- I never got around to it. What I've been doing instead is building small programs that test core functions/classes/objects and confirming output/reliability either visually (with the renderer test, test2 in NAS2D-Tests) or with text output.

NAS2D's test programs are here: https://github.com/lairworks/nas2d-tests

The Renderer test (test2) is the most complete test though there are other functions I haven't added to it yet to inspect their output.

For OutpostHD, I built small sample programs that test specific parts of the simulation. There are three at the moment, the first and second revisions of the Population model and the PopulationPool object to ensure correct behavior: http://redmine.outpostuniverse.org/projects/outposthd/files

I don't know if these could be considered 'unit tests' or if they'd even be helpful. But, there they are. :)
- Leeor
LairWorks Entertainment

Titanum UFO's

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3914
Re: OP2MapImager Development
« Reply #63 on: August 12, 2017, 02:55:52 AM »
Quote
I don't suppose OutpostHD/NAS2D uses any unit test library?

I was thinking similarly.

Leeor_net, nice to see those tests. I didn't even notice they existed before. I took a quick peek. They're not quite unit tests, in that results are presented in a MsgBox. Usually unit tests have an automated pass/fail criteria, and console logging of the test results. This makes it easy to have them run automatically, often after every commit, or pull request, and the results logged to be inspected later. Inspecting the output would be similar to the Travis CI or Codacy output setup for the GitHub repo.

In fact, such tests could be run by the Travis CI build. Though, usually unit tests get added to the same repo, which makes the process a little easier. Makes sense though, since unit tests are usually added and updated along with the code they are testing. You can keep the tests out of the "src" folder though, perhaps in a separate "test" folder. The build system doesn't need to build the test code by default.

Offline Vagabond

  • Sr. Member
  • ****
  • Posts: 431
Re: OP2MapImager Development
« Reply #64 on: August 12, 2017, 04:51:01 PM »
I spent some time reading through Google Test's documentation.

The documentation seems clear to read. It seems like robust support for both Windows and Linux, although based on some of the documentation, I tend to think it wasn't developed for Visual Studio primary (which is fine). I liked how they pointed out in their documentation where they differed some from a standards document on unit testing and that the standards were in fact stated better than what they had developed. Shows a healthy respect for standardization.

It also seems pretty in line with the ideas/theories that I've used in my limited C# based unit testing in the past.

I'll look into downloading the Linux and Windows builds and see how big the downloads are, etc. I'd probably prefer pushing this into OP2Archive first over back-fitting OP2MapImager though.

Happy Coding!

-Brett