Making the Hue Correct Curves Wrap #117114

Merged
Omar Emara merged 31 commits from JonasDichelle/blender:hue-correct-bezier-wrap into main 2024-03-21 15:35:13 +01:00
8 changed files with 133 additions and 36 deletions

View File

@ -29,7 +29,7 @@ extern "C" {
/* Blender file format version. */
#define BLENDER_FILE_VERSION BLENDER_VERSION
#define BLENDER_FILE_SUBVERSION 11
#define BLENDER_FILE_SUBVERSION 12
/* Minimum Blender version that supports reading file written with the current
* version. Older Blender versions will test this and cancel loading the file, showing a warning to

View File

@ -168,6 +168,7 @@ void BKE_curvemapping_premultiply(CurveMapping *cumap, bool restore);
void BKE_curvemapping_blend_write(BlendWriter *writer, const CurveMapping *cumap);
void BKE_curvemapping_curves_blend_write(BlendWriter *writer, const CurveMapping *cumap);
/**
* \note `cumap` itself has been read already.
*/

View File

@ -294,8 +294,8 @@ void BKE_curvemap_reset(CurveMap *cuma, const rctf *clipr, int preset, int slope
case CURVE_PRESET_MAX:
cuma->totpoint = 2;
break;
case CURVE_PRESET_MID9:
cuma->totpoint = 9;
case CURVE_PRESET_MID8:
cuma->totpoint = 8;
break;
case CURVE_PRESET_ROUND:
cuma->totpoint = 4;
@ -363,9 +363,9 @@ void BKE_curvemap_reset(CurveMap *cuma, const rctf *clipr, int preset, int slope
cuma->curve[1].x = 1;
cuma->curve[1].y = 1;
break;
case CURVE_PRESET_MID9: {
case CURVE_PRESET_MID8: {
for (int i = 0; i < cuma->totpoint; i++) {
cuma->curve[i].x = i / (float(cuma->totpoint) - 1);
cuma->curve[i].x = i / (float(cuma->totpoint));
cuma->curve[i].y = 0.5;
}
break;
@ -637,6 +637,17 @@ static float curvemap_calc_extend(const CurveMapping *cumap,
return 0.0f;
}
/* Evaluates CM_RESOL number of points on the Bezier segment defined by the given start and end
* Bezier triples, writing the output to the points array. */
static void curve_eval_bezier_point(float start[3][3], float end[3][3], float *point)
{
BKE_curve_correct_bezpart(start[1], start[2], end[0], end[1]);
BKE_curve_forward_diff_bezier(
JonasDichelle marked this conversation as resolved Outdated

The comment doesn't seem accurate, since we are evaluating many points. I would rename bezt1 and bezt2 to start and end, and change their type to BezTriple. I would also rename the final argument to points to reflect that they are actually many points. Then update the comment as follows:

/* Evaluates CM_RESOL number of points at the Bezier segment defined by the given start and end
 * Bezier triples, writing the output to the points array. */

The comment doesn't seem accurate, since we are evaluating many points. I would rename `bezt1` and `bezt2` to `start` and `end`, and change their type to `BezTriple`. I would also rename the final argument to `points` to reflect that they are actually many points. Then update the comment as follows: ``` /* Evaluates CM_RESOL number of points at the Bezier segment defined by the given start and end * Bezier triples, writing the output to the points array. */ ```
start[1][0], start[2][0], end[0][0], end[1][0], point, CM_RESOL - 1, sizeof(float[2]));
JonasDichelle marked this conversation as resolved Outdated

Drop the BKE prefix, since this is a static function and not part of a public API.

Drop the BKE prefix, since this is a static function and not part of a public API.
BKE_curve_forward_diff_bezier(
start[1][1], start[2][1], end[0][1], end[1][1], point + 1, CM_RESOL - 1, sizeof(float[2]));
}
/* only creates a table for a single channel in CurveMapping */
static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
{
@ -644,6 +655,13 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
CurveMapPoint *cmp = cuma->curve;
BezTriple *bezt;
/* Wrapping ensures that the heights of the first and last points are the same. It adds two
* virtual points, which are copies of the first and last points, and moves them to the opposite
* side of the curve offset by the table range. The handles of these points are calculated, as if
* they were between the last and first real points. */
const bool use_wrapping = cumap->flag & CUMA_USE_WRAPPING;
JonasDichelle marked this conversation as resolved Outdated

Comment lines should start with a start per the style guide.
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#cc-comments

/* Wrapping ensures that the heights of the first and last points are the same. It adds two virtual
 * points, which are copies of the first and last points, and moves them to the opposite side of the
 * curve offset by the table range. The handles of these points are calculated, as if they were
 * between the last and first real points. */
Comment lines should start with a start per the style guide. https://developer.blender.org/docs/handbook/guidelines/c_cpp/#cc-comments ``` /* Wrapping ensures that the heights of the first and last points are the same. It adds two virtual * points, which are copies of the first and last points, and moves them to the opposite side of the * curve offset by the table range. The handles of these points are calculated, as if they were * between the last and first real points. */ ```
if (cuma->curve == nullptr) {
return;
JonasDichelle marked this conversation as resolved Outdated

I would add a comment here that roughly describes the mechanism by which wrapping is implemented.

I would add a comment here that roughly describes the mechanism by which wrapping is implemented.
}
@ -651,6 +669,7 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
/* default rect also is table range */
cuma->mintable = clipr->xmin;
cuma->maxtable = clipr->xmax;
float table_range = cuma->maxtable - cuma->mintable;
/* Rely on Blender interpolation for bezier curves, support extra functionality here as well. */
bezt = static_cast<BezTriple *>(MEM_callocN(cuma->totpoint * sizeof(BezTriple), "beztarr"));
@ -671,16 +690,54 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
}
}
const BezTriple *bezt_next = nullptr;
const BezTriple *bezt_prev = nullptr;
/* Create two extra points for wrapping curves. */
BezTriple bezt_pre = bezt[cuma->totpoint - 1];
BezTriple bezt_post = bezt[0];
JonasDichelle marked this conversation as resolved Outdated

You can do something like, then take their address later.

  BezTriple virtual_start_bezier = bezt[cuma->totpoint - 1];
  virtual_start_bezier.vec[1][0] -= 1.0f;

  BezTriple virtual_end_bezier = bezt[0];
  virtual_end_bezier.vec[1][0] += 1.0f;
You can do something like, then take their address later. ``` BezTriple virtual_start_bezier = bezt[cuma->totpoint - 1]; virtual_start_bezier.vec[1][0] -= 1.0f; BezTriple virtual_end_bezier = bezt[0]; virtual_end_bezier.vec[1][0] += 1.0f; ```
BezTriple *bezt_post_ptr;
if (use_wrapping) {
/* Handle location of pre and post points for wrapping curves. */
bezt_pre.h1 = bezt_pre.h2 = bezt[cuma->totpoint - 1].h2;
bezt_pre.vec[1][0] = bezt[cuma->totpoint - 1].vec[1][0] - table_range;
bezt_pre.vec[1][1] = bezt[cuma->totpoint - 1].vec[1][1];
bezt_post.h1 = bezt_post.h2 = bezt[0].h1;
JonasDichelle marked this conversation as resolved Outdated

This assumes the curve map has a horizontal range of [0, 1]. The actual range is [cumap->clipr->xmin, cumap->clipr->ymin]. I am not sure how this should be handled, bur have you considered this?

This assumes the curve map has a horizontal range of `[0, 1]`. The actual range is `[cumap->clipr->xmin, cumap->clipr->ymin]`. I am not sure how this should be handled, bur have you considered this?

I assumed since this was only being used by hue correct that would fine.
But you're rigth, I replaced it with the correctly calculated table range now.

I assumed since this was only being used by hue correct that would fine. But you're rigth, I replaced it with the correctly calculated table range now.
bezt_post.vec[1][0] = bezt[0].vec[1][0] + table_range;
bezt_post.vec[1][1] = bezt[0].vec[1][1];
bezt_prev = &bezt_pre;
bezt_post_ptr = &bezt_post;
}
else {
bezt_prev = nullptr;
bezt_post_ptr = nullptr;
}
/* Process middle elements */
for (int a = 0; a < cuma->totpoint; a++) {
const BezTriple *bezt_next = (a != cuma->totpoint - 1) ? &bezt[a + 1] : nullptr;
bezt_next = (a != cuma->totpoint - 1) ? &bezt[a + 1] : bezt_post_ptr;
calchandle_curvemap(&bezt[a], bezt_prev, bezt_next);
bezt_prev = &bezt[a];
}
/* Correct handles of pre and post points for wrapping curves. */
bezt_pre.vec[0][0] = bezt[cuma->totpoint - 1].vec[0][0] - table_range;
bezt_pre.vec[0][1] = bezt[cuma->totpoint - 1].vec[0][1];
bezt_pre.vec[2][0] = bezt[cuma->totpoint - 1].vec[2][0] - table_range;
bezt_pre.vec[2][1] = bezt[cuma->totpoint - 1].vec[2][1];
bezt_post.vec[0][0] = bezt[0].vec[0][0] + table_range;
bezt_post.vec[0][1] = bezt[0].vec[0][1];
bezt_post.vec[2][0] = bezt[0].vec[2][0] + table_range;
bezt_post.vec[2][1] = bezt[0].vec[2][1];
/* first and last handle need correction, instead of pointing to center of next/prev,
* we let it point to the closest handle */
if (cuma->totpoint > 2) {
if (cuma->totpoint > 2 && !use_wrapping) {
float hlen, nlen, vec[3];
if (bezt[0].h2 == HD_AUTO) {
@ -719,35 +776,34 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
}
}
}
/* make the bezier curve */
if (cuma->table) {
MEM_freeN(cuma->table);
}
int totpoint = (cuma->totpoint - 1) * CM_RESOL;
int totpoint = use_wrapping ? (cuma->totpoint + 1) * CM_RESOL : (cuma->totpoint - 1) * CM_RESOL;
float *allpoints = static_cast<float *>(MEM_callocN(totpoint * 2 * sizeof(float), "table"));
float *point = allpoints;
JonasDichelle marked this conversation as resolved Outdated

Why separate declarations from initializations? Do the initialization directly.

Why separate declarations from initializations? Do the initialization directly.
for (int a = 0; a < cuma->totpoint - 1; a++, point += 2 * CM_RESOL) {
BKE_curve_correct_bezpart(
bezt[a].vec[1], bezt[a].vec[2], bezt[a + 1].vec[0], bezt[a + 1].vec[1]);
BKE_curve_forward_diff_bezier(bezt[a].vec[1][0],
bezt[a].vec[2][0],
bezt[a + 1].vec[0][0],
bezt[a + 1].vec[1][0],
point,
CM_RESOL - 1,
sizeof(float[2]));
BKE_curve_forward_diff_bezier(bezt[a].vec[1][1],
bezt[a].vec[2][1],
bezt[a + 1].vec[0][1],
bezt[a + 1].vec[1][1],
point + 1,
CM_RESOL - 1,
sizeof(float[2]));
/* Handle pre point for wrapping */
if (use_wrapping) {
curve_eval_bezier_point(bezt_pre.vec, bezt[0].vec, point);
point += 2 * CM_RESOL;
}
/* store first and last handle for extrapolation, unit length */
/* Process middle elements */
for (int a = 0; a < cuma->totpoint - 1; a++, point += 2 * CM_RESOL) {
int b = a + 1;
curve_eval_bezier_point(bezt[a].vec, bezt[b].vec, point);
}
if (use_wrapping) {
/* Handle post point for wrapping */
curve_eval_bezier_point(bezt[cuma->totpoint - 1].vec, bezt_post.vec, point);
}
/* Store first and last handle for extrapolation, unit length. (Only relevant when not using
* wrapping.) */
cuma->ext_in[0] = bezt[0].vec[0][0] - bezt[0].vec[1][0];
cuma->ext_in[1] = bezt[0].vec[0][1] - bezt[0].vec[1][1];
JonasDichelle marked this conversation as resolved Outdated

I am skeptical about this, because the members will now be uninitialized, so I would just leave it even if it doesn't make much sense. We could add an assert in BKE_curvemapping_compute_slopes, but also doesn't seem worth it.

I am skeptical about this, because the members will now be uninitialized, so I would just leave it even if it doesn't make much sense. We could add an assert in `BKE_curvemapping_compute_slopes`, but also doesn't seem worth it.
float ext_in_range = sqrtf(cuma->ext_in[0] * cuma->ext_in[0] +
@ -766,7 +822,7 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
/* cleanup */
MEM_freeN(bezt);
float range = CM_TABLEDIV * (cuma->maxtable - cuma->mintable);
float range = CM_TABLEDIV * table_range;
cuma->range = 1.0f / range;
/* now make a table with CM_TABLE equal x distances */
@ -785,9 +841,8 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
while (cur_x >= point[0] && point != lastpoint) {
point += 2;
}
/* Check if we are on or outside the start or end point. */
if (point == firstpoint || (point == lastpoint && cur_x >= point[0])) {
if ((point == firstpoint || (point == lastpoint && cur_x >= point[0])) && !use_wrapping) {
if (compare_ff(cur_x, point[0], 1e-6f)) {
/* When on the point exactly, use the value directly to avoid precision
* issues with extrapolation of extreme slopes. */

View File

@ -49,6 +49,7 @@
#include "BKE_animsys.h"
#include "BKE_armature.hh"
#include "BKE_attribute.hh"
#include "BKE_colortools.hh"
#include "BKE_curve.hh"
#include "BKE_effect.h"
#include "BKE_grease_pencil.hh"
@ -1998,6 +1999,31 @@ static void image_settings_avi_to_ffmpeg(Scene *scene)
}
}
static bool seq_hue_correct_set_wrapping(Sequence *seq, void * /*user_data*/)
{
LISTBASE_FOREACH (SequenceModifierData *, smd, &seq->modifiers) {
if (smd->type == seqModifierType_HueCorrect) {
HueCorrectModifierData *hcmd = (HueCorrectModifierData *)smd;
CurveMapping *cumap = (CurveMapping *)&hcmd->curve_mapping;
cumap->flag |= CUMA_USE_WRAPPING;
}
}
return true;
}
static void versioning_node_hue_correct_set_wrappng(bNodeTree *ntree)
{
if (ntree->type == NTREE_COMPOSIT) {
LISTBASE_FOREACH_MUTABLE (bNode *, node, &ntree->nodes) {
if (node->type == CMP_NODE_HUECORRECT) {
CurveMapping *cumap = (CurveMapping *)node->storage;
cumap->flag |= CUMA_USE_WRAPPING;
}
}
}
}
void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain)
{
if (!MAIN_VERSION_FILE_ATLEAST(bmain, 400, 1)) {
@ -3070,6 +3096,19 @@ void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain)
}
}
if (!MAIN_VERSION_FILE_ATLEAST(bmain, 402, 12)) {
FOREACH_NODETREE_BEGIN (bmain, ntree, id) {
versioning_node_hue_correct_set_wrappng(ntree);
}
FOREACH_NODETREE_END;
LISTBASE_FOREACH (Scene *, scene, &bmain->scenes) {
if (scene->ed != nullptr) {
SEQ_for_each_callback(&scene->ed->seqbase, seq_hue_correct_set_wrapping, nullptr);
}
}
}
/**
* Always bump subversion in BKE_blender_version.h when adding versioning
* code here, and wrap it inside a MAIN_VERSION_FILE_ATLEAST check.

View File

@ -4275,7 +4275,7 @@ static uiBlock *curvemap_tools_func(
});
}
if (show_extend) {
if (show_extend && !(cumap->flag & CUMA_USE_WRAPPING)) {
{
uiBut *but = uiDefIconTextBut(block,
UI_BTYPE_BUT_MENU,

View File

@ -92,6 +92,7 @@ typedef enum eCurveMappingFlags {
/** The curve is extended by extrapolation. When not set the curve is extended horizontally. */
CUMA_EXTEND_EXTRAPOLATE = (1 << 4),
CUMA_USE_WRAPPING = (1 << 5),
} eCurveMappingFlags;
/** #CurveMapping.preset */
@ -100,7 +101,7 @@ typedef enum eCurveMappingPreset {
CURVE_PRESET_SHARP = 1,
CURVE_PRESET_SMOOTH = 2,
CURVE_PRESET_MAX = 3,
CURVE_PRESET_MID9 = 4,
JonasDichelle marked this conversation as resolved Outdated

Never change the values of DNA enums, if you want to add a new value, add it at the end.
That's because those values can be stored in files.

Anyway, as discussed, you can just change the MID9 to the one we want since it is only used by the hue correction curve.

Never change the values of DNA enums, if you want to add a new value, add it at the end. That's because those values can be stored in files. Anyway, as discussed, you can just change the MID9 to the one we want since it is only used by the hue correction curve.
CURVE_PRESET_MID8 = 4,
CURVE_PRESET_ROUND = 5,
CURVE_PRESET_ROOT = 6,
CURVE_PRESET_GAUSS = 7,

View File

@ -36,13 +36,14 @@ static void node_composit_init_huecorrect(bNodeTree * /*ntree*/, bNode *node)
CurveMapping *cumapping = (CurveMapping *)node->storage;
cumapping->preset = CURVE_PRESET_MID9;
cumapping->preset = CURVE_PRESET_MID8;
for (int c = 0; c < 3; c++) {
CurveMap *cuma = &cumapping->cm[c];
BKE_curvemap_reset(cuma, &cumapping->clipr, cumapping->preset, CURVEMAP_SLOPE_POSITIVE);
}
/* use wrapping for all hue correct nodes */
cumapping->flag |= CUMA_USE_WRAPPING;
/* default to showing Saturation */
cumapping->cur = 1;
}

View File

@ -861,15 +861,15 @@ static void hue_correct_init_data(SequenceModifierData *smd)
int c;
BKE_curvemapping_set_defaults(&hcmd->curve_mapping, 1, 0.0f, 0.0f, 1.0f, 1.0f, HD_AUTO);
hcmd->curve_mapping.preset = CURVE_PRESET_MID9;
hcmd->curve_mapping.preset = CURVE_PRESET_MID8;
for (c = 0; c < 3; c++) {
CurveMap *cuma = &hcmd->curve_mapping.cm[c];
BKE_curvemap_reset(
cuma, &hcmd->curve_mapping.clipr, hcmd->curve_mapping.preset, CURVEMAP_SLOPE_POSITIVE);
}
/* use wrapping for all hue correct modifiers */
hcmd->curve_mapping.flag |= CUMA_USE_WRAPPING;
/* default to showing Saturation */
hcmd->curve_mapping.cur = 1;
}