[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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic