Fixes for KPat Forty & Eight patience game

Fri Jul 24 23:00:01 BST 2020

El dijous, 23 de juliol de 2020, a les 6:22:19 CEST, Ian Wadham va escriure:
> 
> > On 23 Jul 2020, at 9:59 am, Albert Astals Cid <aacid at kde.org> wrote:
> > 
> > El dimarts, 21 de juliol de 2020, a les 0:47:31 CEST, Ian Wadham va escriure:
> >> 
> >>> On 21 Jul 2020, at 7:48 am, Albert Astals Cid <aacid at kde.org> wrote:
> >>> 
> >>> El dilluns, 20 de juliol de 2020, a les 9:05:26 CEST, Ian Wadham va escriure:
> >>>> Hi Albert,
> >>>> 
> >>>>> On 18 Jul 2020, at 5:38 am, Albert Astals Cid <aacid at kde.org> wrote:
> >>>>> 
> >>>>> El divendres, 17 de juliol de 2020, a les 8:35:23 CEST, Ian Wadham va escriure:
> >>>>>> 
> >>>>>>> On 17 Jul 2020, at 6:38 am, Albert Astals Cid <aacid at kde.org> wrote:
> >>>>>>> 
> >>>>>>> El divendres, 10 de juliol de 2020, a les 7:38:40 CEST, Ian Wadham va escriure:
> >>>>>>>> Hi guys,
> >>>>>>>> 
> >>>>>>>> Good news. In brief, I have found two or three causes of the 10 bugs listed in
> >>>>>>>> https://bugs.kde.org/buglist.cgi?quicksearch=kpat%20forty%20eight.
> >>>>>>>> 
> >>>>>>>> However, I am going to need help from an expert with a knowledge of the KPat code to work out the most appropriate patches.
> >>>>>>>> 
> >>>> <snip>
> >>>>>>>> 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.
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> 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.
> >>>>>> 
> >>>>>> 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.
> >>>>>> 
> >>>>>>> My suggestion would be for you to put the patch as a Merge Request in invent.kde.org
> >>>>>> 
> >>>>>> 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 “anongit" and sometimes offering suggestions or a “diff -u” style patch (not a “git diff” 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.
> >>>>> 
> >>>>> git clone https://invent.kde.org/games/kpat.git
> >>>> 
> >>>> Thanks, Albert, I was able to get a local git clone, as above.
> >>>> 
> >>>> 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.
> >>>> 
> >>>> I have the change on a branch in my local clone, but have not committed it, nor written a commit message there.
> >>>> 
> >>>> Attached is a patch in “git diff” format to show you what I mean. It fixes the illegal move bug in kpat/patsolve/fortyeightsolver.cpp.
> >>>> 
> >>>> 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… :-)
> >>>> 
> >>>> So how would you like to proceed from here?
> >>>> 
> >>>> <snip>
> >>>>>>> and that way we all can have a look at it and maybe try to understand it together.
> >>>>>> 
> >>>>>> Indeed. I look forward to that.
> >>>> 
> >>>> <snip>
> >>>>>> I think the best I can do, over the next week or so, is first to tidy up the bug reports in:
> >>>>>> 
> >>>>>> https://bugs.kde.org/buglist.cgi?quicksearch=kpat%20forty%20eight,
> >>>>>> 
> >>>>>> flagging duplicate reports, collating test cases, etc. Then I can add my patches as attachments to whichever are the “main” reports (with appropriate comments). Then maybe links to that work in Bugzilla could help kick off any discussion on a merge request.
> >>>> 
> >>>> Close to finishing that.
> >>>> 
> >>>>>> What do you think, Albert?
> >>>>> 
> >>>>> I guess that could work, not optimal, but better than nothing :)
> >>>> 
> >>>> So how would you like to proceed from here, using the attached fix for https://bugs.kde.org/show_bug.cgi?id=302140 as a trial?
> >>> 
> >>> The patch seems reasonable (from the i've no idea what you're really talking about) scenario :D
> >>> 
> >>> Do you have a numbered hand in which to reproduce the issue?
> >> 
> >> Certainly. The best examples were submitted by Duncan Kinnear in comments https://bugs.kde.org/show_bug.cgi?id=290647#c7 through https://bugs.kde.org/show_bug.cgi?id=290647#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 “Hint” action… 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’s the issue.
> >> 
> >> The saved game at https://bugs.kde.org/show_bug.cgi?id=290647#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.
> >> 
> >> There are other attached save-files in https://bugs.kde.org/show_bug.cgi?id=302140, which is the “main” report for this bug, but they are rather old and I have had no success at using them so far.
> > 
> > Hmm, question to try to understand the code a bit better.
> > 
> > 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?
> 
> Heh, heh… 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 “top” card.
> 
> In the internals of KPat generally (the “model” part?), the code depends on objects called KCard grouped in stack objects (in the Comp Sci sense) called KCardPile, or “pile” for short. The “top” 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().
> 
> When piles are displayed in a game (the “view" part?), they are often spread downward and face-up, as in your illustration. So then the “top” card in the internal “model” is actually the “bottom” card in the “view". In your illustration the “top” card, in KPat's solver semantics, is indeed the 10 of Clubs and the Jack is the next card.
> 
> I find it helps, when reading KPat solver code to think of the “top” 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.
> 
> 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 “top” card of each pile and the array Wlen tells how many cards are in the pile (counting backwards from the “top” card :-) ).
> 
> Hope this helps. Good luck and cheers,
> Ian W.
> 
> P.S. As a path to understanding I used lots of "fprintf( stderr” lines and the procedure “FortyEightSolver::print_layout()”. To keep the output manageable, I used "Settings->Remember State On Exit” and put conditions “max_positions == 1” 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.

Ok, thanks for the explanation.

I've commited the code (with some minor stylistic changes)

Cheers,
  Albert

> 
> > 
> > Cheers,
> >  Albert
> > 
> >> 
> >> Cheers,
> >> Ian W.
> >> 
> >> 
> > 
> > 
> > 
> > 
> 
> 