Regression using last version(s) of ImageMagick

[Sean Enck]

On 2018-10-13, Steve Allewell wrote:
> On 13/10/2018 18:32, Sean Enck wrote:
> > On 2018-10-13, Steve Allewell wrote:
> >> On 12/10/2018 20:42, Sean Enck wrote:
> >>> On 2018-10-08, Sean Enck wrote:
> >>>> It appears importing an image has regressed (no longer works) against 
> >>>> kxstitch 2.1.1 when using the latest (well archlinux latest) versions of 
> >>>> image/libmagick. The current (not working version) in arch:
> >>>>> imagemagick-7.0.8.12-1-x86_64.pkg.tar.xz
> >>>>> libmagick-7.0.8.12-1-x86_64.pkg.tar.xz
> >>>>
> >>>> I downgraded my versions to verify and import worked (no crash):
> >>>>> imagemagick-7.0.7.13-1-x86_64.pkg.tar.xz
> >>>>> libmagick-7.0.7.13-1-x86_64.pkg.tar.xz
> >>>>
> >>>> The steps are:
> >>>> 1. open kxstitch
> >>>> 2. go to import image
> >>>> 3. select an image (any image, I haven't seen it work)
> >>>> 4. doesn't matter if you do anything before hitting "OK" when the image 
> >>>> would go to the normal grid
> >>>> 5. full crash, can provide a trace if there is anything special in kxstitch 
> >>>> or any specific tracing method is preferred.
> >>>>
> >>>> Thanks for any help/insight,
> >>>> --Sean
> >>>
> >>> I was able to do some digging and found the issue started around ImageMagick 
> >>> 7.0.7.24 (works) -> 7.0.7.25 (broken). I traced it to the FlossScheme.cpp 
> >>> color matching where nullptr was being returned and I patched this in 
> >>> Arch using the referenced patch below.
> >>>
> >>> https://aur.archlinux.org/cgit/aur.git/plain/0001-Change-nearest-color-lookup-logic-for-finding-floss-.patch?h=kxstitch
> >>>
> >> Hi Sean
> >>
> >> Apologies for not getting back to you earlier.  I noticed you sent a
> >> message to the list on the 8th, but it doesn't look like I got a copy of it.
> >>
> >> I have version 7.0.8 of ImageMagick too and confirm the bug.
> >>
> >> I had a look at the patch and it does the job, although it does not
> >> require float for near and closest; abs and QColor red, green, blue
> >> functions all return int.
> >>
> >> I did a little rewrite which works for me, please check it for yourself:
> >> -------------------------------------------------------------------------------
> >> Floss *FlossScheme::find(const QColor &color) const
> >> {
> >>     QListIterator<Floss *> flossIterator(m_flosses);
> >>
> >>     Floss *matched = nullptr;
> >>     int closest = 100;
> >>
> >>     while (flossIterator.hasNext()) {
> >>         Floss *floss = flossIterator.next();
> >>         QColor c = floss->color();
> >>
> >> 	// the color mapping may not be perfect so search for a near match.
> >>         int distance = abs(color.red()-c.red()) +
> >> abs(color.green()-c.green()) + abs(color.blue()-c.blue());
> >>
> >>         if (distance < closest) {
> >>             matched = floss;
> >>             closest = distance;
> >>         }
> >>     }
> >>
> >>     return matched;
> >> }
> >> -----------------------------------------------------------------------------
> >>
> >> Do you want to commit the change?
> >>
> >> I will have a look to see what changed in ImageMagic between those two
> >> versions and see if there are any improvements to make.
> >>
> >> Regards
> >>
> >> Steve
> >>
> > 
> > Thanks for another set of eyes on it!
> > 
> > I initially emailed (without troubleshooting more than what I put) 
> > because I didn't have the time until (days) later to look into the code.
> > I'm not (generally) a C++ guy so I need a little extra time to try
> > things (before realizing I could just use the call you had as a the
> > criteria, I tried another which did require a float and didn't go back
> > once I realized what you already had was an int approximation... I just
> > needed to track it). I started trying to see what changed in ImageMagick
> > but I have no idea what kxstitch uses from it so it was more of "where, 
> > in the code does this seem to be happening?"
> > 
> > I applied your update locally and it appears to work just the same as my
> > initial patch (aka bug still gone).
> > 
> > Happy to commit the (joint) change, I wasn't sure what the process 
> > is/was? I initially used format-patch to try and bundle it fully as a
> > consumable idea/be able to patch the arch packaging.
> > 
> > --Sean
> > 
> Sorry, I thought I had seen your name against a commit before, but after checking I realised I was mistaken.  I know we have had a
> number of emails in the past.
> 
> I will do the commit and credit you for the fix if that's ok.
> 
> I had a look at the ImageMagick code changes for 7.0.7-25 and there wasn't anything that stood out.  I will have a look over the
> kxstitch code and how it uses ImageMagick to see if I have made an error in that.
> 
> Thanks for all the efforts.
> 
> Steve

Oops responded direct but alas - I'm good with this commit strategy

--Sean