Details

Reviewers

Brecht Van Lommel (brecht)
Thomas Dinges (dingto)

Commits

rBSdd2dca2f7e77: Add support for multiple interpolation modes on cycles image textures
rC6df828e355b0: Add support for multiple interpolation modes on cycles image textures
rBACdd2dca2f7e77: Add support for multiple interpolation modes on cycles image textures
rBdd2dca2f7e77: Add support for multiple interpolation modes on cycles image textures

Summary

All textures are sampled bi-linear currently with the exception of OSL there texture sampling is fixed and set to smart bi-cubic.

This patch adds user control to this setting.

Added:

bits to DNA / RNA in the form of an enum for supporting multiple interpolations types
changes to the image texture node drawing code ( add enum)
to ImageManager (this needs to know to allocate second texture when interpolation type is different)
to node compiler (pass on interpolation type)
to device tex_alloc this also needs to get the concept of multiple interpolation types
implementation for doing non interpolated lookup for cuda and cpu
implementation where we pass this along to osl ( this makes OSL also do linear untill I add smartcubic to the interface / DNA/ RNA)

Todo:

Test CUDA image packing + interpolation
OpenCL memory statistics not updated correctly on realloc of texture
verify / test / polist

Possible:

Extend cycles device to be able to report if device needs allocation time interpolation or can handle runtime interpolation
Add OIIO texture backend to SVM CPU codepath and split OIIO / OSL assumtions in cycles code so OSL implies OIIO but not the other way around

Outstanding questions:

Should I just map Linear to smart for OSL backend ?
Should I implement bicubic and smart ?

Diff Detail

Repository: rB Blender
Branch: arcpatch-D317

Event Timeline

Added basic implementation for the cpu device

Ohh and it works

Added cuda support and expanded enum to include a not a texture option to accommodate our using of texture ram for non texture data on GPU's

Martijn Berger (juicyfruit) updated this revision to Unknown Object (????).Feb 14 2014, 11:47 PM

cleaned up changes to nodes.cpp
added support for OSL

I would like feedback on changes to the texture node

And I think we might need bicubic and smart for svm to.
left is smart filtering ( equal to bi-cubic bspline for this case) right is bi-linear

Added smart and cubic and code to fall back onto linear if not supported

re-apply to master

allow tex_alloc of already alloced texture, no elegant and should most likely fix device_opencl but it works

allow tex_alloc of already alloced texture, no elegant and should most likely fix device_opencl but it works
added opencl nearest interpolation

Some comments, mainly code style.

intern/cycles/device/device.h
50	Would rename to "NO_TEXTURE", more correct grammar wise. Or we invert the logic and call it "DATA_TEXTURE" or so. I leave the decision to Brecht. :)
intern/cycles/device/device_cuda.cpp
508	Code Style, "else if" should go to a new line.
intern/cycles/device/device_opencl.cpp
890	Newline not needed.
intern/cycles/kernel/kernel_compat_cpu.h
120	Code Style (Spaces)
127	"else" in new line.
intern/cycles/kernel/svm/svm_image.h
86 ↗	(On Diff #1195)	Code Style (Spaces)

Tested with CPU (SVM and OSL) and GPU (CUDA and OpenCL). Functionality wise this works fine.

As already discussed with Martijn in IRC, I would rename the UI names though, to have the "(OSL only)" in the name, rather than in the tooltip, this communicates it better.

Apart from that and the code style, I am fine with it. Good job!

Looks quite good, some comments.

intern/cycles/device/device.h
49	Can this be moved to kernel_types.h, so we can use it instead of int everywhere?
50	I would call it INTERPOLATION_NONE, since as far as I can tell this value disables interpolation.
intern/cycles/device/device_cpu.cpp
108–109	This should make cubic and smart fallback to linear.
intern/cycles/device/device_opencl.cpp
886–894	Why is this needed? Is it when you have a texture with the same name but different interpolation? We should be able to handle that case correctly.
intern/cycles/render/nodes.cpp
370	This should not be static. Can be const if you want, but no reason to have it static. If remove_image accepts InterpolationType it can just pass INTERPOLATION_NONE directly though.
404	Same comment as above.
source/blender/makesdna/DNA_node_types.h
953–956	Can you change INTER to INTERP? Is the more common abbreviation I think.

fixes based on reviews by brecht and DingTo

Looks good to me, just one minor comment.

intern/cycles/render/image.cpp
666	Better use `uint8_t` instead, that's the one we use elsewhere and ensure is available.

Wait just one second. I also fixed a bug and removed work around in device_opencl.

Packed image textures where just allocated again over the previous one. even without this patch.

intern/cycles/device/device.h
49	Well I am not sure about including any kernel_*** in device. Maybe in util_types.h although that is also probably not the best place. It is something that we want to access both above and blow device level.
intern/cycles/device/device_opencl.cpp
886–894	Changing the interpolation changes the texture map and that reallocates the texture that holds all the image textures. But the map we keep is not designed for that and that will hit the assert and fail. I think it can be done better

removed stupid device_opencl workaround and fixed bug where packed texture was not freed before reallocating

Closed by commit rBdd2dca2f7e77 (authored by @Martijn Berger (juicyfruit)).

Add support for multiple interpolation modes on cycles image textures
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1163

intern/cycles/blender/blender_shader.cpp

intern/cycles/device/device.h

intern/cycles/device/device_cpu.cpp

intern/cycles/device/device_cuda.cpp

intern/cycles/device/device_multi.cpp

intern/cycles/device/device_network.cpp

intern/cycles/device/device_opencl.cpp

intern/cycles/kernel/kernel.h

intern/cycles/kernel/kernel.cpp

intern/cycles/kernel/kernel_compat_cpu.h

intern/cycles/kernel/shaders/node_image_texture.osl

intern/cycles/render/image.h

intern/cycles/render/image.cpp

intern/cycles/render/nodes.h

intern/cycles/render/nodes.cpp

source/blender/editors/space_node/drawnode.c

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/intern/rna_nodetree.c

Add support for multiple interpolation modes on cycles image texturesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1163

intern/cycles/blender/blender_shader.cpp

intern/cycles/device/device.h

intern/cycles/device/device_cpu.cpp

intern/cycles/device/device_cuda.cpp

intern/cycles/device/device_multi.cpp

intern/cycles/device/device_network.cpp

intern/cycles/device/device_opencl.cpp

intern/cycles/kernel/kernel.h

intern/cycles/kernel/kernel.cpp

intern/cycles/kernel/kernel_compat_cpu.h

intern/cycles/kernel/shaders/node_image_texture.osl

intern/cycles/render/image.h

intern/cycles/render/image.cpp

intern/cycles/render/nodes.h

intern/cycles/render/nodes.cpp

source/blender/editors/space_node/drawnode.c

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/intern/rna_nodetree.c

Add support for multiple interpolation modes on cycles image textures
ClosedPublic
Actions

Revision Contents
Changeset List