A Dimarts, 25 de gener de 2011, Andrea Canciani va escriure: > On Tue, Jan 25, 2011 at 11:38 PM, Albert Astals Cid <[email protected]> wrote: > > A Dimarts, 25 de gener de 2011, vàreu escriure: > >> On Tue, Jan 25, 2011 at 10:54 PM, Albert Astals Cid <[email protected]> wrote: > >> > A Dimarts, 25 de gener de 2011, vàreu escriure: > >> >> On Tue, Jan 25, 2011 at 10:25 PM, Albert Astals Cid <[email protected]> > > > > wrote: > >> >> > A Dimarts, 25 de gener de 2011, Andrea Canciani va escriure: > >> >> >> On Tue, Jan 25, 2011 at 9:00 PM, Albert Astals Cid <[email protected]> > > > > 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/poppler _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
