Patch revision number suggestion

[Matthew]

This is an idea for making life easier for patchers and for James as project lead.

At the moment, we are in the the happy situation of having several patches submitted at any one time (thank you for your many contributions, Ranran and others!). If more than one needs to increment the revision number, then there can be conflicts. Ranran usually tries to increment the revision number in different patches, but he can't know whether his patches will be accepted in the same order.

And it seems possible that an incorrect number was included in the main (master) branch this week. This is not surprising because the workflow is tricky and boring.

So here's a random idea. Could we add a very short header file, next_patch.h....

Code Select Expand

(Copyright notice)

/// See CodingStyles.txt for usage:
#define NEXT_EX_VERSION_MAJOR 15
#define NEXT_EX_VERSION_MINOR 18

...and then #include it everywhere?

When people are writing patches, they can just write code like get_extended_version() < NEXT_EX_VERSION_MAJOR for compiling and testing.

When James integrates patches into the main (master) branch, you just need to do a find and replace in the code (excluding next_patch.h) and increment the NEXT_EX defines ready for the next round of patches. That could even be scripted, though it might be safer to replace one by one in the IDE.

Does this appeal to you, James? The only point is to make your code reviewing easier so it's entirely subjective.

Or is it just stupid? If so, please tell me why so I can learn from the experience!

[Ranran(retired)]

It's not a way to avoid conflicts, but at least it would be useful to have something similar to history.txt as an easy way to do it.

Briefly describe for what purpose the revision was increased, like:

Code Select Expand


-14.17
48 add convoy and line chart records
49 add a halt chart record (and change some contents)
50 minimap linkage and citylist factorylist (dialog open state)
51 heavy mode
-14.18
52 line color patch


Occasionally we may encounter save-related issues, which are useful for later confirmation.
Most of the information about the revision is hidden and we have to look for the code.

[PJMack]

I had a few other ideas on how to handle this situation that we may want to consider as well:

1. For the .pak files, each node has its own version that is read independently.  Each instance of a rdwr function could have its own internal revision number so as long as two people are not working on the same function at the same time, no conflict would occur (and those that do would likely need a manual merge anyway).  A list of internal revision numbers would probably go on the header with the main revision number to reduce redundancy.

2. Have a sub-revision number equal to the PR number.  The revision header would contain a compacted list of merged PRs, and any new branches would add their PR number to the list.  The compaction scheme would be an array of sub-revision numbers, and lowest number PR that is still pending.

3. Have a pool of sub-revision numbers "a"-"h" than can be manually assigned to each developer or project.

One case that would need special attention is the settings rdrw function as it is likely to have multiple people working on independent sets of features in this function concurrently.  As that particular function is only used once per game, it may be a candidate for refactoring to save as a dictionary or std::map type structure where the variables could be written in random order, and read regardless of the revision number (the settings file itself is in no particular order).  This would come with some overhead, so I cannot recommend it for any other parts of a simutrans file.

I have thus far managed to avoid needing to change the revision number in a branch.  For the private car routing compression PR, I had taken advantage of a coordinate array which I could put negative coordinates in to indicate a newer version and squeeze the added header information into.  For parts of the pier system, I was able to squeeze a boolean value into the upper bits of a ribi_t.  Unfortunately tricks like that are not always available, and I had to temporarily hard-code a what should be settings value in the pier system branch.  I placed the variable in the settings class, but I have delayed making it actually user settable.

[ceeac]

My suggestion is to auto-label Pull Requests if simversion.h changes. This way, one can easily see if a) the PR affects the save format, and b) whether one should check that the PR still increments the save version correctly if other PRs were merged that affect the save format. Usually, simversion.h does not change except if the save version is bumped.

[jamespetts]

Thank you all for your suggestions, and apologies for not having had a chance to consider this earlier.

I would suggest following Ceeac's suggestion in this instances - having a NEXT_EXTENDED_VERSION (etc.) would not avoid clashes, as there is always a risk of there being a clash of the NEXT version just as much as the current version, so this rather defers the problem than solves it, and also adds much work on each merge.

As for PJMack's suggestion, this is more about pakset versioning, I think - can you elaborate on what exactly you mean by each node having its own version? Each type of item (e.g. way, vehicle, etc.) already has its own versioning independent of each other type of item.

[PJMack]

I was referring to the savegame format but using the pak format as an example as to follow for the first idea.  Currently the .pak files have a version number for each object, however the .sve savegame files only have a single version number for the entire file.  What I was recommending is that the non-trivial objects (e.g. not koord/koord3d) each have there own version number in the savegame.  The other two ideas were also for the savegame, however now that it has been brought up I believe that they could be adapted for pak files of so desired.

[jamespetts]

Interesting - can I ask what it would mean for a save game file to have different version numbers for different parts of the file? How would this help in a real life situation?

[Mariculous]

I don't think we should try savegame format versioning on feature branches at all!
Due to the nature of how collaboration on git works, it simply won't work reliably.

Savegame versions are uniquely versioned in an incremental way as soon as they are merged into master.
Otherwise, it is sufficient to indicate that the savegame is stored in a format which differs from those incrementally versioned on master.

This can be indicated by a simple flag in simversions.h
A magic macro variable FUTURE_VERSION can be defined to any high number.
It should be used in those rdwr methods that work with the new version.

On merge of the PR, an automated script can reset the flag, increment the version number and replace all occourences of FUTURE_VERSION by that number.

Bear in mind that builds from a feature branch are not meant for production use but for debugging and testing only.

[PJMack]

Interesting - can I ask what it would mean for a save game file to have different version numbers for different parts of the file? How would this help in a real life situation?

One example would be if one developer adds variables to industries that needs to be saved with the game when at the same time another adds variables to tunnels that needs to be saved with the game.  Both developers would be able to add their own variables to the save file, each incrementing the appropriate object versions, and still be able automatically merge.

Bear in mind that builds from a feature branch are not meant for production use but for debugging and testing only.

I disagree that developers and testers should only use feature branches and patches for test games.  I believe that the most thorough testing would be testing of these branches and patches in actual gameplay.  Not allowing players to continue their games after starting them on a branch or patch would likely cause fewer players to be willing to volunteer test these branches, and the developers (also volunteers) would be more likely to only test artificially created scenarios rather than in actual gameplay, myself included.