widelands

Merge lp://staging/~widelands-dev/widelands/bug-better-syncstreams into lp://staging/widelands

  1. bug-better-syncstreams
  2. Merge into trunk
Proposed by Notabilis
Status: Merged
Merged at revision: 8981
Proposed branch: lp://staging/~widelands-dev/widelands/bug-better-syncstreams
Merge into: lp://staging/widelands
Diff against target: 544 lines (+273/-11)
16 files modified
data/tribes/buildings/productionsites/atlanteans/mill/init.lua (+1/-1)
src/economy/economy.cc (+2/-1)
src/economy/request.cc (+1/-1)
src/logic/cmd_queue.cc (+1/-2)
src/logic/filesystem_constants.h (+1/-0)
src/logic/game.cc (+51/-0)
src/logic/game.h (+66/-1)
src/logic/map_objects/bob.cc (+1/-0)
src/logic/map_objects/map_object.cc (+7/-1)
src/logic/map_objects/tribes/battle.cc (+1/-1)
src/logic/map_objects/tribes/program_result.h (+1/-0)
src/logic/map_objects/tribes/worker.cc (+2/-2)
src/network/gameclient.cc (+5/-1)
src/network/gamehost.cc (+3/-0)
src/wlapplication.cc (+7/-0)
utils/syncstream/syncexcerpt-to-text.py (+123/-0)
To merge this branch: bzr merge lp://staging/~widelands-dev/widelands/bug-better-syncstreams
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+361922@code.staging.launchpad.net

Commit message

Print more information in syncstreams. Create additional smaller syncstream files containing the last few seconds leading to a desync.

Description of the change

[Ready for review and merge]

Since I am not really able to get useful information out of the existing syncstream files, this branch adds further information to the syncstream describing the type of the syncstream entries.

Additionally, create a smaller syncstream extract file next to the syncstream file which contains only the last few seconds. This should be enough to debug the desync but reduces the size of the to-be-uploaded files.

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4409. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/481989790.
Appveyor build 4200. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_better_syncstreams-4200.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4411. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/482186977.
Appveyor build 4201. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_better_syncstreams-4201.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I think this is an excellent idea, so I've done a preliminary code review. Let me know when you are finished for a final review.

Revision history for this message
Notabilis (notabilis27) wrote :

Thanks for the review. I answered some of your comments below and will push a merge-ready version later on.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Answered.

Revision history for this message
Notabilis (notabilis27) wrote :

As far as I am concerned, this branch is ready for review and merge now.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LGTM, not tested

review: Approve
Revision history for this message
Notabilis (notabilis27) wrote :

I tested(?) it by enforcing a desync (modifying player.cc and using std::rand() as ID for new economies, then changing economy targets ingame). The new *.wse files were created and could be parsed by the python script. No idea what else can be tested.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I have done some similar testing. I'd like to see a few changes:

1. WLApplication:cleanup_replays() does not clean up old wse files
2. There are no instructions in the Python script.
   I'd like to at least see a usage hint when it's called without parameters.

Revision history for this message
Notabilis (notabilis27) wrote :

I wrote some documentation, do you think it is understandable and sufficient?

Revision history for this message
GunChleoc (gunchleoc) wrote :

Looks good to me, let's have it!

@bunnybot merge

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: