Neworigins v6 merger with master branch #142
Conversation
New origins storyline
Ruleset update
Bugs hunting
Rules changes for man and prices
Add stables and modify name
Continents map setting
Camels and configs
Remove ending condition
Better World Events. Now with assassinations :)
* Updated description for FORGET to reflect the defined ruleset. * Rephrased after commit review.
* fixed event order, now more significant events will be on top * cossmetic update of the text art * added missing space after number of looses * even more missing spaces added back
Update the workflow runner to run on the current neworigins-v6 branch. Also have it run on any jt-* branches which I use for local feature testing (I don't necessarily like pushing that last bit into master but I figure I am likely the only one who cares)
We have a number of compiler warnings in the current code. Let's get rid of them all. Add -Werror to prevent new compile warnings from creeping in
This code is mostly dead and hasn't been updated/built/looked at in a long time. It will still exist in git history, and I believe that Artyom is going to create 'last state/arcadia' branch which preserves it before merging all the neworigin and neworigins-v6 changes to master to bring things back to sanity.
Snapshot tests kind of suck in general and are more fragile, but they are all we really have right now for testing before the unit test changes and they can still serve some purpose for doing end-to-end testing. This moves the directory to be better named, updates the github workflow to use the new directory/driver script and enhances that script to compare the output to 'expected' output each step of the way.
I'd forgotten to remove it from the Makefile when I killed the directory.
Begin process of wiring up creating and running unit test.
In order to make sure that some of the work I am doing around file output works correctly, I wanted to make sure I had some neworigins snapshots including the quest system which the standard snapshots don't have. This checkin includes those snapshots to the point that quests have been created on turn 13. This also updates the scripts to run them so that both sets of snapshots will be run to validate the tests.
The intent of the code was that weighter landmarks (settlments) would take preference over fortifications, etc. This test was backwards. However, it was also possible that if there were two equally close and equally weighty landmarks, the order would fluctate based solely on how the in-memory layout of the unordered_map being used to construct the data got laid out. This changes this to be a stable ordering based on distance (shorter distance), weight (higher weight), x coordinate (westernmost preferred) and finally y coordinate (northernmost preferred). Exposed the comparison function to allow unit tests and added unit tests.
| @@ -129,6 +129,19 @@ void ARegionList::CreateNexusLevel(int level, int xSize, int ySize, char const * | |||
| } | |||
| } | |||
|
|
|||
| // void ARegionList::CreateConstrainedSurfaceLevel(int level, int xSize, int ySize, char const *name, int contients, | |||
There was a problem hiding this comment.
I am not sure why this is commended here
There was a problem hiding this comment.
It looks (from looking at the code and the history) that this was added by Valdis about 2 years ago for neworigins as an attempt to improve the map generation, and was added as a dummy/empty function to the other rulesets, then later removed from the game.h header and commented out everywhere. This is (to me) prime example of code that should be just tossed since it's pretty obvious that an even better system has already replaced it.
|
|
||
| void WriteSides(ARegion *,Unit *,Unit *,AList *,AList *,int, | ||
| ARegionList *pRegs ); | ||
|
|
||
| // void WriteBattleStats(ArmyStats *); |
There was a problem hiding this comment.
Need to investigate why it was commended
There was a problem hiding this comment.
Similarly, it looks like that was your initial attempt from 3 years ago to create some battle statistics and has been replaced with newer/better code (the BattleLogLevel::Detailed stuff in the normal battle output generation).
|
This was merged as a squash merge that kills the history. It is fine when we merge changes from the one author like that. But when there are many it is not correct. @artyomtrityak @jt-traub |
|
Also, deletion of the Arcadia was intentional? |
|
I propose to revert this PR and merge no_v6 directly into the master. This will be more correct. |
Yes.. I got annoyed when searching for functions that it kept going to the arcadia version rather than the real version, and since arcadia wasn't regularly being built and was quite diverged from the changes that had been made in the mainline files on neworgins, it seemed better to leave it in-state on the v5_stable branch, and get rid of it from the 'main' branch. I am the one who authored the pr to remove it. :) |
In general I agree with you.. I would probably have retained the branch commit history. But I don't feel strongly enough that it needs to be reverted and remerged. That branch history still exists in github, it just takes a bit of work to get to. |
|
As I'm the one who is most annoyed with that. Then I will unwind history myself or do nothing. |
WIP, reviewing the code