[Webkit-unassigned] [Bug 32197] feDiffuseLighting filter is not implemented

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32197





--- Comment #30 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-11 08:05:36 PST ---
(In reply to comment #29)
> Thank you. I only have a few questions, where the answer is not obvious (for me at :) )
> 
> > WebCore/svg/graphics/filters/SVGFEDiffuseLighting.h:34
> >  +          static PassRefPtr<FEDiffuseLighting> create(FilterEffect*, const Color&, const float, const float,
> > Ditto. NOTE: Please leave "const Color&" - non-POD types should still be passed as const-refererences, only POD types like float/int/etc.. not.
> 
> There is no change here, isn't it? It has already been defined as "const Color&". Passing pointers is faster, of course.
Right, just wanted to mention that you shouldn't change that one :-)


> > WebCore/svg/graphics/filters/SVGFELighting.cpp:164
> >  +      data.lightingType = m_lightingType;
> > Hmm, isn't it possible to just store a pointer to the current FELighting object in LightingData? Otherwhise you're duplicating the storage of lightingType/lightSsource/diffuseConstant/etc..
> 
> The inline functions are not friend functions of FELighting, thus the protected members are not accessible. There are several possible workaround for that, and I chose the simplest one (without header modification). Shall I make them a member function? Or friend function (moving LightingData inside FELighting)? Or something else?
Friendship sounds the fastest solution to me, choose whatever you like, if it avoids duplicating memory.

> 
> > WebCore/svg/graphics/filters/SVGFELighting.cpp:250
> >  +              data.pixels->set(i, static_cast<unsigned char>(0xff));
> > Please move the magic 0xff value to a constant out-of-the-loop "cMyColorValue" or something. To avoid doing this cast on every iteration of the loop.
> 
> You mentioned this before, but "static_cast<unsigned char>(0xff)" is a compile time constant, and there is no runtime casting. But I could move it out anyway.
Oh that's true of course. Hm, I still find it easier to read.

> 
> > 
> > WebCore/svg/graphics/filters/SVGFELighting.h:41
> >  +      class CanvasPixelArray;
> > Indention is wrong in this file, code inside namespace should not be indented.
> 
> The style checker mention this as well, but most classes in WebKit is indented in the Header file, including other headers in the same directory.
Yeah it's a "new rule" compared to when this code appeared in trunk. We're fixing them over the time whenver we see the style bot complain.

> 
> > Hm, could you explain again why x/y are int and float is z? I may have overlooked the obvious reason.
> 
> I will put a comment there. z = alpha value (opaque treated as 1.0) scaled by a user specified constant. That constant is a float.
Good.

> 
> 
> Next time I will add a ChangeLog and update mac pixel tests as well.
Excellent!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.