Page MenuHome

Regression: Blender doesn't allow to animate any Colormanagement setting (e.g. Exposure)
Closed, ResolvedPublic

Description

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 rB35aedd87e78d: Fix T66913: undo after frame-change doesn't refresh properly

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.

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

Event Timeline

Philipp Oeser (lichtwerk) changed the task status from Needs Triage to Confirmed.Apr 4 2022, 12:38 PM

Can confirm, will check

Philipp Oeser (lichtwerk) renamed this task from Blender doesn't allow to animate the Exposure parameter to Regression: Blender doesn't allow to animate the Exposure parameter.Apr 4 2022, 2:10 PM
Philipp Oeser (lichtwerk) updated the task description. (Show Details)
Philipp Oeser (lichtwerk) updated the task description. (Show Details)
Philipp Oeser (lichtwerk) updated the task description. (Show Details)
Philipp Oeser (lichtwerk) renamed this task 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).Apr 4 2022, 2:44 PM

As said in the description: main problem is tagging a scene so generally with DEG_id_tag_update(id, 0)
@Campbell Barton (campbellbarton) 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 Sharybin (sergey): opinions?

In the very middle of something else now..
Does the

1diff --git a/source/blender/depsgraph/intern/depsgraph_tag.cc b/source/blender/depsgraph/intern/depsgraph_tag.cc
2index f945e9b6fbc..3b87f63ef23 100644
3--- a/source/blender/depsgraph/intern/depsgraph_tag.cc
4+++ b/source/blender/depsgraph/intern/depsgraph_tag.cc
5@@ -445,8 +445,8 @@ const char *update_source_as_string(eUpdateSource source)
6
7 int deg_recalc_flags_for_legacy_zero()
8 {
9- return ID_RECALC_ALL &
10- ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION | ID_RECALC_SOURCE | ID_RECALC_EDITORS);
11+ return ID_RECALC_ALL & ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION | ID_RECALC_SOURCE |
12+ ID_RECALC_EDITORS | ID_RECALC_FRAME_CHANGE);
13 }
14
15 int deg_recalc_flags_effective(Depsgraph *graph, int flags)
fix the problem?

In the very middle of something else now..
Does the P2882 fix the problem?

Yes, and that would also take care of T97028: Audio scrubbing plays current frame when changing any color management setting. it seems

@Philipp Oeser (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.

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

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.

Thx checking, @Campbell Barton (campbellbarton) .
Seems this is in good hands, please go ahead with committing etc.

@Campbell Barton (campbellbarton), 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.

@Sergey Sharybin (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.

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.

@Sergey Sharybin (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.