Page MenuHome

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

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

Details

Summary

This patch has the following fixes:

  1. Vector Curves BI: enabling factor influence
  2. Vector Curves Viewport: space convertion to allow negative values in GLSL texture
  3. 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:


Alexander (Blend4Web Team)

Diff Detail

Event Timeline

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.

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

Brecht Van Lommel (brecht) requested changes to this revision.Apr 13 2016, 10:47 PM
Brecht Van Lommel (brecht) edited edge metadata.

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

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?

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

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.