Re: [Wesnoth-dev] [bug #13118] Risk of OOS with hidden units
On Sat, Mar 07, 2009 at 05:21:50PM +0100, Jörg Hinrichs wrote: We can add code to make RC2 send data in both formats, allowing the two versions to communicate, but I prefer not Yes, please don't do that Agreed So, we must forbid RC1 to join the RC2 server. Yes, please do that ;-) Agreed -- Regards, Mark de Wever aka Mordante/SkeletonCrew ___ Wesnoth-dev mailing list Wesnoth-dev@gna.org https://mail.gna.org/listinfo/wesnoth-dev
Re: [Wesnoth-dev] [bug #13118] Risk of OOS with hidden units
Hi Ali, first of all, thanks a lot for this precise and very helpful analysis. It described the problem very well and gave me a good direction where to take a closer look at. The motivation behind my change was to provide the same codebase for replays and the regular in-game actions. It was originally triggered by a bug (the one that is mentioned in the commit comment obviously). However, there is more to it. Problems like these weren't visible in the past, simply because WML was either much simpler back then or its possibilities were not exploited to the max (or both ;-). Meanwhile WML has become very mighty and its might is used as well. So unless we don't do things in replays exactly as they are done in-game, i am sure we will fight an uphill battle against OOS, since replays are the central mechanism to synchronize clients in multiplayer. That is why I would like to investigate the first (though probably more complicated) solution: Adjust pathfinding, so that it is the same in-game and for replays. Unless that turns out to be totally infeasible for 1.6 of course. I will keep you updated. Greetings Yogi Details: It's mainly pathfinding difference between clients. But first, I think that we must revert r32669 http://svn.gna.org/viewcvs/wesnoth?rev=32669view=rev or at least the part where we now do a real move of unit, instead of just drawing one. The commit doesn't say what event was not being triggered, seems to be move events (possibly sighted event, but was never replay-safe, and will probably never be). In that case just firing it manually in replay code may fix this. The core of this problem is that we just send source and destination of the move action, letting local pathfinding found a path between them and assume that it will be the same as the sender really used. The 1.4 version was already horrible about that, because it didn't even used the same pathfinding algorithm (paths instead of A*) I tried to improve that with r27830: http://svn.gna.org/viewcvs/wesnoth?rev=27830view=rev But at that time is was only a visual glitch, for example the receiver could see (enemy) sender's unit pass by the hex of his hidden unit. This old system is not safe because, any difference between two clients could cause different pathfinding results. Which is mainly a problem when the receiver solution pass near an ambushed unit, but it's also less good for the gameplay, because you have false assumptions on which hexes your opponent know to be ambush-safe or the fog he has cleared. Track and fix all of these pathfinding differences is probably too much work and we will never be sure to get them all (I already see potential problems with multi-turn moves from mouse or goto, AI, fog or discovered hidden units not up-to-date) Note that using different paths may also cause an OOS bug about the move_left (bug #12467) So, I suggest: - Get back to a purely visual move, send move event, and forget fog and sighted event in replay (I believe it's only a problem if they modify the game state) - Transmit the move_left in the [move] data, to be sure to avoid the move_left bug OR (and better I think) - Transmit the path in the [move] data, that is something like path=3,3,4,3,5,3... or x=3,4,5 and y=3,3,3 I think there is also future plan with a pass by this hex event which will need exact path. But sadly all of this will break compatibility with RC1, and it's a bit late for such change. OtOH it may prevent OOS troubles in 1.6 PS: YogiHH, assigned it you to get your opinion and possibly let you fix the problem (you know this code better than me), but I am available for help or coding about this (I also have other work which will depend how this stuff change or not) ___ Reply to this item at: http://gna.org/bugs/?13118 ___ Message sent via/by Gna! http://gna.org/ attachment: winmail.dat___ Wesnoth-dev mailing list Wesnoth-dev@gna.org https://mail.gna.org/listinfo/wesnoth-dev
Re: [Wesnoth-dev] [bug #13118] Risk of OOS with hidden units
No, i am not opposed to transmitting the path data. It would require to separate the pathfinding stuff from actions.cpp::move_unit but that shouldn't be so hard to do. But you are right, it will break compatibility. If there is a way to get around that, I'd prefer that one. I will have to look at the code again to get an impression what the implications of different approaches are. But I tend to do it right, even if it means more work than apply a quick workaround. I simply had too many bad experiences with that. Follow-up Comment #1, bug #13118 (project wesnoth): I totally agree with the idea of using the save action::move_unit function, it's a wise change. I was probably not clear in my report, but if I suggested to revert it, it's because it makes these pathfinding differences much more important (it was only visual before) and because it's recent bold change. But have you noticed that it's orthogonal to my proposition to send the full path in the [move] action ? In fact, it may even be an optimization of replay, because it allow to skip all the pathfinding search (only the sender do it). And I believe it's easy to code, just need to parse a string. Like I said I suspect that controlling all areas affecting pathfinding will be more work. Also, note that relying on pathfinding exact match and changing it in RC2 will break compatibility anyway. So are you opposed to transmit path data ? ___ Wesnoth-dev mailing list Wesnoth-dev@gna.org https://mail.gna.org/listinfo/wesnoth-dev
Re: [Wesnoth-dev] [bug #13118] Risk of OOS with hidden units
Follow-up Comment #4, bug #13118 (project wesnoth): alink, i see i misunderstood you about an important point. I thought you wanted to skip the actions.cpp::move_unit call altogether and return to the old behaviour. Looking at your patch I saw this is not the case. I actually wasn't aware about the connections between fog and pathfinding. That is indeed a major reason not to try to synchronize pathfinding on all clients. Furthermore, your patch removes the pathfinding logic of replay moves in replay.cpp, which I think is a good thing as well. I thought about that quickly myself when I was reworking that part, but couldn't find an easy way to do it, so I left it standing the way it is. But I am happy to see it vanish. One last comment: When calling actions.cpp::move_unit the last parameter (is_replay) is meant to continue moves which would otherwise be interrupted (which they should not in a replay). This might happen, if the AI moves and a skirmishing unit is ambushed. The move will be interrupted for a human (which actions.cpp::move_unit was originally designed to deal with), but not for the AI (which has its own moving code). So please add that parameter to the call since the default value is false. @Ivanovic: I leave this up to you, since this will break compatibility with the current release (any replay before this patch won't work at all). However, I strongly recommend to apply this patch since I feel it will make the code a lot more robust and prevent having to deal with some very nasty, hard to detect and reproduce bugs in the future. As alink mentioned before, we can continue to support the old replay format (source and destination instead of a route). There are two hearts about this in my chest: The coder says no, don't introduce some temporary stuff that has potential for a lot of problems, the community member says people might be pretty pissed off, if all their replays don't work anymore :(. As this is still a release candidate, personally I'd let the coder win, to be honest. Greetings Yogi ___ Wesnoth-dev mailing list Wesnoth-dev@gna.org https://mail.gna.org/listinfo/wesnoth-dev
