[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-games-devel
Subject:    Re: Fixes for KPat Forty & Eight patience game
From:       Ian Wadham <iandw.au () gmail ! com>
Date:       2020-07-23 4:22:19
Message-ID: 19754ECA-5F03-4618-8C4E-8DE06695224D () gmail ! com
[Download RAW message or body]



> On 23 Jul 2020, at 9:59 am, Albert Astals Cid <aacid@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@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@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@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.

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


[prev in list] [next in list] [prev in thread] [next in thread]