Refactored enfuseAdvanced #179
Conversation
I was having issue wit this script periodically. The original version of this was real a hack-job of Holger Klemms original. This new version utilizes tables in a manner that I hope will make the code easier to follow and understand, but also allows for certain functions to be re-used throughout (and potentially in different scripts as well). In general: Each program (align image stack, enfuse, exiftool) how has a table that contains information about that program, this includes a sub-table that contains argument data for the program. There is a GUI table which contains all gui elements. Those elements are split into sub-tables within GUI based on what aspect of the scrpt behavior they relate to (AIS, ENF, Target File, Presets, etc...) These two structures (and the similarities between them) have allows me to create loops which populate the arguments from the GUI and vice-versa, this is all done through an intermediary preference, the "active" preference. Whenever a change to a GUI element occurs the associated member of hte active preference is updated. then when it becomes time to build an argument string for a program command the active preference is read from and the value contained within is used. This works fine for all GUI elements that have a "changed" or "clicked" callback, as they can remain up-to-date real time. Sliders and Entry boxes do not have this, so at a certain point withing the script the GUI values are read directly for these elements and written to the associated active preference field. As you will see about 50% of this code is simply building all the GUI elements and making sure that the initialization value for that element is valid (via the InRange() function). The actual heart of the program and it's functions really only takes up about 300 lines (starting at 180 and going through 476). That is broken down into multiple fairly concise functions. Hopefully this makes maintenance, feature enhancement, and troubleshooting much easier in the future. I also made a few UI tweaks. This script use to create such a massive list of GUI elements that it was a nuisance when actually using it. They are now grouped into three containers for AIS options, Enfuse options, and target options. Only one of these options containers is displayed at a time. Additionally, I removed all the registered preferences I had before, so now I only register 3 options to enable user to point towards the program binaries.
|
Why not add a fourth container for configuration and put the executable_path_preference widgets there so the use can just configure the program on the fly and not have to restart darktable. |
True. I originally avoided doing that as it would add even more clutter to the UI. But now that I have containers it wouldn't really make any difference at all. I also made a comment of in the HDRMerge PR that applies here: |
| GUI.AIS.control_points, | ||
| GUI.AIS.control_points_remove, | ||
| GUI.AIS.correlation | ||
| } |
There was a problem hiding this comment.
Did you try doing table.unpack(GUI.AIS)?
There was a problem hiding this comment.
No. I don't know what that is. I'll have to look it up
There was a problem hiding this comment.
I'll look into the unpack function. However, things are working as-is, and this is already a big step forward on refactoring this, so let's not let table.unpack implementation hold up this PR. If I can get around to implementing it in the future I can do so.
|
I'm going to download the source file and go thru it and add comments. It's really hard to get a good grip on what you've done in the diff view provided here. What I can see looks good. I think you might be able to take it a little farther and specify your gui elements in the tables and just call a routine to build them, but I need to see the script as a whole to figure it out. |
|
You might want to look at contrib/video_ffmpeg.lua. @theres had some preference read and write functions that looked pretty slick and might be useful. There's also a placeholder replacement function so that you could replace {datatype} with bool, string, int, etc. |
|
Linux can determine executable paths using which, and it works. MacOS needs to specify the location to be safe, and windows needs to specify the location. So, if dt.configuration.running_os ~= "linux" then |
The executable choosers have been moved to a box which is now element 4 in the GUI stack. Once the user configures these and presses update they are confirmed good or bad and the GUI changed to normal view if all works out.
Made text translatable except for line 499 which kept giving me problems
Making L499 translatable would throw the following error lua:499: attempt to call a nil value (upvalue '_')
Troubleshooting steps taken and results:
1) Changed the translation function from _() to translate() - this worked, no errors but ugly
2) Reverted the changes from 1, then changed all "for _, var in table do" to be "for ignore, var in table do" - this did not help
Moved a chunk of code due to anomalous behavior, I suspect previous code was attempting to change the value in the stack widget before it was fully created. Adding in a print statement before trying to change the stack.active would cause things to work fine. Moving it behind the register call also helped.
|
Is this version better then the old one and does it run on all OS? If yes, lets merge it. It can still be improved later. |
This does run on all OS, though the old one did too. It is my belief this is indeed better than the old. Here's why:
|
I was having issue with this script periodically. The original version of this was real a hack-job of Holger Klemms original. This new version utilizes tables in a manner that I hope will make the code easier to follow and understand, but also allows for certain functions to be re-used throughout (and potentially in different scripts as well). In general:
Each program (align image stack, enfuse, exiftool) now has a table that contains information about that program, this includes a sub-table that contains argument data for the program.
There is a GUI table which contains all GUI elements. Those elements are split into sub-tables within GUI based on what aspect of the scrpt behavior they relate to (AIS, ENF, Target File, Presets, etc...)
These two structures (and the similarities between them) have allows me to create loops which populate the arguments from the GUI and vice-versa, this is all done through an intermediary preference, the "active" preference.
Whenever a change to a GUI element occurs the associated member of hte active preference is updated. then when it becomes time to build an argument string for a program command the active preference is read from and the value contained within is used. This works fine for all GUI elements that have a "changed" or "clicked" callback, as they can remain up-to-date real time. Sliders and Entry boxes do not have this, so at a certain point withing the script the GUI values are read directly for these elements and written to the associated active preference field.
As you will see about 50% of this code is simply building all the GUI elements and making sure that the initialization value for that element is valid (via the InRange() function). The actual heart of the program and it's functions really only takes up about 300 lines (starting at 180 and going through 476). That is broken down into multiple fairly concise functions. Hopefully this makes maintenance, feature enhancement, and troubleshooting much easier in the future.
I also made a few UI tweaks. This script use to create such a massive list of GUI elements that it was a nuisance when actually using it. They are now grouped into three containers for AIS options, Enfuse options, and target options. Only one of these options containers is displayed at a time. Additionally, I removed all the registered preferences I had before, so now I only register 3 options to enable user to point towards the program binaries.