[poppler] Followup Bug 32349 & Poppler: More shading fun ;-)

[Albert Astals Cid]

A Dimarts, 25 de gener de 2011, Andrea Canciani va escriure:
> On Tue, Jan 25, 2011 at 11:38 PM, Albert Astals Cid <aacid at kde.org> wrote:
> > A Dimarts, 25 de gener de 2011, vàreu escriure:
> >> On Tue, Jan 25, 2011 at 10:54 PM, Albert Astals Cid <aacid at kde.org> 
wrote:
> >> > A Dimarts, 25 de gener de 2011, vàreu escriure:
> >> >> On Tue, Jan 25, 2011 at 10:25 PM, Albert Astals Cid <aacid at kde.org>
> > 
> > wrote:
> >> >> > A Dimarts, 25 de gener de 2011, Andrea Canciani va escriure:
> >> >> >> On Tue, Jan 25, 2011 at 9:00 PM, Albert Astals Cid <aacid at kde.org>
> > 
> > wrote:
> >> >> >> > ...
> >> >> >> > 
> >> >> >> >> Finished successfully, i'll have a look at the code tomorrow
> >> >> >> >> and if i don't find anything obviously wrong will commit it to
> >> >> >> >> master
> >> >> >> >> 
> >> >> >> >> :-)
> >> >> >> > 
> >> >> >> > Had a look at the code and it is too untidy, please remove all
> >> >> >> > the ifdefs of unused code and remove the T_EDGE and T_CORNER
> >> >> >> > defines and i'll commit it.
> >> >> >> 
> >> >> >> 0001 and 0002 replace most of Thomas's patch, the exception being
> >> >> >> attachment 0003. I don't understand what is the purpose of that
> >> >> >> change, maybe Thomas can explain it and suggest a better commit
> >> >> >> message.
> >> >> >> 
> >> >> >> I think 0001 and 0002 should be quite clean and commit ready.
> >> >> >> Please review them, I'll fix any remaining problem as soon as
> >> >> >> possible.
> >> >> > 
> >> >> > Hmm, sincerely i prefer to commit Thomas patches first. It has
> >> >> > taken us lots of regtesting iterations to get to something that
> >> >> > gives improvements and no regressions. Once that is in we can
> >> >> > start with your patches :-)
> >> >> 
> >> >> These patches replace it, they implement the same change.
> >> >> I think it's quite pointless to commit it just to revert it
> >> >> immediately, but if you like it better...
> >> > 
> >> > I know they implement the same feature, but can you guarantee 100%
> >> > that they will not have any regression that Thomas patches we know do
> >> > not have?
> >> 
> >> No, I can't guarantee at 100% (I wouldn't do it even for Thomas's patch,
> >> even if it has been tested throughly), but I'm quite confident they are
> >> as correct, because they performs the very same computation, which is
> >> also the same as the one performed in pixman and in cairo-gl (yes, I've
> >> already implemented radial gradients multiple times...)
> > 
> > I am by no means doubting that your code is wrong, i am simply expressing
> > that i prefer to go step by step. I don't want you to take this as any
> > offense because it is not meant to be.
> > 
> >> I did not test them as extensively as the current Thomas's patch has
> >> been tested, but I think that the same objection would be apply to the
> >> cleaned up patch (which would also require additional work).
> > 
> > The cleanedup up patch can be regtested automatically, i just need to
> > check that poppler with the cleaned up patch generates exactly the same
> > png than with the non cleanup of patch for all the files. Testing yours
> > will require me to spend again 4 hours looking at all the files that
> > have an axial shading and making sure the changes are improvements and
> > not regressions, i hope you can understand it's a different amount of
> > work.
> 
> The axial code is not being modified, it is just being moved around.
> 
> The radial code should return the same images as Thomas patch
> except for the RADIAL_EPSILON, which I set up as a "round"
> number.
> 
> If you plan on performing automated regtesting, please apply 0003 as
> well and define RADIAL_EPSILON to be the same as in Thomas
> patch (1.192092896e-07F).
> This should be sufficient to guarantee that the code generates
> exactly the same images as the latest patch by Thomas (assuming
> that SPLASH_RADIAL_USEBITMAP was not defined).
> 
> 0003 obviously cannot be committed as-is, it is in bad need of a better
> commit message.

Ok, you got me, i'll regtest your patch. Please talk with Tomas to get a 
better understanding of why we need 0003 and what it does :-)

Albert

> 
> Andrea
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler