GNOME Bugzilla – Bug 764801
pipeline: Place gegl:crop before photos:magic-filter, and rethink some implementation details
Last modified: 2016-09-09 12:39:15 UTC
Currently if you select a magic filter with a vignette followed by crop, then the vignette will get clipped. This is not good because if the magic filter added a vignette then the crop should not interfere with it. If the user doesn't want a vignette, then she can either use a different magic-filter preset or just mix and match the other tools. Secondly, we should move towards having fixed positions for all the operations. That is what prominent non-destructive editors do. eg., Darktable [1] and Shotwell [2]. We won't be able to change the order for images edited in previous versions because it is already baked in the XML, and, I think, that is fine. [1] https://www.darktable.org/usermanual/ch03s02.html.php [2] https://wiki.gnome.org/Apps/Shotwell/Architecture/PhotoPipeline
See darktable/tools/iop_dependencies.py for the exact order used by Darktable.
Created attachment 325607 [details] [review] pipeline: Mark nodes as passthrough instead of actually removing them
Created attachment 325608 [details] [review] pipeline: Refactor connecting a list of nodes into a separate function
Created attachment 325609 [details] [review] pipeline: Refactor photos_pipeline_reset to be more extensible
Created attachment 325610 [details] [review] pipeline: Assign a fixed position to gegl:crop
Review of attachment 325607 [details] [review]: Looks good. I'll have to update my Discard all edits patch, since it didn't account for pass-through nodes.
Review of attachment 325608 [details] [review]: Nice.
Review of attachment 325609 [details] [review]: Looks good overall. ::: src/photos-pipeline.c @@ +109,3 @@ + node = gegl_node_new_child (self->graph, "operation", OPERATIONS[i], NULL); + gegl_node_set_passthrough (node, TRUE); + g_hash_table_insert (self->hash, g_strdup (OPERATIONS[i]), g_object_ref (node)); Could we avoid strdup'ing the operations' names for every pipeline? If we don't pass a g_free when creating the hash, we could just point to the string in the static array.
(In reply to Rafael Fonseca from comment #8) > Review of attachment 325609 [details] [review] [review]: > ::: src/photos-pipeline.c > @@ +109,3 @@ > + node = gegl_node_new_child (self->graph, "operation", OPERATIONS[i], > NULL); > + gegl_node_set_passthrough (node, TRUE); > + g_hash_table_insert (self->hash, g_strdup (OPERATIONS[i]), > g_object_ref (node)); > > Could we avoid strdup'ing the operations' names for every pipeline? If we > don't pass a g_free when creating the hash, we could just point to the > string in the static array. We can remove the strdup once we no longer need to insert operation names that point to non-static memory. eg., in photos_pipeline_add and when deserializing the XML. I think we can handle photos_pipeline_add when we have a fixed position for everything. It needs some research on the placement of the other operations. Then, we can populate an empty graph before deserialization, read the properties and update the existing nodes, instead of creating new nodes.
Comment on attachment 325607 [details] [review] pipeline: Mark nodes as passthrough instead of actually removing them Pushed to master after tweaking the commit message to be less rambling.
Review of attachment 325608 [details] [review]: ::: src/photos-pipeline.c @@ +69,3 @@ + + node = GEGL_NODE (nodes->data); + gegl_node_link (input, node); Would be nice to handle nodes == NULL, because it will simplify the calling function even more.
(In reply to Rafael Fonseca from comment #8) Sorry for not replying sooner. This bug fell off my radar. > Review of attachment 325609 [details] [review] [review]: > ::: src/photos-pipeline.c > @@ +109,3 @@ > + node = gegl_node_new_child (self->graph, "operation", OPERATIONS[i], > NULL); > + gegl_node_set_passthrough (node, TRUE); > + g_hash_table_insert (self->hash, g_strdup (OPERATIONS[i]), > g_object_ref (node)); > > Could we avoid strdup'ing the operations' names for every pipeline? If we > don't pass a g_free when creating the hash, we could just point to the > string in the static array. Yes, good point. I think we can do it once we have figured out fixed positions for all the operations, because then all the strings would be in OPERATIONS.
Created attachment 335183 [details] [review] pipeline: Refactor connecting a list of nodes into a separate function
These patches solve the problem with cropping interfering with vignettes. Plus, once an image has been edited, the order of the operations should should stay the same unless we revert back to the original. The ordering is: crop -> magic-filter -> (others - in the order in which they were 1st used) We don't interfere with the order of edits done by previous versions of the application. This should address the most pressing problems. In future, I think we should do what other non-destructive editors (Darktable and Shotwell) have been doing. See below: (In reply to Debarshi Ray from comment #0) > Secondly, we should move towards having fixed positions for all the > operations. That is what prominent non-destructive editors do. eg., > Darktable [1] and Shotwell [2]. We won't be able to change the order for > images edited in previous versions because it is already baked in the XML, > and, I think, that is fine. > > [1] https://www.darktable.org/usermanual/ch03s02.html.php > [2] https://wiki.gnome.org/Apps/Shotwell/Architecture/PhotoPipeline (In reply to Debarshi Ray from comment #1) > See darktable/tools/iop_dependencies.py for the exact order used by > Darktable.