Regression: Blender doesn't allow to animate any Colormanagement setting (e.g. Exposure) #97003

New Issue

Denis · 2022-04-03T19:18:56+02:00

Denis commented

2022-04-03 19:18:56 +02:00

System Information
Operating system: Manjaro
Graphics card: RTX 3060

Blender Version
Broken: 3.1.2, 3.2.0
Worked: 3.0.1, 2.93.7

Caused by 35aedd87e7

I can't animate the Exposure parameter. If it has at least one keyframe, Blender will reset it every time when I try to change the value.
The issue was not noticed in 2.93.7.

2022-04-03 19-59-42.mp4

Reason: when Colormanagement settings change, the scene is tagged DEG_id_tag_update(id, 0); which then triggers deg_graph->tag_time_source(); since 35aedd87e7

**System Information** Operating system: Manjaro Graphics card: RTX 3060 **Blender Version** Broken: 3.1.2, 3.2.0 Worked: 3.0.1, 2.93.7 Caused by 35aedd87e7 I can't animate the Exposure parameter. If it has at least one keyframe, Blender will reset it every time when I try to change the value. The issue was not noticed in 2.93.7. [2022-04-03 19-59-42.mp4](https://archive.blender.org/developer/F12966899/2022-04-03_19-59-42.mp4) Reason: when Colormanagement settings change, the scene is tagged `DEG_id_tag_update(id, 0);` which then triggers `deg_graph->tag_time_source();` since 35aedd87e7

Denis commented

2022-04-03 19:18:56 +02:00

Added subscriber: @Mapper720-4

Philipp Oeser commented

2022-04-04 12:38:11 +02:00

Added subscriber: @lichtwerk

Philipp Oeser commented

2022-04-04 12:38:11 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Philipp Oeser commented

2022-04-04 12:38:11 +02:00

Can confirm, will check

Philipp Oeser commented

2022-04-04 14:09:32 +02:00

Added subscriber: @ideasman42

Philipp Oeser commented

2022-04-04 14:09:32 +02:00

Caused by 35aedd87e7

CC @ideasman42

Caused by 35aedd87e7 CC @ideasman42

Philipp Oeser changed title from ~~Blender doesn't allow to animate the Exposure parameter~~ to Regression: Blender doesn't allow to animate the Exposure parameter

2022-04-04 14:10:08 +02:00

Philipp Oeser changed title from ~~Regression: Blender doesn't allow to animate the Exposure parameter~~ to Regression: Blender doesn't allow to animate any Colormanagement setting (e.g. Exposure)

2022-04-04 14:44:32 +02:00

Philipp Oeser commented

2022-04-04 15:26:04 +02:00

Added subscriber: @Sergey

Philipp Oeser commented

2022-04-04 15:26:04 +02:00

As said in the description: main problem is tagging a scene so generally with DEG_id_tag_update(id, 0)
@ideasman42 suggested DEG_id_tag_update(id, ALL & ~ID_RECALC_FRAME_CHANGE), or figuring out what colormanagement settings really need to be updating (maybe add a new flag).
There is also many more occasions of this tagging the scene.

@Sergey: opinions?

As said in the description: main problem is tagging a scene so generally with `DEG_id_tag_update(id, 0)` @ideasman42 suggested `DEG_id_tag_update(id, ALL & ~ID_RECALC_FRAME_CHANGE)`, or figuring out what colormanagement settings really need to be updating (maybe add a new flag). There is also many more occasions of this tagging the scene. @Sergey: opinions?

Sergey Sharybin commented

2022-04-04 15:32:52 +02:00

In the very middle of something else now..
Does the P2882: (An Untitled Masterwork)

diff --git a/source/blender/depsgraph/intern/depsgraph_tag.cc b/source/blender/depsgraph/intern/depsgraph_tag.cc
index f945e9b6fbc..3b87f63ef23 100644
--- a/source/blender/depsgraph/intern/depsgraph_tag.cc
+++ b/source/blender/depsgraph/intern/depsgraph_tag.cc
@@ -445,8 +445,8 @@ const char *update_source_as_string(eUpdateSource source)

 int deg_recalc_flags_for_legacy_zero()
 {
-  return ID_RECALC_ALL &
-         ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION | ID_RECALC_SOURCE | ID_RECALC_EDITORS);
+  return ID_RECALC_ALL & ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION | ID_RECALC_SOURCE |
+                           ID_RECALC_EDITORS | ID_RECALC_FRAME_CHANGE);
 }

 int deg_recalc_flags_effective(Depsgraph *graph, int flags)

fix the problem?

In the very middle of something else now.. Does the [P2882: (An Untitled Masterwork)](https://archive.blender.org/developer/P2882.txt) ``` diff --git a/source/blender/depsgraph/intern/depsgraph_tag.cc b/source/blender/depsgraph/intern/depsgraph_tag.cc index f945e9b6fbc..3b87f63ef23 100644 --- a/source/blender/depsgraph/intern/depsgraph_tag.cc +++ b/source/blender/depsgraph/intern/depsgraph_tag.cc @@ -445,8 +445,8 @@ const char *update_source_as_string(eUpdateSource source) int deg_recalc_flags_for_legacy_zero() { - return ID_RECALC_ALL & - ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION | ID_RECALC_SOURCE | ID_RECALC_EDITORS); + return ID_RECALC_ALL & ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION | ID_RECALC_SOURCE | + ID_RECALC_EDITORS | ID_RECALC_FRAME_CHANGE); } int deg_recalc_flags_effective(Depsgraph *graph, int flags) ``` fix the problem?

Philipp Oeser commented

2022-04-04 15:40:09 +02:00

In #97003#1334937, @Sergey wrote:
In the very middle of something else now..
Does the P2882 fix the problem?

Yes, and that would also take care of #97028 (Audio scrubbing plays current frame when changing any color management setting.) it seems

> In #97003#1334937, @Sergey wrote: > In the very middle of something else now.. > Does the [P2882](https://archive.blender.org/developer/P2882.txt) fix the problem? Yes, and that would also take care of #97028 (Audio scrubbing plays current frame when changing any color management setting.) it seems

Sergey Sharybin commented

2022-04-04 15:52:32 +02:00

@lichtwerk You fine having an honors of typing commit message and pushing it? :)

Form the legacy-zero perspective FRAME_CHANGE is not different from ANIMATION: is not supposed to be tagging for anything which involves animation re-evaluation.

@lichtwerk You fine having an honors of typing commit message and pushing it? :) Form the legacy-zero perspective FRAME_CHANGE is not different from ANIMATION: is not supposed to be tagging for anything which involves animation re-evaluation.

Philipp Oeser commented

2022-04-04 16:21:42 +02:00

Could do, but prefer to carefully go over the occurrances of scene being tagged legacyzero that possibly rely on this (even if falsely so).
Meaning: check if the formerly ID_RECALC_AUDIO_SEEK (now ID_RECALC_FRAME_CHANGE) was not expected to be "included" in legacyzero

Could do, but prefer to carefully go over the occurrances of scene being tagged legacyzero that possibly rely on this (even if falsely so). Meaning: check if the formerly `ID_RECALC_AUDIO_SEEK` (now `ID_RECALC_FRAME_CHANGE`) was not expected to be "included" in legacyzero

Campbell Barton commented

2022-04-05 06:41:02 +02:00

Regarding changes to deg_recalc_flags_for_legacy_zero (I was testing this before noticing the updates to this task).

As far as I can see none of the callers to DEG_id_tag_update(&scene->id, 0) should be relying on audio-seeking (ID_RECALC_FRAME_CHANGE) being set. Mostly this relates to changes to view layers as well as scene quality settings & freestyle modifiers.

Further, we could consider having the define for legacy flags along side other defines P2884 while there is a benefit in keeping them private/localized, it means it's easy to accidentally overlook the fact new flags are implicitly included for all callers that pass in zero to DEG_id_tag_update. OTOH, these flags kinds of flags are not added regularly, so this could be noted in the comments, e.g. P2885.

Regarding changes to `deg_recalc_flags_for_legacy_zero` (I was testing this before noticing the updates to this task). As far as I can see none of the callers to `DEG_id_tag_update(&scene->id, 0)` should be relying on audio-seeking (ID_RECALC_FRAME_CHANGE) being set. Mostly this relates to changes to view layers as well as scene quality settings & freestyle modifiers. --- Further, we could consider having the define for legacy flags along side other defines [P2884](https://archive.blender.org/developer/P2884.txt) while there is a benefit in keeping them private/localized, it means it's easy to accidentally overlook the fact new flags are implicitly included for all callers that pass in zero to `DEG_id_tag_update`. OTOH, these flags kinds of flags are not added regularly, so this could be noted in the comments, e.g. [P2885](https://archive.blender.org/developer/P2885.txt).

Philipp Oeser commented

2022-04-05 09:16:47 +02:00

Thx checking, @ideasman42 .
Seems this is in good hands, please go ahead with committing etc.

Thx checking, @ideasman42 . Seems this is in good hands, please go ahead with committing etc.

Sergey Sharybin commented

2022-04-05 10:38:27 +02:00

@ideasman42, Going away from using 0 as a tag is a nice goal, but the proposed in P2884 way is wrong. The deg_recalc_flags_for_legacy_zero is an internal function which is used by a branch of DEG_id_tag_update which handles 0 flag given to it.
It is also bad idea to tag for everything. The DEG_id_tag_update should be given flag corresponding to what was actually changed.

@ideasman42, Going away from using 0 as a tag is a nice goal, but the proposed in [P2884](https://archive.blender.org/developer/P2884.txt) way is wrong. The `deg_recalc_flags_for_legacy_zero ` is an internal function which is used by a branch of `DEG_id_tag_update ` which handles 0 flag given to it. It is also bad idea to tag for everything. The `DEG_id_tag_update ` should be given flag corresponding to what was actually changed.

Campbell Barton commented

2022-04-05 11:30:25 +02:00

@Sergey it makes sense that it's hidden & not used publicly, the outcome of this is that it's easy to accidentally introduce bugs by not including a flag in deg_recalc_flags_for_legacy_zero. That's why I'm suggesting some way to make this less likely for this to happen in the future - even if it's to note in a comment that the function should be checked when adding new flags.

@Sergey it makes sense that it's hidden & not used publicly, the outcome of this is that it's easy to accidentally introduce bugs by not including a flag in `deg_recalc_flags_for_legacy_zero`. That's why I'm suggesting some way to make this less likely for this to happen in the future - even if it's to note in a comment that the function should be checked when adding new flags.

Sergey Sharybin commented

2022-04-05 12:31:12 +02:00

That's why I'm suggesting some way to make this less likely for this to happen in the future - even if it's to note in a comment that the function should be checked when adding new flags.

The proper way to do so is to stop using flag 0.

> That's why I'm suggesting some way to make this less likely for this to happen in the future - even if it's to note in a comment that the function should be checked when adding new flags. The proper way to do so is to stop using flag 0.

Campbell Barton commented

2022-04-05 12:41:13 +02:00

@Sergey that's fine as a goal, currently there are over 200 instances of this - unless there are near-term plans to remove them (by the next release for e.g) we should document the current state of the code.

Part of the issue is there isn't much incentive to spend time removing these but further discussion about this could happen over chat.

@Sergey that's fine as a goal, currently there are over 200 instances of this - unless there are near-term plans to remove them (by the next release for e.g) we should document the current state of the code. Part of the issue is there isn't much incentive to spend time removing these but further discussion about this could happen over chat.

blender-admin commented

2022-04-05 12:58:50 +02:00

This issue was referenced by 328dfab423

This issue was referenced by 328dfab4236ac56a36969335df795e80cba08188

Campbell Barton commented

2022-04-05 13:07:39 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Campbell Barton closed this issue

2022-04-05 13:07:39 +02:00

Campbell Barton self-assigned this 2022-04-05 13:07:39 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Regression: Blender doesn't allow to animate any Colormanagement setting (e.g. Exposure) #97003