[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-25 2:25:12
Message-ID: BCD8CB22-9796-411C-925F-D3C24A690041 () gmail ! com
[Download RAW message or body]


> On 25 Jul 2020, at 8:00 am, Albert Astals Cid <aacid@kde.org> wrote:
> 
> 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@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.
> 
> Ok, thanks for the explanation.
> 
> 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… :-)

All the best,
Ian W.


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