A Dimecres, 26 de gener de 2011, Albert Astals Cid va escriure: > 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 :-)
Your patch regresses at altona_visual_1v2a_x3.pdf [1] The circles are not totally filled in their right border. I stopped having a look all at the other files in my regtest that change (so it might regress somewhere else) but makes no point having a look at all the other files since i'll have to do it anyway once you fix this regression. Now my question is if I should commit cleaned Thomas patch or not. Albert [1] http://www.eci.org/lib/exe/fetch.php?id=de%3Adownloads&cache=cache&media=downloads:altona_test_suite:altona_visual_1v2a_x3.pdf > > 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 _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
