Fix consistency between Viewport and BI for Vector and RGB curves nodes
Needs RevisionPublic
Actions

Authored by Alexander Romanov (a.romanov) on Feb 26 2016, 12:53 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Mike Erwin (merwin)
Sergey Sharybin (sergey)

Summary

This patch has the following fixes:

Vector Curves BI: enabling factor influence
Vector Curves Viewport: space convertion to allow negative values in GLSL texture
RGB Curves BI: value clamping after mapping to obtain the result with "color" type. I think, it should be done in cycles as well.

Test file:

curves bug.blend1 MBDownload

Alexander (Blend4Web Team)

Diff Detail

Event Timeline

Alexander Romanov (a.romanov) updated this revision to Diff 6175.Feb 26 2016, 12:53 PM

Alexander Romanov (a.romanov) retitled this revision from to Fix consistency between Viewport and BI for Vector and RGB curves nodes.

Alexander Romanov (a.romanov) updated this object.

Alexander Romanov (a.romanov) updated this revision to Diff 6176.Feb 26 2016, 12:56 PM

Alexander Romanov (a.romanov) added a parent revision: D1672: Fix T46915: Non-intuitive behavior of Vector Curve Mapping node.

Alexander Romanov (a.romanov) updated this object.Feb 26 2016, 1:06 PM

Alexander Romanov (a.romanov) added reviewers: Sergey Sharybin (sergey), Brecht Van Lommel (brecht).

Alexander Romanov (a.romanov) added subscribers: Yuri Kovelenov (yurikovelenov), Alexander Kovelenov (AlexKowel), Evgeny_Rodygin.

Alexander Romanov (a.romanov) updated this object.Apr 13 2016, 5:13 PM

Alexander Romanov (a.romanov) added a reviewer: Mike Erwin (merwin).Apr 13 2016, 5:18 PM

It would be great if someone looked at the patch. This is not just improvement, this is bug fixing.

If we want negative values for the curve mapping, isn't the better solution to use a float or half float OpenGL texture? Extending the range from 0..1 to -1..1 is ok but doesn't solve the issue entirely.

source/blender/gpu/shaders/gpu_shader_material.glsl
421	Should be `2.0 * outvec - vec3(1.0)`, should not use integers or float postfix.
source/blender/nodes/shader/nodes/node_shader_curves.c
116–118	I can see the argument for clamping negative colors, though we generally don't do that in other color nodes I think? HDR color values > 1 should definitely not be clamped like this though, they are valid colors.

This revision now requires changes to proceed.Apr 13 2016, 10:47 PM

In D1827#43801, @Brecht Van Lommel (brecht) wrote:

If we want negative values for the curve mapping, isn't the better solution to use a float or half float OpenGL texture? Extending the range from 0..1 to -1..1 is ok but doesn't solve the issue entirely.

You mean to increase CM_TABLE to max value of half float to make it possible to extrapolate curve, how it is done for BI? Or just modify bounds in curvemapping_add? You want to change the range for the argument of the function or for the value?

Alexander Romanov (a.romanov) added inline comments.Apr 15 2016, 5:28 PM

source/blender/nodes/shader/nodes/node_shader_curves.c
116–118	I didn't think about HDR colors. It seems to me If you remove clamping than one of Curves node should be removed. What is the difference between them originally? RGB curves seems more generic

In D1827#43915, @Alexander Romanov (a.romanov) wrote:

You mean to increase CM_TABLE to max value of half float to make it possible to extrapolate curve, how it is done for BI? Or just modify bounds in curvemapping_add? You want to change the range for the argument of the function or for the value?

Ah, I was thinking about the vertical range, since that's what this patch seems to address. GPU_texture() could get an extra GPUHDRType hdr argument for that.

For the horizontal range it's indeed more complicated. Cycles computes the min and max X value, and rescales the input values with that, and there's also code for extrapolation. GLSL could do something similar. This patch doesn't have to solve that issue, just for the vertical range I'd prefer a HDR texture as a solution rather than adding more assumptions about the range being limited to -1..1.

source/blender/nodes/shader/nodes/node_shader_curves.c
116–118	I'm not sure about the original intent. A node specifically for the two data types might be easier to understand for users? I'm guessing the vector curves node is not used much though. The Mix node has a Clamp option, which could be a good feature to add to the RGB curves node. But I don't think we can just do that always, it's breaking backwards compatibility, so we need an option for it if you want to change this.