[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]