GNOME Bugzilla – Bug 134956
Curves tool doesn't save free curves
Last modified: 2008-10-09 15:33:25 UTC
submitting this using mozilla, hopefully linebreaks will work. steps to reproduce: 1. open layers->curves 2. select free curve mode 3. draw a line continuously from far left to far right, staying at the top. 4. go to the bottom, and click repeatedly while moving right->left (staying at the bottom) 5. what you have now should look like a barcode. save the curve. 6. load the curve. it now looks like a few gradients :( perhaps i'm abusing curves; i don't know any other way to apply a brightness LUT with precise control. however this behaviour is confusing and nonintuitive.
I reproduced it here... quite ugly, indeed. The curve is simply reset to smooth, and is redrawn as a spline. :-(
Found the problem... In curves_write_to_file (GimpCurvesTool *c_tool, FILE *file), inside app/tools/gimpcurvestool.c No association specifing the curve type is recorded in the file. I would suggest creating a new version of the curves file, in which a second, textual, line of information, would describe the curve as FREE or SMOOTH. I see no reason, in case a second file version be made, not to write all the curve points to the file, instead of sampling them at even spacings.
Hi, Yup - verified that the curve loading doesn't do anything to take into account the curve type. If the curve is a free curve, it simply takes samples every 8 pixels and makes those pixels the control points for the loaded curve. This would be a fairly straightforward fix, but requires a change in the curves file format, and as such can't go in before a stable release. It should be addressed for 2.2, though. The relevant file is app/tools/gimpcurvestool.c, and the functions to modify are curves_read_from_file() and curves_write_to_file(). For consistency with other metadata files we write, the files should be versioned so that we can continue to load old files after the change. A possible file format would be something along the lines of Magic string (same as now) #(version number) #(curve type) And then, as we currently have, a set of control points for a smooth curve, or alternatively a series of 256 points per channel for the 5 channels if the curve is free. Something is required to distinguish the version and type strings from existing valid curve tool definitions, which is why I've preceeded the version and curve type with comment symbols. In the line after the magic string doesn't start with a #, we're in "classic" mode. It's not particularly smart, but it should be good enough. Any better ideas? Cheers, Dave.
I had actually starting editing the file before I even read in your comment. (I will not be able to finish it today though ... I am eating into working time ) It won't change any strings and it won't cause incompatibilities, so I think that if it might be readied in time for 2.0 it could go in without any problens. As for the versioning scheme you suggested, there is a small issue that would cause an ugly hack: If I read the first character in the second line and it is not a "#", it should be part of the curve points information. I was inserting the version number into the magic string - but that is not smart either. So I thought of working with 2 different magic strings: The current one - "# GIMP Curves File\n" for version 1, and "# GIMP2 Curves File\n", for versions 2 and above - which would include the version number on the second line. What do you say?
Hmmm... it would work, but it seems even more hacky than my idea. As you say - you're more or less just writing the version number in the magic string. I don't see a problem with reading the first character, and if it's a # read a version string, otherwise keep reading characters until a \n, then do what we currently do on the result. And the problem is always going to be that if we change a file format days before the stable release, we risk breaking stuff and having to delay it. I really think this should wait a while and be committed after we branch. Dave.
Created attachment 24615 [details] [review] Fix Bug by creating new version,and versioning system, for GIMP curves files. COMPILED, BUT NOT TESTED.
HI...I think I made it. Unfortunattely I was given a machinne with "debian testing" here at the office, and could only manage to build the GIMP inside a SID chroot I made. But something in the build proccess broke the hack I was using to run the GIMP from inside the chrooted enviroment, so <b> I could not test this patch </b>. Therefore, I ask somebody to test it. If it works, or is made to work, I do not see why not to include it for 2.0, since only this file reads The GIMP curves files, so there is no risk of breaking compatibility with anythi
doesnt work for me :( with the patch, as soon as i hit 'save' for a free-curve, gimp crashes.
This is certainly not a major problem, but simply an enhancement request.
Debian testing is the perfect environment to build GIMP2 since it provides all dependencies. Why are you fiddling with a sid chroot environment?
I had a look at your patch and have a few comments: Please don't waste your time on extending the curves file format in such an akward way. This change won't make it into 2.0 so we can as well fix it properly. We need a GimpCurve object in multiple places like for example to control the pressure mapping of paint tools. Such a curve object should implement the GimpConfigInterface and can then reuse a lot of existing code. So, please, lets deprecate the old format and replace this crap with more general code that can then be reused all over The GIMP.
Allright then Sven. I will not update it here, as I find the curves fixed in this 8 bit / 16 handlers form rather dull. I'd like to see floating point curves for 2.2, and not waiting for GEGL, tough. Neota, however if you do need saving free-form curves to have your work done, my patch may work - I could test it today and it is not at all ready, but I could make it a stand-alone patch - that _you_ would have to compile in your personal GIMP - so you won't have to wait for gimp 2.1 to be able to save free-hand curves do get your job done. I propose closing this as "wont-fix" and opening another bug to track a general curves object for GIMP 2.1 them.
Created attachment 24849 [details] [review] Patch - fixes bug 134956, extends curves files to save Freehand Curves and add versioning system to them.
Hi Sven, Please don't call someone else's approach to a problem a waste of time. If you have a solution that's better than the one that we've talked about here, propose it. It's hard enough to keep motivation up without someone implying that something you've spent time on is useless. Dave.
You misunderstood me. Let me try to rephrase it then... Please don't waste your valuable time with this approach. We will need a more generic curve object in multiple other places and your efforts would much better be spend on this instead. If you need more detailed explanations or hints on where to start, please let me know.
Thank you Dave and Sven, I've been around a couple of months and can outlive Sven´s sometimes looking harsh wordings just fine. As for this bug, good ro bad, the latest patch does fix it, and I see no reason for it not to go in 2.0.1 - Anyway, backwards compatibility from an all new, reviewd type of curves to a "2.0.1 gimp2 curve files" won't be needed for that many people. And if someone needs it, I could always provide a python script to convert from the format created by this patch to the GIMP 2.2 one. Anyway, just as it won't hurt to put this in, it won't be a big difference for most users if freehand curves can not be saved for another 6 months -- the proof of it is that this bug was reported just now. As for me, I am still finishing the Portuguese BR translation of GIMP and components, and finally will be able to get a couple hours a week to look over the GIMP. I plan to work first on some new features on the Postscript plugin, as I had noted in a bug here somewhere, and see were else I can help. I also intend to work on the Custom Layer Combination Mode issue I've mentioned a couple of times.
Given the existing GimpConfig infrastructure, a serializable GimpCurves object with an extensible format can probably be hacked up quicker than any converter you'd have to write to support the suggested gimp2 curves file format. For this reason, I'd rather not see a new format being introduced only for the purpose of replacing it a few weeks later (and providing backwards compatibility to it since we always do that).
Created attachment 25455 [details] [review] Updated patch - for those who need the feature now.
Please Sven, do not take this as a chalenge or something - I agree with the way you suggest. I will leave the patch here just for people who need this feature before it gets implemented (for instance - to apply a special curve effect on all images in an animation), get a chance to do it, since the work to is done anyways. Known issue: :-( A curve taht uses freehand had to be loaded twice for it to work properly. I could not work around that.
No problem.
As of sept 12/2007 (3 years after the original fix), this bug still exists in GIMP version 2.2.13 (ubuntu).
Not surprising, none of the patches has been committed.
totem still shouldn't be restoring the screen back to the maximum size if it didn't change the resolution in the first place. Consider this use case: 1. I change the resolution to 800x600, so I can attach an external screen that only works in that resolution 2. I start totem's fullscreen mode, which doesn't change the resolution and works great 3. Now I want to start a new movie, so I exit fullscreen mode 4. Totem changes the resolution back to the maximum, so I now have to attach the original monitor again to restore the resolution I want.
Argh, sorry for the spam. My save-textarea script screwed up.
Damn, this was actually one of the main goals of all the hacking on image map tools for 2.6, but i simply forgot to finish it. It's quite easy, but not a trivial change. Will attach a patch to go into 2.6.2.
Created attachment 120269 [details] [review] Patch implementing the feature based on the new infrastructure
Fixed in both branches: 2008-10-09 Michael Natterer <mitch@gimp.org> Merged from trunk: This is not exactly a bugfix-only commit, but fixing this bug was the original reason for all the curve and image map tool refactorings that went into 2.6. It would be silly not to fix it just because I simply forgot to hack the final step of enabling all the new code. The two new translatable strings are a PITA, sorry about that; but better than still only exporting the old curves and levels formats in 2.6. Bug 134956 – Curves tool doesn't save free curves * app/core/gimpmarshal.list * app/widgets/gimpsettingsbox.[ch]: add signal "file-dialog-setup" and emit it when the export/import file chooser is fully constructed. Callbacks can then do additional things to the dialog, like adding custom buttons. * app/tools/gimpcurvestool.h * app/tools/gimplevelstool.h: add boolean member "export_old_format". * app/tools/gimpcurvestool.c * app/tools/gimplevelstool.c (gimp_*_tool_dialog): connect to the settings box' "file-dialog-setup". (gimp_*_tool_export_setup): new callback which adds a toggle to the file choosers that allows to export to the old format. Default saving the new format, we defaulted to the old one before. (gimp_*_tool_settings_export): check the "export_old_format" boolean and only save the cruft format if it is TRUE; chain up otherwise, which generically saves the new format. * app/tools/gimplevelstool.c (gimp_levels_tool_settings_import): add the same file format detection code as in the curves tool so it transparently loads old and new levels files.