Making the Hue Correct Curves Wrap #117114
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117114
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "JonasDichelle/blender:hue-correct-bezier-wrap"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Making the Hue Correct Curves Wrap
Overview
Currently, the hue correct curves in both the compositor and the video sequence editor don't wrap on either side.
This causes there to be two reds, one on the bluer side and the other on the greener side.
Both are treated as separate parts of the spectrum, even though they should wrap back onto each other.
This results in tearing between the colors.
The Proposal
To fix the issue, I've added the ability to enable wrapping to the curve maps.
When enabled, we add two imaginary bezier curve points at either end of the curve map and calculate their handles to get a seamless wrapping fall off.
I added the function
BKE_curvemap_set_wrapping
, which is called whenever we initialize new hue correct curves.That way we make sure already existing nodes will not be affected.
works in the VSE too
Awesome addition! Makes colour correcting images soooo much better!
I feel like this could be implemented in a simpler way, with less such large branching, though I might be wrong. In particular, I suspect we can implement this by only adding the necessary elements to the
bezt
array, with minor adjustements to the rest of the function. Can you clarify why it is implemented like this?@OmarEmaraDev, thank you for reviewing the code!
You are right, there were some areas of the code that added too many unnecessary redundancies.
I went through the code and I believe that I simplified it as much as possible.
Additionally, a different curve preset needs to be used for curves that wrap. CURVE_PRESET_MID9 has points at the very start and very end, which won't play well with wrapping. Instead, another present that exempt the end point needs to be added and used.
@ -638,2 +643,4 @@
}
/* Evaluate a point on a curve, given two bezier triples */
static void BKE_curve_eval_bezier_point(float bezt1[3][3], float bezt2[3][3], float *point)
Drop the BKE prefix, since this is a static function and not part of a public API.
@ -654,3 +675,3 @@
/* Rely on Blender interpolation for bezier curves, support extra functionality here as well. */
bezt = static_cast<BezTriple *>(MEM_callocN(cuma->totpoint * sizeof(BezTriple), "beztarr"));
int bezt_count = cuma->totpoint;
Why allocate the extra bezier points as part of the array? Just create two stack variables for them, see below.
I thought since we're already allocating an array of the same type to the heap we might aswell add the two new points. But you're right it's cleaner to just have them as seperate variables.
@ -674,1 +696,4 @@
const BezTriple *bezt_next = nullptr;
const BezTriple *bezt_prev = nullptr;
/* Create two extra points for wrapping curves. */
You can do something like, then take their address later.
@ -675,0 +705,4 @@
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] - 1.0f;
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.
@ -727,3 +788,1 @@
int totpoint = (cuma->totpoint - 1) * CM_RESOL;
float *allpoints = static_cast<float *>(MEM_callocN(totpoint * 2 * sizeof(float), "table"));
float *point = allpoints;
int totpoint;
Why separate declarations from initializations? Do the initialization directly.
@ -765,0 +809,4 @@
/* Handle post point for wrapping */
BKE_curve_eval_bezier_point(bezt[cuma->totpoint - 1].vec, bezt_post->vec, point);
}
else {
I would put that else in its own condition
(!use_wrapping)
for clarity.I added CURVE_PRESET_MID8. Now MID9 isn't being anymore, since it was only being used by hue correct.
Do you think we can just replace MID9 with MID8?
@JonasDichelle Yah, just do that.
@ -655,2 +687,3 @@
/* Rely on Blender interpolation for bezier curves, support extra functionality here as well. */
bezt = static_cast<BezTriple *>(MEM_callocN(cuma->totpoint * sizeof(BezTriple), "beztarr"));
int bezt_count = cuma->totpoint;
bezt = static_cast<BezTriple *>(MEM_callocN((bezt_count) * sizeof(BezTriple), "beztarr"));
bezt_count
is no longer used anywhere, so just use it directly as it was.@ -788,3 +859,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])) && not use_wrapping) {
Today I learned that C++ has a
not
keyword. :)Not sure if the style guide has anything against it, but I would just use
!
to avoid confusion.hahaha sure
@ -100,12 +101,13 @@ typedef enum eCurveMappingPreset {
CURVE_PRESET_SHARP = 1,
CURVE_PRESET_SMOOTH = 2,
CURVE_PRESET_MAX = 3,
CURVE_PRESET_MID9 = 4,
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.
@ -637,12 +642,28 @@ static float curvemap_calc_extend(const CurveMapping *cumap,
return 0.0f;
}
/* Evaluate a point on a curve, given two bezier triples */
The comment doesn't seem accurate, since we are evaluating many points. I would rename
bezt1
andbezt2
tostart
andend
, and change their type toBezTriple
. I would also rename the final argument topoints
to reflect that they are actually many points. Then update the comment as follows:@ -643,3 +663,4 @@
const rctf *clipr = &cumap->clipr;
CurveMapPoint *cmp = cuma->curve;
BezTriple *bezt;
const bool use_wrapping = cuma->use_wrapping;
I would add a comment here that roughly describes the mechanism by which wrapping is implemented.
@ -765,0 +805,4 @@
/* Handle post point for wrapping */
curve_eval_bezier_point(bezt[cuma->totpoint - 1].vec, bezt_post.vec, point);
}
if (!use_wrapping) {
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.@ -59,2 +59,3 @@
short default_handle_type;
char _pad[6];
char _pad[2];
int use_wrapping;
Keep the padding last and use
char
foruse_wrapping
.@JonasDichelle Looks generally good now. There are some style issues still with comments, but I would like to get review from someone else first.
@Sergey Do you want to take a look at this?
Being able to wrap the curve used by Hue Correction sounds good, but the interface needs to become more clear: either the hue correction always needs to wrap (including the existing files) or it needs to become an option. With the current state of the patch it is confusing for artists to have such discrepancy: they'll see two Hue nodes which behave differently, without having any hint why is it so, and without a way to make existing nodes behave the same as if they were added new.
Another source of confusion is that there are still options for extrapolation in the menu. Wrapping and extrapolating sounds mutually exclusive behaviors.
@Sergey
In my opinion hue correction should absolutely always wrap. Them not wrapping results in behavior that is always unwanted. So I don't think it should be an option.
Currently existing nodes, from old blend files are not affected. Only newly added nodes get the wrapping.
However I don't know if that is the best approach.
And you're right, in this case the "extend" options should be removed from the menu.
I'll try to make existing nodes wrap too. And I'll remove the extend options.
I removed the “Extend” options from the hue correct curves.
And it’s now setting wrapping in uiTemplateCurveMapping, so that it affects existing curves too.
Setting wrapping for old files should be done using versioning. See code in
versioning_400.cc
for examples. If you need help, let us know.I am not sure we should be setting this flag at all. I didn't have enough time to verify what's possible in the current design of curve-mapping, but you can imagine that it could be just a runtime flag, indicating how the widget is drawn and used by nodes. Having the flag stored on the curve-mapping would useful if we would want to make it a user-selectable behavior (which I don't think we want?)
@Sergey This is kind of how I image the property already. I assume you don't want to store it in DNA? Then where should it be stored?
@OmarEmaraDev I don't think it needs to be stored at all, but live as an argument to UI template (passed from the UI script), and be passed to the evaluation by the node/modifier.
Or is there something non-obvious that make us required to store it in DNA? If so, would be nice to know it, and maybe give it some thought :)
@Sergey Table construction happens mainly from
BKE_curvemapping_changed
andBKE_curvemapping_init
, the former is called many times in many places like interface handlers, templates, and operators, where it is not immediately clear to me how wrapping could be passed down or identified. It could be thatBKE_curvemapping_changed
shouldn't really construct the table I guess, but I am not sure if code in Blender relies on that behavior somehow, so it seems possible, just might open a can of worms.Further, having to pass it to both the draw code and the place of evaluation means that we could potentially have a curve that is drawn using wrapping but evaluated without wrapping, so less robust that having a flag.
@OmarEmaraDev Ok, things are more entagled than you hope they would. Then the wrapping should be stored in the curve mapping once during construction, and the evaluation and UI template would use it. So we wouldn't have a new parameter in the UI template.
@Sergey When should it happen during construction? If it happens when the table is constructed it won't be applied to existing curves.
The best place I can see to put it that would work without adding a parameter to ui Template is in
curvemap_buttons_layout
where it handles the hsv curves.That way it can automatically be added to HSV curves.
@JonasDichelle That's why we mentioned versioning, as it should allow you to adjust existing curves. See my comment above.
@OmarEmaraDev thanks, now I understand. Sorry, I missunderstood earlier.
So the idea is to still store it in DNA, but to only set it during construction and versioning?
@JonasDichelle Yes.
@ -644,6 +660,14 @@ static void curvemap_make_table(const CurveMapping *cumap, CurveMap *cuma)
CurveMapPoint *cmp = cuma->curve;
BezTriple *bezt;
/*
Comment lines should start with a start per the style guide.
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#cc-comments
@ -751,0 +808,4 @@
/* 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
Comment style as well.
@blender-bot build
Looks like main is not merged completely and your versioning code is out of sync with the version define.
I wasn't exactly sure how to adjust the versioning.
The last version being checked in versioning_400 was
402, 5
but the version in BKE_blender_version was already402, 6
. So I just made my changes be402, 7
now. I hope that is correct.Versioning needs to be updated, but we can do that before merging. Otherwise, looks good.
@ -171,0 +172,4 @@
/**
* Set the wrapping mode for the curve map.
*/
void BKE_curvemap_set_wrapping(CurveMapping *cumap, bool use_wrapping);
Do we need this function? Not sure why is this better than direct assignment. Even the current PR kinda mixes the setter approach with the direct assignment.
@ -1413,3 +1473,3 @@
hist->x_resolution = 256;
hist->xmax = 1.0f;
// hist->ymax = 1.0f; /* now do this on the operator _only_ */
/* hist->ymax = 1.0f; /* now do this on the operator _only_ */
This probably generates warning about
/*
used within/* ... */
. Having a note like/* NOTE: The maximum Y value is expected to be set by the users of the mapping, i.e. via operator. */
will be better than such historical/time references in the code. Or just leave the code untouched as it is not directly related implementing the feature.@ -81,3 +81,3 @@
short tone;
char _pad[6];
char use_wrapping;
Any specific reason for this being a new flag and not a flag like
CUMA_DO_CLIP
?@Sergey I implemented your suggestions!
@JonasDichelle Thanks! On the code side I do not have any further remarks at this time.
For the next steps I'd want to double-check on the possible unwanted behavior change with all the latest code updates just to be on a safe side. And either me or Omar will help with the versioning.
@Sergey To me, user-level behavior seems as one might expect.
@blender-bot package
Package build started. Download here when ready.
@OmarEmaraDev Indeed it does. I've tested it now as well.
So the remaining part is to solve merge conflict, land the patch, take care of the tests (i think some reference images might need to be updated).
@JonasDichelle We should be ready to merge this, just need to fix the mergeconflict and update versioning, can you do that?
It seems like you accidentally committed submodule hashes, can you revert those?
Yes sorry about that. But the merge and versioning should all be up to date now!