ColorBalanceNode doesn't use Autokey correctly #42567

New Issue

Sebastian Koenig · 2014-11-11T11:51:47+01:00

Sebastian Koenig commented

2014-11-11 11:51:47 +01:00

System Information
OSX 10.9.4, GTX660ti

Blender Version
Broken: 5c6e333, 2.72
Worked: i doubt this ever worked

Short description of error
When changing the brightness and color values in the ColorBalanceNode the autokeying only sets a key for the first value (red). That makes animating color changes with autokeying useless.

Exact steps for others to reproduce the error
Open attached file, go to any frame, change the brightness or color of Gamma Values in the CB-Node, see how it draws a yellow border around the color field (=key), change the frame again and notice how the color switches to something else, because only the red channel got keyed.color_balance_autokey.blend

**System Information** OSX 10.9.4, GTX660ti **Blender Version** Broken: 5c6e333, 2.72 Worked: i doubt this ever worked **Short description of error** When changing the brightness and color values in the ColorBalanceNode the autokeying only sets a key for the first value (red). That makes animating color changes with autokeying useless. **Exact steps for others to reproduce the error** Open attached file, go to any frame, change the brightness or color of Gamma Values in the CB-Node, see how it draws a yellow border around the color field (=key), change the frame again and notice how the color switches to something else, because only the red channel got keyed.[color_balance_autokey.blend](https://archive.blender.org/developer/F123244/color_balance_autokey.blend)

Sebastian Koenig commented

2014-11-11 11:51:47 +01:00

Changed status to: 'Open'

Jeroen Bakker was assigned by Sebastian Koenig

2014-11-11 11:51:47 +01:00

Sebastian Koenig commented

2014-11-11 11:51:47 +01:00

Added subscriber: @sebastian_k

Julian Eisel commented

2014-11-11 12:04:56 +01:00

Added subscriber: @JulianEisel

Ton Roosendaal commented

2014-11-14 12:49:30 +01:00

Added subscriber: @Ton

Ton Roosendaal commented

2014-11-14 12:49:30 +01:00

Julian: I think you can do better than this. Triaging means that you verify and analyze the issue, share your analysis with the rest of us, and that you verified (in code, via commit logs) who is responsible for it.

In this case you could have written out that:

you confirmed that the issue is not working
that it did not work ever, so it's not a bug really but more of a limitation of code
but then you check in which similar (RGB or XYZ) cases it works
maybe it is only an issue for node properties/buttons?
if so, then why is that only a node problem and not for other buttons/panels

Then you have pinned down the precise issue, and you can find who was responsible for the feature, and assign to him, with the message:

Please check if you can fix this, or mark it as a known limitation in our UI section of wiki todo.

That's what you got involved for - help others do their work :)

Julian: I think you can do better than this. Triaging means that you verify and analyze the issue, share your analysis with the rest of us, and that you verified (in code, via commit logs) who is responsible for it. In this case you could have written out that: - you confirmed that the issue is not working - that it did not work ever, so it's not a bug really but more of a limitation of code - but then you check in which similar (RGB or XYZ) cases it works - maybe it is only an issue for node properties/buttons? - if so, then why is that only a node problem and not for other buttons/panels Then you have pinned down the precise issue, and you can find who was responsible for the feature, and assign to him, with the message: - Please check if you can fix this, or mark it as a known limitation in our UI section of wiki todo. That's what you got involved for - help others do their work :)

Jeroen Bakker was unassigned by Julian Eisel

2014-11-14 17:35:13 +01:00

Julian Eisel self-assigned this 2014-11-14 17:35:13 +01:00

Julian Eisel commented

2014-11-14 17:35:13 +01:00

Added subscriber: @Jeroen-Bakker

Julian Eisel commented

2014-11-14 17:35:13 +01:00

Hey @Ton,
Indeed, this isn't really helpful. I should've spend much more time on this. I just had a quick look on it while working on something different (same with #42475).
Next time I'll try to investigate more time into the points you've thrown in.
Instead of writing moar excuses I just looked into this bug and found a possible fix (D898) - needs approval before commiting). I hope that's sort of a compensation ;)

Hey @Ton, Indeed, this isn't really helpful. I should've spend much more time on this. I just had a quick look on it while working on something different (same with #42475). Next time I'll try to investigate more time into the points you've thrown in. Instead of writing moar excuses I just looked into this bug and found a possible fix ([D898](https://archive.blender.org/developer/D898)) - needs approval before commiting). I hope that's sort of a compensation ;)

Bastien Montagne commented

2014-11-14 21:47:14 +01:00

Added subscribers: @JoshuaLeung, @ideasman42, @mont29

Bastien Montagne commented

2014-11-14 21:47:14 +01:00

@JulianEisel No, this will not work. 0 is a valid index in RNA arrays, you should absolutely not use it as 'everything' marker (else, modifying e.g. only X coord would key whole XYZ, same for R/RGBA, etc.).

Also, there is nothing specific to nodes here, same issue exists for any 'whole RNA array' button (all color wheels mostly). Issue is, we only always key one FCurve in this case (the one returned by ui_but_get_fcurve()), since we give this FCurve's array_index to insert_keyframe() call in ui_but_anim_autokey().

This patch rather uses button's rna_index value here, which will be set to expected -1 value for color wheels & co. Would like to get blessing from Joshua and/or Campbell here, though...

P165: #42567

diff --git a/source/blender/editors/interface/interface_anim.c b/source/blender/editors/interface/interface_anim.c
index 43899f9..735c73d 100644
--- a/source/blender/editors/interface/interface_anim.c
+++ b/source/blender/editors/interface/interface_anim.c
@@ -219,7 +219,11 @@ void ui_but_anim_autokey(bContext *C, uiBut *but, Scene *scene, float cfra)
 			short flag = ANIM_get_keyframing_flags(scene, 1);
 
 			fcu->flag &= ~FCURVE_SELECTED;
-			insert_keyframe(reports, id, action, ((fcu->grp) ? (fcu->grp->name) : (NULL)), fcu->rna_path, fcu->array_index, cfra, flag);
+			/* Note: We use but->rnaindex instead of fcu->array_index,
+			 *       because a button may control all items of an array at once.
+			 *       E.g., color wheels (see #42567). */
+			insert_keyframe(reports, id, action, ((fcu->grp) ? (fcu->grp->name) : (NULL)),
+			                fcu->rna_path, but->rnaindex, cfra, flag);
 			WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_EDITED, NULL);
 		}
 	}

@JulianEisel No, this will not work. 0 is a valid index in RNA arrays, you should absolutely not use it as 'everything' marker (else, modifying e.g. only X coord would key whole XYZ, same for R/RGBA, etc.). Also, there is nothing specific to nodes here, same issue exists for any 'whole RNA array' button (all color wheels mostly). Issue is, we only always key **one** FCurve in this case (the one returned by `ui_but_get_fcurve()`), since we give this FCurve's `array_index` to `insert_keyframe()` call in `ui_but_anim_autokey()`. This patch rather uses button's `rna_index` value here, which will be set to expected `-1` value for color wheels & co. Would like to get blessing from Joshua and/or Campbell here, though... [P165: #42567](https://archive.blender.org/developer/P165.txt) ``` diff --git a/source/blender/editors/interface/interface_anim.c b/source/blender/editors/interface/interface_anim.c index 43899f9..735c73d 100644 --- a/source/blender/editors/interface/interface_anim.c +++ b/source/blender/editors/interface/interface_anim.c @@ -219,7 +219,11 @@ void ui_but_anim_autokey(bContext *C, uiBut *but, Scene *scene, float cfra) short flag = ANIM_get_keyframing_flags(scene, 1); fcu->flag &= ~FCURVE_SELECTED; - insert_keyframe(reports, id, action, ((fcu->grp) ? (fcu->grp->name) : (NULL)), fcu->rna_path, fcu->array_index, cfra, flag); + /* Note: We use but->rnaindex instead of fcu->array_index, + * because a button may control all items of an array at once. + * E.g., color wheels (see #42567). */ + insert_keyframe(reports, id, action, ((fcu->grp) ? (fcu->grp->name) : (NULL)), + fcu->rna_path, but->rnaindex, cfra, flag); WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_EDITED, NULL); } } ```

Campbell Barton commented

2014-11-15 18:05:05 +01:00

@mont29. looks good
would assert that rna-index == fcu-index (when its not -1), just to catch any errors.

@mont29. looks good would assert that rna-index == fcu-index (when its not -1), just to catch any errors.

blender-admin commented

2014-11-15 18:07:43 +01:00

This issue was referenced by 141064d7ae

This issue was referenced by 141064d7ae3f2e9277ba272d582c0a72c1500e1c

Bastien Montagne commented

2014-11-15 18:20:02 +01:00

Changed status from 'Open' to: 'Resolved'

Bastien Montagne closed this issue