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

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55119|review?                     |review-
               Flag|                            |




--- Comment #24 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-07 07:11:01 PST ---
(From update of attachment 55119)
WebCore/svg/graphics/filters/SVGDistantLightSource.h:41
 +          virtual void initVectors(PaintVectors*);
I'd name it initLightVectors, the name is too general for my taste.
PaintVectors doesn't sound too great as well, though it's a subclass of
LightSource so it's fine I guess.

WebCore/svg/graphics/filters/SVGDistantLightSource.h:42
 +          virtual void updateVectors(PaintVectors*, int, int, float);
Can you please add the parameter names, int, int, float is too confusing.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:32
 +  FEDiffuseLighting::FEDiffuseLighting(FilterEffect* in, const Color&
lightingColor, const float& surfaceScale,
Not introduced by you, but I think we should really use "float" here, not
"const float&", there's almost no gain, and WebKit in general does not pass POD
types as const-references.

WebCore/svg/graphics/filters/SVGFELighting.cpp:40
 +  FELighting::FELighting(LightingType lightingType, FilterEffect* in, const
Color& lightingColor, const float& surfaceScale,
Ditto.

WebCore/svg/graphics/filters/SVGFELighting.cpp:57-58
 + #define UPLEFT \
 +      static_cast<int>(pixels->get(offs - widthM4 - 4 + 3))
I dislike these macros. I see the idea, but how about adding static inlines
like this:
static inline int upLeftPixelValue(LightingData& data)
{
    return static_cast<int>(pixels->get(data.offset -
data.widthMultipledFourTimes - 4 + 3);
}

LightingData could be a simple struct, constructed in the drawLighing function,
just holding a pointer to the CanvasPixelArray, and the variables you need in
the process.
This would make the code more readable I think, and it's easier to spot
compilation errors etc.
Do you think it would be too much overhead? I'd think the compiler would
generate the same code, if you'd define the functions as static inlines,
instead of using macros.

Same for all the other macros.

WebCore/svg/graphics/filters/SVGFELighting.cpp:76
 +  #define SETPIXEL(lightX, lightY, factorX, normalX, factorY, normalY) \
SETPIXEL should also be a static inline helper function, taking the
LightingData structure.
(LightingData also needs to hold the normalVectors variable, of course, forgot
to mention above).

WebCore/svg/graphics/filters/SVGFELighting.cpp:78
 +      normalVector.setX((factorX) * static_cast<float>(normalX) *
surfaceScale); \
Unneeded parenthesis around factorX.

WebCore/svg/graphics/filters/SVGFELighting.cpp:79
 +      normalVector.setY((factorY) * static_cast<float>(normalY) *
surfaceScale); \
Ditto.

WebCore/svg/graphics/filters/SVGFELighting.cpp:107
 +      // TODO: do something if width or height is 1 pixel. Now the filter
does nothing
Name it FIXME, this is prefereed WebKit style.

WebCore/svg/graphics/filters/SVGFELighting.cpp:114
 +      int widthM4 = width << 2;
Needs a better name, maybe the explicit "withMultipliedByFour".

WebCore/svg/graphics/filters/SVGFELighting.cpp:115
 +      int widthS1 = width - 1;
Same here.

WebCore/svg/graphics/filters/SVGFELighting.cpp:116
 +      int heightS1 = height - 1;
Same here.

WebCore/svg/graphics/filters/SVGFELighting.cpp:117
 +      int offs;
Please name it "offset", no abbrevations in WebKit code.

WebCore/svg/graphics/filters/SVGFELighting.cpp:193
 +               pixels->set(i, (unsigned char)0xff);
Please use  c style cast, or maybe even better a "static unsigned char
s_fooColorValue = 0xff" outside the loop to save the cast every time in the for
loop.

WebCore/svg/graphics/filters/SVGFELighting.cpp:194
 +      else
A parenthesis should be added around the block below. else { ... }

WebCore/svg/graphics/filters/SVGFELighting.cpp:199
 +               pixels->set(i + 3, a1 >= a2 ? (a1 >= a3 ? a1 : a3) : (a2 >= a3
? a2 : a3));
I'd love a comment here, because it's not clear for me on first sight what's
happening :-) Please use more explicit variable names (aka. longer &
descriptive).

WebCore/svg/graphics/filters/SVGFELighting.cpp:211
 +      if (!getEffectContext())
Ewww, getEffectContext() - who named it this way? Dirk? ;-)

WebCore/svg/graphics/filters/SVGFELighting.h:59
 +          LightingType m_lightingType;
I'd reorder the variables here, sorted by type, to possibly allow the compiler
to pad better.

WebCore/svg/graphics/filters/SVGLightSource.cpp:35
 +  #define M_PI 3.14159265358979323846
No need to define M_PI and include math.h. Just include wtf/MathExtras.h, it
contains a piFloat gobal variable.

WebCore/svg/graphics/filters/SVGLightSource.cpp:38
 +  static const double degToRad = M_PI / 180.0f;
Also available in MathExtras.h, as inlined helper function deg2Rad.

WebCore/svg/graphics/filters/SVGLightSource.cpp:94
 +      float ls = paintVectors->lightVector * paintVectors->directionVector;
Please use a more descriptive variable name.

WebCore/svg/graphics/filters/SVGLightSource.cpp:104
 +      float p;
Same here.

Other than those simple comments, an _excellent_ patch, good job Zoltan!
Looking forward to the revised version.

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