From kde-games-devel Sat Jul 25 02:25:12 2020 From: Ian Wadham Date: Sat, 25 Jul 2020 02:25:12 +0000 To: kde-games-devel Subject: Re: Fixes for KPat Forty & Eight patience game Message-Id: X-MARC-Message: https://marc.info/?l=kde-games-devel&m=159564393823096 > On 25 Jul 2020, at 8:00 am, Albert Astals Cid wrote: >=20 > El dijous, 23 de juliol de 2020, a les 6:22:19 CEST, Ian Wadham va = escriure: >>=20 >>> On 23 Jul 2020, at 9:59 am, Albert Astals Cid wrote: >>>=20 >>> El dimarts, 21 de juliol de 2020, a les 0:47:31 CEST, Ian Wadham va = escriure: >>>>=20 >>>>> On 21 Jul 2020, at 7:48 am, Albert Astals Cid = wrote: >>>>>=20 >>>>> El dilluns, 20 de juliol de 2020, a les 9:05:26 CEST, Ian Wadham = va escriure: >>>>>> Hi Albert, >>>>>>=20 >>>>>>> On 18 Jul 2020, at 5:38 am, Albert Astals Cid = wrote: >>>>>>>=20 >>>>>>> El divendres, 17 de juliol de 2020, a les 8:35:23 CEST, Ian = Wadham va escriure: >>>>>>>>=20 >>>>>>>>> On 17 Jul 2020, at 6:38 am, Albert Astals Cid = wrote: >>>>>>>>>=20 >>>>>>>>> El divendres, 10 de juliol de 2020, a les 7:38:40 CEST, Ian = Wadham va escriure: >>>>>>>>>> Hi guys, >>>>>>>>>>=20 >>>>>>>>>> Good news. In brief, I have found two or three causes of the = 10 bugs listed in >>>>>>>>>> = https://bugs.kde.org/buglist.cgi?quicksearch=3Dkpat%20forty%20eight. >>>>>>>>>>=20 >>>>>>>>>> However, I am going to need help from an expert with a = knowledge of the KPat code to work out the most appropriate patches. >>>>>>>>>>=20 >>>>>> >>>>>>>>>> I will also need help to get patches committed, when they are = ready, because I do not have access to or knowledge of the newer KDE = Community repositories and procedures. >>>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> Unfortunately I don't think we have many KPat experts around = anymore, Shlomi and Fabian are the two that did changes to somewhat core = parts lately. >>>>>>>>=20 >>>>>>>> I was hoping Parker Coates might be able to advise us. I did = see a post on kde-games-devel from him not long ago. >>>>>>>>=20 >>>>>>>>> My suggestion would be for you to put the patch as a Merge = Request in invent.kde.org >>>>>>>>=20 >>>>>>>> I have looked at that online, but I am really not up to it, = Albert. The last review tool I had anything to do with was ReviewBoard = and I have not committed or checked out anything for about five years. I = have been keeping an eye on things using =E2=80=9Canongit" and sometimes = offering suggestions or a =E2=80=9Cdiff -u=E2=80=9D style patch (not a = =E2=80=9Cgit diff=E2=80=9D style one). But as of now I cannot even = figure out how to get a read-only git clone of KPat from invent.kde.org. >>>>>>>=20 >>>>>>> git clone https://invent.kde.org/games/kpat.git >>>>>>=20 >>>>>> Thanks, Albert, I was able to get a local git clone, as above. >>>>>>=20 >>>>>> As you may have seen, I have sorted out the Forty & Eight bugs on = bugs.kde.org and eliminated some duplicates. I have also finalised my = changes to KPat and am doing final testing on my Apple OSX KDE 4 version = of KPat. One patch is tested and ready to present, but I am not able to = test it on the KF5 version of KPat. >>>>>>=20 >>>>>> I have the change on a branch in my local clone, but have not = committed it, nor written a commit message there. >>>>>>=20 >>>>>> Attached is a patch in =E2=80=9Cgit diff=E2=80=9D format to show = you what I mean. It fixes the illegal move bug in = kpat/patsolve/fortyeightsolver.cpp. >>>>>>=20 >>>>>> I have taken the liberty of adding some comments to explain what = is going on in the code and make it less cryptic. Also I have re-written = the conditional code for multi-card moves, to use multi-character = identifiers and make it more readable. I could have fixed the whole = thing with one line and no comments, but that is not how I like to = work=E2=80=A6 :-) >>>>>>=20 >>>>>> So how would you like to proceed from here? >>>>>>=20 >>>>>> >>>>>>>>> and that way we all can have a look at it and maybe try to = understand it together. >>>>>>>>=20 >>>>>>>> Indeed. I look forward to that. >>>>>>=20 >>>>>> >>>>>>>> I think the best I can do, over the next week or so, is first = to tidy up the bug reports in: >>>>>>>>=20 >>>>>>>> = https://bugs.kde.org/buglist.cgi?quicksearch=3Dkpat%20forty%20eight, >>>>>>>>=20 >>>>>>>> flagging duplicate reports, collating test cases, etc. Then I = can add my patches as attachments to whichever are the =E2=80=9Cmain=E2=80= =9D reports (with appropriate comments). Then maybe links to that work = in Bugzilla could help kick off any discussion on a merge request. >>>>>>=20 >>>>>> Close to finishing that. >>>>>>=20 >>>>>>>> What do you think, Albert? >>>>>>>=20 >>>>>>> I guess that could work, not optimal, but better than nothing :) >>>>>>=20 >>>>>> So how would you like to proceed from here, using the attached = fix for https://bugs.kde.org/show_bug.cgi?id=3D302140 as a trial? >>>>>=20 >>>>> The patch seems reasonable (from the i've no idea what you're = really talking about) scenario :D >>>>>=20 >>>>> Do you have a numbered hand in which to reproduce the issue? >>>>=20 >>>> Certainly. The best examples were submitted by Duncan Kinnear in = comments https://bugs.kde.org/show_bug.cgi?id=3D290647#c7 through = https://bugs.kde.org/show_bug.cgi?id=3D290647#c10. Comments 7 and 9 = provide attached KPat save-files from which you can load a game and = observe the bug. Comments 8 and 10 have attached screenshots of the same = game-states with the invalid move highlighted by =E2=80=9CHint=E2=80=9D = action=E2=80=A6 but you cannot actually play a move where two cards are = of the same suit and not in ascending sequence. Demo, Hint and solver = play it happily and claim that the game is winnable. That=E2=80=99s the = issue. >>>>=20 >>>> The saved game at https://bugs.kde.org/show_bug.cgi?id=3D290647#c9 = is the better of the two examples. The invalid move is used by the = solver to claim that the game is winnable. The 5 of Clubs is under the = King of Clubs but the Demo plays both cards onto the 6 of Clubs. For = that to be valid, the two cards to move would have to be 4 and 5 of = Clubs. This example is particularly good because when the bug is fixed, = the game becomes unwinnable, as seems to be the case however I try to = win it. >>>>=20 >>>> There are other attached save-files in = https://bugs.kde.org/show_bug.cgi?id=3D302140, which is the =E2=80=9Cmain=E2= =80=9D report for this bug, but they are rather old and I have had no = success at using them so far. >>>=20 >>> Hmm, question to try to understand the code a bit better. >>>=20 >>> When you say "starting with the top two cards" you mean 8/K or J/10 = in https://i.imgur.com/5paxqtH.png ? i guess you mean J/10 as in "cards = in the top of the pile" and not as in "cards closer to the top part of = the screen", right? >>=20 >> Heh, heh=E2=80=A6 Understanding the code is 95% of the battle. It = took me 3 or 4 weeks to understand patsolve/fortyeightsolver.cpp, before = I could fix the bugs. That included a LOT of confusion about what is a = =E2=80=9Ctop=E2=80=9D card. >>=20 >> In the internals of KPat generally (the =E2=80=9Cmodel=E2=80=9D = part?), the code depends on objects called KCard grouped in stack = objects (in the Comp Sci sense) called KCardPile, or =E2=80=9Cpile=E2=80=9D= for short. The =E2=80=9Ctop=E2=80=9D card is often referred to and it = is always the top of a pile or end of list or end of array, e.g. there = are functions with names like topCard(). >>=20 >> When piles are displayed in a game (the =E2=80=9Cview" part?), they = are often spread downward and face-up, as in your illustration. So then = the =E2=80=9Ctop=E2=80=9D card in the internal =E2=80=9Cmodel=E2=80=9D = is actually the =E2=80=9Cbottom=E2=80=9D card in the =E2=80=9Cview". In = your illustration the =E2=80=9Ctop=E2=80=9D card, in KPat's solver = semantics, is indeed the 10 of Clubs and the Jack is the next card. >>=20 >> I find it helps, when reading KPat solver code to think of the = =E2=80=9Ctop=E2=80=9D card in a face-up pile as being the one whose face = is fully visible, regardless of how the pile is displayed. Often this is = also the first moveable card in the pile. >>=20 >> Just to add to the difficulty, several lower-level KPat solvers = (including patsolve/fortyeightsolver.cpp) pack cards and piles into an = array called W (for workspace) with one byte per card (see definition in = patsolve/patsolve.h). The array Wp provides an index into the =E2=80=9Ctop= =E2=80=9D card of each pile and the array Wlen tells how many cards are = in the pile (counting backwards from the =E2=80=9Ctop=E2=80=9D card :-) = ). >>=20 >> Hope this helps. Good luck and cheers, >> Ian W. >>=20 >> P.S. As a path to understanding I used lots of "fprintf( stderr=E2=80=9D= lines and the procedure =E2=80=9CFortyEightSolver::print_layout()=E2=80=9D= . To keep the output manageable, I used "Settings->Remember State On = Exit=E2=80=9D and put conditions =E2=80=9Cmax_positions =3D=3D 1=E2=80=9D = on the printing. The max depth to which the solver will go is set by = that parameter and it is set to 1 to get a Hint. So, to analyse a single = position, I could just press Hint and get about a page of printout. When = I had examined it and maybe changed some print statements, I could just = start KPat to get back to the same game number and move number. Very = nice. >=20 > Ok, thanks for the explanation. >=20 > I've commited the code (with some minor stylistic changes) Thank you very much for all your help on this, Albert, and for seeing it = through to a conclusion=E2=80=A6 :-) All the best, Ian W.