Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.81k stars 318 forks source link

HaldCLUT shown but not used on switching image #4388

Closed Beep6581 closed 6 years ago

Beep6581 commented 6 years ago

It seems that when switching to the "next" image whose HaldCLUT does not exist, RT will show the previous image HaldCLUT's name in the "Film Simulation" combo, even though it is not being used.

To reproduce:

  1. Copy amsterdam.pef to 1.pef and 2.pef
  2. Apply neutral to both, enable Film Simulation for both. It doesn't matter whether you specify a HaldCLUT or not.
  3. In a text editor set the ClutFilename key to a path which does not exist for both, e.g.
    ClutFilename=/does/not/exist/foo

    Some background info why my HaldCLUT no longer exists: I used old raws with old absolute PP3 paths. Issue #3639 allowed RT to use relative paths (good), but now when I open one of these old raws, the CLUT combobox is empty, and RT gives no indication that a HaldCLUT should be used but isn't found - issue #4261.

  4. Open 1.pef. Manually choose a HaldCLUT. In my case, the old PP3s were using absolute paths which were no longer valid because I keep my HaldCLUTs in a different folder. So I went to this new folder and selected the correct HaldCLUT.
  5. Now F4 to the next image. The Film Simulation tool is enabled. The combobox shows that you're using the HaldCLUT from the previous image, but it's not being applied in the pipeline. Turn the tool off and on, and now it gets applied.
Hombre57 commented 6 years ago

@Beep6581 Should this be solved for 5.4 ? Doesn't seem complex to solve.

Beep6581 commented 6 years ago

@Hombre57 that would be nice.

Beep6581 commented 6 years ago

@Hombre57 should I add this to the 5.4 milestone, or leave it for 5.5?

Hombre57 commented 6 years ago

Add it to 5.4, seems serious to me and I'll work on it this w.e.

Hombre57 commented 6 years ago

@Beep6581 Absolute paths are supported now, but given that you can't set an absolute path through the GUI, should we try to convert the absolute path to relative ones ? The logic would be to walk through all the entries in the combo box and see if one match the end of the absolute path. It should work fine, unless the user expect to process the image with a CLUT file outside of it's base HALDClut directory.

Beep6581 commented 6 years ago

@Hombre57 one the one hand, if the user created their own HaldCLUTs and saved them, for example, as C:\stuff\color\1.png and C:\stuff\bw\1.png, then this auto-matching by filename would likely fail to find the correct one. On the other hand, I suppose most users use our RT Film Simulation pack, so auto-matching should work because as far as I remember the filenames are unique. Would be good if RT warned the user that the HaldCLUT was auto-matched and could be wrong, but that depends on #4261. However, the suggested auto-matching is really a different issue - it's a new feature and we're already in feature-freeze, so it should not go into 5.4.

This issue is about the combobox telling lies, it is more urgent and the solution simple - to set the selected item in the combobox to nothing if the HaldCLUT cannot be found.

Hombre57 commented 6 years ago

@Beep6581 Warning the user should not be enough, he can leave the problem unsoved, so how should RT react ? When saving the pp3, which path should it save ?

However it's fine for me if we only have to warn the user.

How should we warn ? A Gtk::InfoBar (and where) ? An alert dialog when opening the image + a warning sign next to the combo ?

Beep6581 commented 6 years ago

As discussed in IRC, we will fix this bug issue for 5.4, and leave user notification for 5.5.

Hombre57 commented 6 years ago

@Beep6581 Could you test this patch :

diff --git a/rtgui/filmsimulation.cc b/rtgui/filmsimulation.cc
index 89ab317..ba8f674 100644
--- a/rtgui/filmsimulation.cc
+++ b/rtgui/filmsimulation.cc
@@ -425,6 +425,8 @@

         if ( found ) {
             set_active( found );
+        } else {
+            set_active(-1);
         }
     }
 }

Now when switching to another image, the combo is back to "no value" and the film simulation is not applied.