Bug 5862 – feDisplacementMap filter is not implemented

• Status: CLOSED
• Resolution: FIXED
• Component: SVG

[Oliver Hunt]

[Oliver Hunt]

Created attachment 6352 [details]
First patch for displacement mapping

Okay this is the first version of the feDisplacementMap implementation.  Filter should be correct (per logic + quartz composer) however it's difficult to validate given test case (http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-filters-displace-01-f.html) fails to render the inputs correctly, let alone the output.  That said sphere map appears to be doing correct thing on page.

[Oliver Hunt]

Created attachment 6353 [details]
First patch for displacement mapping

Okay this is the first version of the feDisplacementMap implementation.  Filter should be correct (per logic + quartz composer) however it's difficult to validate given test case (http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-filters-displace-01-f.html) fails to render the inputs correctly, let alone the output.  That said sphere map appears to be doing correct thing on page.

[Oliver Hunt]

Created attachment 6354 [details]
Formatting changes

Damn formatting changes

[Eric Seidel (no email)]

Comment on attachment 6354 [details]
Formatting changes

This patch looks fine.

Ideally, we would want to remove the data duplication in the kcanvas objects, by making the KCanvasFEDisplacementMap object just fetch its needed data off of it's element() instead of storing a second copy locally.

Also, there are a couple tabs in the cikernel file which need to be fixed before we land.

also XChannelSelector() and m_XChannelSelector should be xChannelSelector() and m_xChannelSelector per our style guidelines.  (Or just completely removed per my above comment)

It also doesn't look like KCanvasFEDisplacementMap has a default constructor which sets things to sane default values (also unecessary if its data members are removed)

getVectorForChannel could be done using an array of floats of 0's, followed by setting one at the correct offset to 1 and returning.  Your method is also totally fine.

Since you don't have commit bit, I'm going to mark this as r-.  If you did, it would be find to land, making those tweaks as you landed.  This does not need another review, but a final patch should be posted.

[Oliver Hunt]

Created attachment 6375 [details]
A few more formatting changes

[Eric Seidel (no email)]

Comment on attachment 6375 [details]
A few more formatting changes

Fabulous, as always. r=me.

[Oliver Hunt]

Created attachment 6417 [details]
Fixed filter kernel

oops... correct the kernel so it works when the dispalcement map isn't completely symmetrical :)

[Eric Seidel (no email)]

Comment on attachment 6417 [details]
Fixed filter kernel

great!

[Eric Seidel (no email)]

Comment on attachment 6417 [details]
Fixed filter kernel

Hum... doens't apply cleanly though.  Let's get one more patch.  And I'll land your test case at that time too.

[Oliver Hunt]

Created attachment 6418 [details]
Updated to patch against ToT

[Oliver Hunt]

Created attachment 6419 [details]
Testcase

[Oliver Hunt]

Created attachment 6420 [details]
displacement map for testcase

[Oliver Hunt]

Created attachment 6421 [details]
Source image for testcase

[Eric Seidel (no email)]

Comment on attachment 6418 [details]
Updated to patch against ToT

Looks good.  Lets land this.