Regression: Image Texture node: color space and alpha changes are ignored in undo/redo #103242

New Issue

Sun Kim · 2022-12-15T14:15:48+01:00

Sun Kim commented

2022-12-15 14:15:48 +01:00

System Information
Operating system: Windows-10-10.0.19045-SP0 64 Bits
Graphics card: Radeon RX 570 Series ATI Technologies Inc. 4.5.0 Core Profile Context 22.11.2.221130

Blender Version
Broken: version: 3.5.0 Alpha, branch: master, commit date: 2022-12-15 01:17, hash: 0240b89599
Worked: 3.1
Caused by e06399c223

Also confirmed broken in 3.2.2, 3.3.2, and 3.4.0.

Short description of error
Color space and alpha setting changes in Image Texture node are ignored in undo/redo.

Exact steps for others to reproduce the error

Add Image Texture Node in shader.
Change its color space or alpha setting.
Undo and redo.

ImageNodeUndo.blend
ImageNodeUndo.mp4

**System Information** Operating system: Windows-10-10.0.19045-SP0 64 Bits Graphics card: Radeon RX 570 Series ATI Technologies Inc. 4.5.0 Core Profile Context 22.11.2.221130 **Blender Version** Broken: version: 3.5.0 Alpha, branch: master, commit date: 2022-12-15 01:17, hash: `0240b89599` Worked: 3.1 Caused by e06399c223 Also confirmed broken in 3.2.2, 3.3.2, and 3.4.0. **Short description of error** Color space and alpha setting changes in Image Texture node are ignored in undo/redo. **Exact steps for others to reproduce the error** - Add Image Texture Node in shader. - Change its color space or alpha setting. - Undo and redo. [ImageNodeUndo.blend](https://archive.blender.org/developer/F14059492/ImageNodeUndo.blend) [ImageNodeUndo.mp4](https://archive.blender.org/developer/F14059490/ImageNodeUndo.mp4)

Sun Kim commented

2022-12-15 14:15:48 +01:00

Added subscriber: @persun

Pratik Borhade commented

2022-12-16 08:03:56 +01:00

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

Pratik Borhade commented

2022-12-16 08:04:01 +01:00

Added subscriber: @PratikPB2123

Pratik Borhade changed title from ~~Image Texture node: color space and alpha changes are ignored in undo/redo~~ to Regression: Image Texture node: color space and alpha changes are ignored in undo/redo

2022-12-16 08:04:41 +01:00

Pratik Borhade commented

2022-12-16 08:22:53 +01:00

Added subscriber: @mont29

Pratik Borhade commented

2022-12-16 08:22:53 +01:00

Caused by e06399c223
@mont29 ^

Caused by e06399c223 @mont29 ^

Bastien Montagne commented

2022-12-19 02:06:18 +01:00

Added subscribers: @Sergey, @Jeroen-Bakker

Bastien Montagne commented

2022-12-19 02:06:18 +01:00

I do not think e06399c223 caused this, but merely revealed that issue (that commit simply prevents clearing all image caches on undo/redo, even when not needed).

Also, this is not related to nodes, these settings are Image ones, and issue can be replicated with a picture loaded in the Image editor.

I think the issue here is once again that image is using its own update system (BKE_image_signal) that by-passes the depsgraph. Undo can only handle depsgraph-managed updates.

Until this is addressed, one possible work-around could be to check for a specific RECALC flag in code restoring caches on undo, and skip it then, something along that line (this seems to work, but I suspect we'd need a new dedicated depsgraph tag for that?):

P3400: (An Untitled Masterwork)

diff --git a/source/blender/blenloader/intern/readfile.cc b/source/blender/blenloader/intern/readfile.cc
index d1a5bfce52a..73e09cef703 100644
--- a/source/blender/blenloader/intern/readfile.cc
+++ b/source/blender/blenloader/intern/readfile.cc
@@ -1604,7 +1604,7 @@ static void blo_cache_storage_entry_register(
 
 /** Restore a cache data entry from old ID into new one, when reading some undo memfile. */
 static void blo_cache_storage_entry_restore_in_new(
-    ID * /*id*/, const IDCacheKey *key, void **cache_p, uint flags, void *cache_storage_v)
+    ID *id, const IDCacheKey *key, void **cache_p, uint flags, void *cache_storage_v)
 {
   BLOCacheStorage *cache_storage = static_cast<BLOCacheStorage *>(cache_storage_v);
 
@@ -1618,6 +1618,15 @@ static void blo_cache_storage_entry_restore_in_new(
     return;
   }
 
+  /* Assume that when ID source is tagged as changed, its caches need to be cleared.
+   * NOTE: This is mainly a work-around for some IDs, like Image, which use a non-depsgraph-handled
+   * process for part of their updates.
+   */
+  if (id->recalc & ID_RECALC_SOURCE) {
+    *cache_p = nullptr;
+    return;
+  }
+
   BLOCacheStorageValue *storage_value = static_cast<BLOCacheStorageValue *>(
       BLI_ghash_lookup(cache_storage->cache_map, key));
   if (storage_value == nullptr) {
diff --git a/source/blender/makesrna/intern/rna_color.c b/source/blender/makesrna/intern/rna_color.c
index acc1efe8d12..716b9427e53 100644
--- a/source/blender/makesrna/intern/rna_color.c
+++ b/source/blender/makesrna/intern/rna_color.c
@@ -625,7 +625,7 @@ static void rna_ColorManagedColorspaceSettings_reload_update(Main *bmain,
   if (GS(id->name) == ID_IM) {
     Image *ima = (Image *)id;
 
-    DEG_id_tag_update(&ima->id, 0);
+    DEG_id_tag_update(&ima->id, ID_RECALC_ALL);
 
     BKE_image_signal(bmain, ima, NULL, IMA_SIGNAL_COLORMANAGE);

0 or ID_RECALC_ALL is most certainly not the desired update tag(s) here either.

NOTE: This patch snippet addresses the color space issue, but the alpha mode case is exactly the same, see rna_Image_colormanage_update code.

@Jeroen-Bakker @Sergey wouldn't mind having your thoughts here?

I do not think e06399c223 caused this, but merely revealed that issue (that commit simply prevents clearing all image caches on undo/redo, even when not needed). Also, this is not related to nodes, these settings are Image ones, and issue can be replicated with a picture loaded in the Image editor. I think the issue here is once again that image is using its own update system (`BKE_image_signal`) that by-passes the depsgraph. Undo can only handle depsgraph-managed updates. Until this is addressed, one possible work-around could be to check for a specific `RECALC` flag in code restoring caches on undo, and skip it then, something along that line (this seems to work, but I suspect we'd need a new dedicated depsgraph tag for that?): [P3400: (An Untitled Masterwork)](https://archive.blender.org/developer/P3400.txt) ``` diff --git a/source/blender/blenloader/intern/readfile.cc b/source/blender/blenloader/intern/readfile.cc index d1a5bfce52a..73e09cef703 100644 --- a/source/blender/blenloader/intern/readfile.cc +++ b/source/blender/blenloader/intern/readfile.cc @@ -1604,7 +1604,7 @@ static void blo_cache_storage_entry_register( /** Restore a cache data entry from old ID into new one, when reading some undo memfile. */ static void blo_cache_storage_entry_restore_in_new( - ID * /*id*/, const IDCacheKey *key, void **cache_p, uint flags, void *cache_storage_v) + ID *id, const IDCacheKey *key, void **cache_p, uint flags, void *cache_storage_v) { BLOCacheStorage *cache_storage = static_cast<BLOCacheStorage *>(cache_storage_v); @@ -1618,6 +1618,15 @@ static void blo_cache_storage_entry_restore_in_new( return; } + /* Assume that when ID source is tagged as changed, its caches need to be cleared. + * NOTE: This is mainly a work-around for some IDs, like Image, which use a non-depsgraph-handled + * process for part of their updates. + */ + if (id->recalc & ID_RECALC_SOURCE) { + *cache_p = nullptr; + return; + } + BLOCacheStorageValue *storage_value = static_cast<BLOCacheStorageValue *>( BLI_ghash_lookup(cache_storage->cache_map, key)); if (storage_value == nullptr) { diff --git a/source/blender/makesrna/intern/rna_color.c b/source/blender/makesrna/intern/rna_color.c index acc1efe8d12..716b9427e53 100644 --- a/source/blender/makesrna/intern/rna_color.c +++ b/source/blender/makesrna/intern/rna_color.c @@ -625,7 +625,7 @@ static void rna_ColorManagedColorspaceSettings_reload_update(Main *bmain, if (GS(id->name) == ID_IM) { Image *ima = (Image *)id; - DEG_id_tag_update(&ima->id, 0); + DEG_id_tag_update(&ima->id, ID_RECALC_ALL); BKE_image_signal(bmain, ima, NULL, IMA_SIGNAL_COLORMANAGE); ``` `0` or `ID_RECALC_ALL` is most certainly not the desired update tag(s) here either. NOTE: This patch snippet addresses the color space issue, but the alpha mode case is exactly the same, see `rna_Image_colormanage_update` code. @Jeroen-Bakker @Sergey wouldn't mind having your thoughts here?

Sergey Sharybin commented

2023-01-03 14:44:57 +01:00

@mont29, I can not think of an easier fix to what you've proposed in the blo_cache_storage_entry_restore_in_new.

Ideally, the entire image signalling will be replaced with the dependency graph tagging, but I am not sure how to make it work in practice as there is no image ID tag hapenning on undo or redo of the specific property. If the undo system somehow can inform dependency graph the same tag as what is used in the RNA update function that'd make it easy.

As for the ID_RECALC_ALL: the comment above it explicitly tells to not use the flag for tagging. I think it is not unreasonable to use ID_RECALC_SOURCE here.

@mont29, I can not think of an easier fix to what you've proposed in the `blo_cache_storage_entry_restore_in_new`. Ideally, the entire image signalling will be replaced with the dependency graph tagging, but I am not sure how to make it work in practice as there is no image ID tag hapenning on undo or redo of the specific property. If the undo system somehow can inform dependency graph the same tag as what is used in the RNA update function that'd make it easy. As for the `ID_RECALC_ALL`: the comment above it explicitly tells to not use the flag for tagging. I think it is not unreasonable to use `ID_RECALC_SOURCE` here.

Bastien Montagne commented

2023-01-05 17:31:12 +01:00

In #103242#1468544, @Sergey wrote:
Ideally, the entire image signalling will be replaced with the dependency graph tagging, but I am not sure how to make it work in practice as there is no image ID tag hapenning on undo or redo of the specific property. If the undo system somehow can inform dependency graph the same tag as what is used in the RNA update function that'd make it easy.

Actually the undo system is already storing update tags, and restoring/replaying them when reading an undo step. That's how it avoids re-creating and re-evaluating the DEG from scratch on every undo. See ID.recalc_up_to_undo_push, ID.recalc_after_undo_push, and their usages in code.

> In #103242#1468544, @Sergey wrote: > Ideally, the entire image signalling will be replaced with the dependency graph tagging, but I am not sure how to make it work in practice as there is no image ID tag hapenning on undo or redo of the specific property. If the undo system somehow can inform dependency graph the same tag as what is used in the RNA update function that'd make it easy. Actually the undo system is already storing update tags, and restoring/replaying them when reading an undo step. That's how it avoids re-creating and re-evaluating the DEG from scratch on every undo. See `ID.recalc_up_to_undo_push`, `ID.recalc_after_undo_push`, and their usages in code.

Sergey Sharybin commented

2023-01-05 17:55:05 +01:00

Ah, great!

Here is a quick prototype of using the dependency graph to invalidate the buffers when the source changes: P3426

it is not something production-ready, as it potentially causes threading conflict if a background dependency graph does full re-evaluation. Need to somehow protect the invalidation to happen unless an explicit tag happened (the depsgraph evaluates all nodes on its first evaluation, unless persuaded otherwise). But even then maybe need to limit such invalidation to the active dependency graphs only.

In any case, there is a potential, but it is a project on its own to replace the signal system with depsgraph tags. For the meanwhile it might be better/safer to follow follow your change in the blo_cache_storage_entry_restore_in_new.

Ah, great! Here is a quick prototype of using the dependency graph to invalidate the buffers when the source changes: [P3426](https://archive.blender.org/developer/P3426.txt) it is not something production-ready, as it potentially causes threading conflict if a background dependency graph does full re-evaluation. Need to somehow protect the invalidation to happen unless an explicit tag happened (the depsgraph evaluates all nodes on its first evaluation, unless persuaded otherwise). But even then maybe need to limit such invalidation to the active dependency graphs only. In any case, there is a potential, but it is a project on its own to replace the signal system with depsgraph tags. For the meanwhile it might be better/safer to follow follow your change in the `blo_cache_storage_entry_restore_in_new`.

blender-admin commented

2023-01-06 11:01:11 +01:00

This issue was referenced by fc9c39e320

This issue was referenced by fc9c39e320a87f40473b077f96be4472f53e98b5

Bastien Montagne commented

2023-01-06 11:32:03 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Bastien Montagne closed this issue

2023-01-06 11:32:03 +01:00

Bastien Montagne self-assigned this 2023-01-06 11:32:03 +01:00

Bastien Montagne added this to the Core project 2023-02-09 15:43:10 +01:00

Bastien Montagne removed this from the Core project 2023-02-09 18:19:07 +01: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: Image Texture node: color space and alpha changes are ignored in undo/redo #103242