Am 26.01.2011 09:57, schrieb Albert Astals Cid:
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.
I think, You mean 0005, isn't it? Here the reason for the changes
a) SplashOutputDev::beginTransparencyGroup
The wrong setup of tx and ty caused segmentation faults in
Splash::blitTransparent()
b) Splash::pipeRun
If getColor return gFalse, we need to increment ALL pointers, use now
the already existing method pipeIncX
c) Splash::shadedFill
shadedFill is now used by axial and radial shading, axial supports
antialiasing, radial not. Therefore we need to support either case in it.
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]
Okay, that's obviously. This PDF was the reason, why I removed
antialiasing again in radial shading. Andrea hadn't known that, and
therefore probably missed it.
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.
If You agree, give Andrea's patch a final chance (I had a lot of chances
:- ) ). Unfortunately I'm not able to make this small patch pieces
today, so here just the correction at end of SplashOutputDev.cc:
>>> code snippet start
GBool SplashOutputDev::univariateShadedFill(GfxState *state,
SplashUnivariatePattern *pattern, double tMin, double tMax) {
double xMin, yMin, xMax, yMax;
SplashPath *path;
GBool retVal = gFalse;
// get the clip region bbox
state->getUserClipBBox(&xMin, &yMin, &xMax, &yMax);
// fill the region
state->moveTo(xMin, yMin);
state->lineTo(xMax, yMin);
state->lineTo(xMax, yMax);
state->lineTo(xMin, yMax);
state->closePath();
path = convertPath(state, state->getPath());
retVal = (splash->shadedFill(path,
pattern->getShading()->getHasBBox(), pattern) == splashOk);
state->clearPath();
delete path;
return retVal;
}
GBool SplashOutputDev::axialShadedFill(GfxState *state, GfxAxialShading
*shading, double tMin, double tMax) {
GBool vaa = getVectorAntialias();
// restore vector antialias because we support it here
setVectorAntialias(gTrue);
SplashAxialPattern *pattern = new SplashAxialPattern(colorMode,
state, shading);
GBool retVal = univariateShadedFill(state, pattern, tMin, tMax);
setVectorAntialias(vaa);
delete pattern;
return retVal;
}
GBool SplashOutputDev::radialShadedFill(GfxState *state,
GfxRadialShading *shading, double tMin, double tMax) {
SplashRadialPattern *pattern = new SplashRadialPattern(colorMode,
state, shading);
GBool retVal = univariateShadedFill(state, pattern, tMin, tMax);
delete pattern;
return retVal;
}
<<< code snippet end.
Thomas
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
.
_______________________________________________
poppler mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/poppler
