-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Raster Mask: PNG support and image folder #20170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Aah :-) Had a quick first look right now with nothing appearing bad to me now. will review and check the next days. Also, welcome to the party! |
TurboGit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested yet. Two points:
See 66d9708 for the updated _check_extension().
Also be prepared for conflicts when #20198 is merged.
Do we really want the extra complexity for enforcing the mask directory. Current behavior which is remembering the last used directory seems just fine to me.
All in all I think you should separate the work in two PR. One for supporting PNG and one for the root directory to be discussed.
TIA.
src/iop/rasterfile.c
Outdated
|
|
||
| char *extension = g_strrstr(filename, "."); | ||
| char *ext_lower = extension ? g_ascii_strdown(extension, -1) : NULL; | ||
| const gboolean is_png = ext_lower && !g_strcmp0(ext_lower, ".png"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify with : g_ascii_strcasecmp()
|
I looked at https://github.com/darktable-org/darktable/pull/20198 and found that it makes my pull request irrelevant. PNG files were needed to avoid keeping large pfm files on our drives. With vectorisation, we no longer need to keep the mask files, so the relative path is also irrelevant.
If this is implemented (or Lua like you suggested), the image folder will also no longer be needed. In my opinion, this PR can be rejected now. What do you guys think? |
So maybe a PR just for png now? |
|
Agreed, PNG support for now would be good. |
|
Good. I'm on it! |
467f319 to
c698372
Compare
|
I've updated the PR to png support only. I'm not sure what the right procedure is, but I think when #20198 is merged, I will rebase, check for conflict and push again. Let me know if this is fine. |
|
Is there a function that I can call with a filename to load the mask file? I'm thinking of Lua integration being able to load a mask from a script. |
|
Right now there isn’t a function to load the mask file from Lua, but that would be nice. I tried looking at how this could be done, but I don’t understand the code enough yet to pull it out. |
|
I just need the name of a function in your module that I can call with a filename. I can handle interfacing it to Lua. Let's say int your module there is a function called dt_load_external_mask(const char *fname) that would load the mask file and make it available to the pipeline. I would then add a function to the Lua API to load a mask that would basically take a filename and then call dt_load_external_mask or whatever it's called. |
The png support fixes issue #19667 and part of issue #19829.
The image folder possibly fix #19689
The relative path fixes issue #20088