Displace Node

I had to make it into a Complex node to allow the readEWA to be used.
(the other alternative is to expose readEWA to all node types when dx and dy are passed as parameter to read).

The result is almost perfect (comparing to trunk). The only problem is a glitch in the middle:

trunk: http://www.pasteall.org/pic/show.php?id=24506
tile: http://www.pasteall.org/pic/show.php?id=24505

I'm yet to investigate the reason. It may be the readEWA code (which is also used by the MapUV node, but works fine there).

edit: committed

[xavier.thomas.1980_gmail.com, 2012-01-17 22:44:54]

It might be a "Off by one" bug less noticeable in the non tiled pipeline.

2012/1/16 <dfelinto@gmail.com>

> Reviewers: bf-codereview_blender.org, jbkkavt,
>
> Description:
> I had to make it into a Complex node to allow the readEWA to be used.
> (the other alternative is to expose readEWA to all node types when dx
> and dy are passed as parameter to read).
>
> The result is almost perfect (comparing to trunk). The only problem is a
> glitch in the middle:
>
> trunk: http://www.pasteall.org/pic/show.php?id=24506
> tile: http://www.pasteall.org/pic/show.php?id=24505
>
> I'm yet to investigate the reason. It may be the readEWA code (which is
> also used by the MapUV node, but works fine there).
>
> Please review this at http://codereview.appspot.com/5528108/
>
> Affected files:
>   source/blender/compositor/CMakeLists.txt
>   source/blender/compositor/intern/COM_Converter.cpp
>   source/blender/compositor/nodes/COM_DisplaceNode.cpp
>   source/blender/compositor/nodes/COM_DisplaceNode.h
>   source/blender/compositor/operations/COM_DisplaceOperation.cpp
>   source/blender/compositor/operations/COM_DisplaceOperation.h
>
>
> _______________________________________________
> Bf-codereview mailing list
> Bf-codereview@blender.org
> http://lists.blender.org/mailman/listinfo/bf-codereview
>

[dfelinto, 2012-01-17 23:04:15]

If I change the render output to 200% I see the 'problem' happening again in the
quarters of the screen. (so as marking the boundary of some tiles).

So what I may need to do is to have a initializeTileData for this operation. I
haven't defined that in any previous nodes so I need to investigate if that's
the problem.

On 2012/01/17 22:44:54, xavier.thomas.1980_gmail.com wrote:
> It might be a "Off by one" bug less noticeable in the non tiled pipeline.
> 
> 2012/1/16 <mailto:dfelinto@gmail.com>
> 
> > Reviewers: http://bf-codereview_blender.org, jbkkavt,
> >
> > Description:
> > I had to make it into a Complex node to allow the readEWA to be used.
> > (the other alternative is to expose readEWA to all node types when dx
> > and dy are passed as parameter to read).
> >
> > The result is almost perfect (comparing to trunk). The only problem is a
> > glitch in the middle:
> >
> > trunk: http://www.pasteall.org/pic/show.php?id=24506
> > tile: http://www.pasteall.org/pic/show.php?id=24505
> >
> > I'm yet to investigate the reason. It may be the readEWA code (which is
> > also used by the MapUV node, but works fine there).
> >
> > Please review this at http://codereview.appspot.com/5528108/
> >
> > Affected files:
> >   source/blender/compositor/CMakeLists.txt
> >   source/blender/compositor/intern/COM_Converter.cpp
> >   source/blender/compositor/nodes/COM_DisplaceNode.cpp
> >   source/blender/compositor/nodes/COM_DisplaceNode.h
> >   source/blender/compositor/operations/COM_DisplaceOperation.cpp
> >   source/blender/compositor/operations/COM_DisplaceOperation.h
> >
> >
> > _______________________________________________
> > Bf-codereview mailing list
> > mailto:Bf-codereview@blender.org
> > http://lists.blender.org/mailman/listinfo/bf-codereview
> >

[jbkkavt, 2012-01-18 13:56:08]

I have no comments on the code structure. soo when the functional part is solved
you can commit.

Jeroen Bakker
 - At Mind -

[dfelinto, 2012-01-18 16:18:04]

Any clues on what can cause that problem?
Or any tips on when to use InitilizeTileData?

--
Dalai

On 2012/01/18 13:56:08, jbkkavt wrote:
> I have no comments on the code structure. soo when the functional part is
solved
> you can commit.
> 
> Jeroen Bakker
>  - At Mind -

[jbkkavt, 2012-01-18 18:23:12]

I haven't look at the issue (or even) what the issue is about.

InitializeTileData is used to cache data that is constant for a single
tile, but not for the whole image.

It is also used when you need all pixels of an input to calculate a value
(like average color) that is used in the execute pixel.

Jeroen.

On Wed, Jan 18, 2012 at 5:18 PM, <dfelinto@gmail.com> wrote:

> Any clues on what can cause that problem?
> Or any tips on when to use InitilizeTileData?
>
> --
> Dalai
>
>
> On 2012/01/18 13:56:08, jbkkavt wrote:
>
>> I have no comments on the code structure. soo when the functional part
>>
> is solved
>
>> you can commit.
>>
>
>  Jeroen Bakker
>>  - At Mind -
>>
>
>
>
>
http://codereview.appspot.com/**5528108/<http://codereview.appspot.com/5528108/>
>



-- 
Jeroen Bakker

[jbkkavt, 2012-01-24 15:34:17]

Dalai, 

please check. Using you example file I couldn't reproduce it anymore. It is a
rounding issue with the +0.5f isplaced pixel in uv coords, for image sampling

I tested with values from -100 to +100 for Xscale and Yscale.

Regards, 
Jeroen

http://codereview.appspot.com/5528108/diff/1/source/blender/compositor/operat...
File source/blender/compositor/operations/COM_DisplaceOperation.cpp (right):

http://codereview.appspot.com/5528108/diff/1/source/blender/compositor/operat...
source/blender/compositor/operations/COM_DisplaceOperation.cpp:130:
vectorInput.xmax = input->xmax + 1;
Change +1 to +2 solves some rounding issues in determination of the uv
coordinates

http://codereview.appspot.com/5528108/diff/1/source/blender/compositor/operat...
source/blender/compositor/operations/COM_DisplaceOperation.cpp:132:
vectorInput.ymax = input->ymax + 1;
Change +1 to +2 solves some rounding issues in determination of the uv
coordinates

[dfelinto, 2012-01-24 17:03:52]

It seems to be good now. If we have other reports in the future we know where to
tweak at least (and I'm glad the algorithm is working).

thanks Jeroen, I just committed.


On 2012/01/24 15:34:17, jbkkavt wrote:
> Dalai, 
> 
> please check. Using you example file I couldn't reproduce it anymore. It is a
> rounding issue with the +0.5f isplaced pixel in uv coords, for image sampling
> 
> I tested with values from -100 to +100 for Xscale and Yscale.
> 
> Regards, 
> Jeroen
> 
>
http://codereview.appspot.com/5528108/diff/1/source/blender/compositor/operat...
> File source/blender/compositor/operations/COM_DisplaceOperation.cpp (right):
> 
>
http://codereview.appspot.com/5528108/diff/1/source/blender/compositor/operat...
> source/blender/compositor/operations/COM_DisplaceOperation.cpp:130:
> vectorInput.xmax = input->xmax + 1;
> Change +1 to +2 solves some rounding issues in determination of the uv
> coordinates
> 
>
http://codereview.appspot.com/5528108/diff/1/source/blender/compositor/operat...
> source/blender/compositor/operations/COM_DisplaceOperation.cpp:132:
> vectorInput.ymax = input->ymax + 1;
> Change +1 to +2 solves some rounding issues in determination of the uv
> coordinates