Standardize documentation with Doxygen format

[Brett208]

@ldicker83 was looking to use Doxygen for documenting the code. Doxygen is widely used and popular and I have no issue trending toward using it.

Wanted to open this issue to give a chance for anyone to complain before decent sections of code start getting marked up in the Doxygen format.

[DanRStevens]

Go ahead.

I'm not terribly fond of it, but that's mostly because I'm not used to it yet.

Doesn't look like there are many strong alternatives to Doxygen either.

[ldicker83]

Okay so I've been taking a closer look at the code (finally, been a rough week) and I'm noticing a lot of giant question marks about design choices, particularly in Map (I'll check out the rest as well).

There are a number of member variables that are declared public -- this flies in the face of typical C++ development practices. Generally you would provide functions to modify these values and/or get references to these members.

I don't want to go mucking around too much in the code and just make sweeping changes but I have made some name changes for member variables (coding practice to make it obvious to anybody using the code where the variable is coming from... e.g., prefix an 'm' to member variables for class members, ALL_CAPS for local/static variables and CamelCase for namespace members). Function names are usually in camelCase style in most C/C++ libraries but I'm not opposed to the former use that's more typical of newer libraries, JAVA and C#.

Will push a pull request soon with basic documentation and formatting updates soon for review.

[ldicker83]

Okay so in my infinite wisdom I didn't fork the repository, I just cloned it so I was unable to push a pull request versus simply pushing my changes. Would like it if you guys could review the changes and let me know what need to be addressed. Will fork for the future to allow for pull requests (unless there's a way I can do it without forking)

[Brett208]

@ldicker83 You made about 700 lines of code changes without a pull request, changed style practices without discussing, and broke the automated build on the master branch. You will not even know if your changes compile proper on Linux (g++ and clang) until pushing unless you are compiling on both operating systems on your own.

We need to delete the changes from master and push them into a branch/pull request for discussion and fixing.

In this case, a fork is not required to create a pull request. Just work in a branch on your local copy. When you push the branch, you can open a pull request. You only need to fork if you plan to diverge long term, or if usage restrictions on the repository require it (OP2Utility does not).

-Brett

[ldicker83]

Yeah, that's what bugged me about not having the option to create a pull request. Really wanted to have the changes reviewed before a full commit.

Anyway, not sure how to revert changes in master -- will research tomorrow and go from there.

EDIT:

Okay, simple enough, rolled back.

[DanRStevens]

For reference, I think we were planning to stay away from member variable prefixes. If you really need to distinguish from a local variable with the same name, you can use this->memberVariableName.

And yeah, just makes changes on a new branch in this repository, and push the branch. GitHub then lets you open a pull request between the branch and master within the same repository. That's how we've been working and doing code reviews.

I just tried to do a pull, and it didn't bring in any changes. I'm guessing history was edited to remove the additions. Not sure what happened to your work, but it's possible to create a branch with the changes after they have been committed to master. There are times when I accidentally commit to my local master branch, but realize my mistake before pushing. In that case, I branch at that point, switch back to master, reset the HEAD of my local master to before the commits were added, (clear out local changes), and then switch back to the branch I'd just created to keep working. Very quick to fix this sort of thing.

Much more difficult though, is splitting a large commit into smaller targeted self contained commits. Makes review much easier though. You brought up some interesting points in your commit. A few I have concerns about though. If they were done as separate commits, or even separate branches, it would make review and inclusion easier as a concern on one change doesn't need to hold up other changes.

Edit: I'm glad to see you're taking an interest in the code though.

[Brett208]

Leeor, you brought up a good point that the Map codebase is not well encapsulated. Since Outpost 2 map code contains definitions for tile groups and tilesets, we had discussed trying to separate this data off from the core map class.

If we could decide on how this separation looks, then I think there would be 3 set types:

• Structs that represent how the data appears in Outpost2 (mostly what we already have, just maybe stripping the object oriented convenience shims from the structs)
• A class or set of classes that represent the map itself with the tileset details divorced.
• A class or set of classes that represent the tileset data, tile ranges, tile groups, etc

Then we would just need to write code that combines the map info with the tileset info when saving or separates the data when loading.

I had tried to more fully encapsulate the current code earlier, but found the coupling of the tilesets to the maps difficult to handle. For example, deleting a tileset would mean completing the following:

• Removint the Tileset Source from the map
• Remove Tiles from map that are from the tileset
• Update remaining Tiles to account for removed tiles (changes indices)
• Update Tile Groups After Tileset Removal (deal with groups that contained tiles from the removed tileset)
• UpdateTerrainTypesAfterTilesetRemoval (remove or update terrain types that contained removed tiles)

In particular, the ranges are tough to deal with because they can span multiple tilesets. So if you delete a tileset in the middle of the range, you would have to pluck it out of the tile range.

When adding a tileset back into a map, we currently have no way to define which tile is the bulldozed version, or meteor hit tile, or tube tile, etc. So I don't know how we could create an AddTileset type function to a map right now.

Ultimately, I just decided to leave things unencapsulated until we decided on a way to split concerns between the tileset information and the map information.

Sorry that I didn't write that down anywhere earlier.

Perhaps we need to define our end goal with the maps and tilesets being defined separately before getting heavy with the doxygen formal documentation?

-Brett

[DanRStevens]

I think you hit the nail on the head there. We don't really have a clear idea on where we're going with this part.

We know it's messy, and not well encapsulated. It's somewhere between an open struct and a proper class, and there's no clear indication which way it should go. Additionally, we don't yet have clear semantics defined for object validity. Do we allow invalid states to make it easier and more efficient to work with the object, and then validate before saving. Or validate only when requested. Or do we design operations to ensure the object is always in a valid state. Right now we don't have much functionality for working with the tilesets in a cohesive consistent way. That means we don't have operations for working with the tilesets in a way that ensures validity. We can either deny access to the tileset data, preventing people from working with it, or doing anything useful with it, and thus protecting it. Or we can allow free reign on the data for now, until we have better functions in place.

I think my preference is to allow the invalid states until we have time to develop methods to work with the data in a safe and consistent manner. We may even want to continue that way permanently for efficiency reasons. For instance, consider if someone wants to remove one tileset and replace it with a compatible one. If it breaks down into a remove followed by an add, without disturbing the map data, then there is an intermediate invalid state. If we had tried to clear out the map data when removing a tileset, it'd be very difficult to replace a tileset with another graphically different yet compatible version. One way to do it safely would be to provide an explicit replace method, which handles the operation atomically, but that may start to bloat the class. It may also prove restrictive. Consider another example, where someone replaces one tileset with two smaller tilesets that together are compatible. How does that map onto the API?

We do need to start somewhere though. Something small, targeted, and standalone. Iterate to the design we want.

[ldicker83]

All the changes were poofed. I did a history rollback so all the changes are gone. The 700 lines changed was a stretch, maybe only 70 lines changed and most of that was minor formatting with the addition of some functions that provided tighter encapsulation. But it's meh, I just need to remember to create a new branch.

[DanRStevens]

You can check the git reflog of your local repository. The changes won't be garbage collected for about 30 days. It might be possible to checkout an old poofed revision by reflog name or SHA name, and then create a branch from it.

[ldicker83]

Nope. It's all completely gone. No references to it locally. Nothing in reflog about it, just the commits leading up to HEAD.

[DanRStevens]

Try:

git checkout 3a67f1aa6f8f33ba11496ccd8be77752f028fefb

---

It's all going to look like "HEAD" in the git reflog.

Each time you create a new commit, or switch branches, you update the position of HEAD. Git stores a history of HEAD in the git reflog. You can access specific items with the syntax HEAD@{n}, which refers to the position of HEAD n moves ago.

You may get something out of reading: Git: Restoring Orphaned Commits with git reflog

I was tempted to try and find a way to examine the reflog on GitHub. It seems there is no standard way to do that through Git, but GitHub does offer it's own API way to do it:

curl https://api.github.com/repos/OutpostUniverse/OP2Utility/events

Reference: git can I view the reflog of a remote?

---

Or, there is an obvious shortcut. Just look up in this thread to the message about the commit referencing this thread. It links to the commit, and reveals the SHA. That's how I got it for the git checkout command I listed. You can view the commit online: https://github.com/OutpostUniverse/OP2Utility/tree/3a67f1aa6f8f33ba11496ccd8be77752f028fefb

---

Curiously, when I tried to download that specific commit myself, I got an error message. Doesn't look like there is an easy way for me to download the changes, even though I can view them online:

git fetch origin 3a67f1aa6f8f33ba11496ccd8be77752f028fefb
error: Server does not allow request for unadvertised object 3a67f1aa6f8f33ba11496ccd8be77752f028fefb

The error message is the same if I used a nonsense made up SHA. It works for SHAs on a known branch though.

You'd probably have better luck trying to restore from your local repository. I had fun learning today, but I leave it you if you want to salvage any of that work.