[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-20 7:05:26
Message-ID: C7B928C2-4F9A-477C-934F-F9A4D3303DA6 () gmail ! com
[Download RAW message or body]

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?

Cheers,
Ian W.


["patch-kpat-bug302140" (patch-kpat-bug302140)]

diff --git a/patsolve/fortyeightsolver.cpp b/patsolve/fortyeightsolver.cpp
index 0e244c2..82840b8 100644
--- a/patsolve/fortyeightsolver.cpp
+++ b/patsolve/fortyeightsolver.cpp
@@ -411,7 +411,17 @@ int FortyeightSolver::get_possible_moves(int *a, int *numout)
         }
         if ( Wlen[w] > 1 && d.freestores )
         {
-            if ( SUIT( *Wp[w] ) == SUIT( W[w][Wlen[w]-2] ) )
+            // Column w has two or more cards and there is at least one empty
+            // column, so we can try for multi-card moves from w to other cols.
+            // This is valid only if ALL the cards to move are of the same
+            // suit and in ascending sequence, starting with the top two cards.
+
+            // Fix bug https://bugs.kde.org/show_bug.cgi?id=302140
+            //     "Forty & Eight solver makes an illegal move".
+            card_t nextw = W[w][Wlen[w]-2];	// Next card after top.
+            bool possMultiMove = ( SUIT( nextw ) == SUIT( *Wp[w] ) ) &&
+                                 ( ( RANK( nextw ) - RANK( *Wp[w] ) ) == 1 );
+            if ( possMultiMove )	// Prev code tested SUIT but not RANK.
             {
                 //print_layout();
 


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