Darktable: style & iop-order-version

[TurboGit]

Let's try to rethink about this.

• First I said (in https://github.com/darktable-org/darktable/pull/3140#issuecomment-541440005) that we should have a style iop-order and use some upgrade path, this is wrong

A style cannot have an iop-order-version as:

• a style can be created using iop from different images with different order versions
• a style can be created by step in time upgrading or adding more iop to it

So clearly at this point I don't see how we could have some meaningful style iop-order-version.

• If I'm not wrong about that, we can only record iop in a style without versions.

• That being said, when we apply a style we have no way to know in which iop-order-version it has been created.

• So I would say that we can only do our best by:

• assuming the style is designed for latest iop-order version to have best result
• use the iop-order-version of the image the style is applied to
• let users upgrade the style when needed to have best result on latest order version

What can we do more?

[junkyardsparkle]

Almost asleep, sorry if this makes no sense, but...

What about a "keep ordering" checkbox in the dialog (with a tooltip explaining the hazards) when saving a style from an image's history? Without the flag set by the checkbox, ordering is default for version for image applied to.

[spaceChRiS]

Why not carry the iop order version the style was created/last updated with anyway, to inform the user about a version mismatch whenever the style is applied/added to/updated to give the option to cancel the operation? In case of update/adding to a style, if not canceled, the style iop order version would be set to the current iop order version.

[jenshannoschwalm]

• assuming the style is designed for latest iop-order version to have best result
• use the iop-order-version of the image the style is applied to

Aren't these exclusive?

What can we do more?

I didn't look into the code yet, but it might well be we have a similar situation if we copy history from one image to another. (This is a guess, i'll check.)

I think styles have been implemented and kept as they are ignoring iop reordering at all.
The implementation might need a redesign, we don't want iop_orders as a constant module parameter but probably need a set of rules for each module included in the style _describing_ it's place in the pipeline.

If we really want styles to insert a module at a specific order this gets very complicated. In the end every style most likely needs a complete representation of all modules iop_orders when it was created. If such a rule is not existing we simply don't know where to put it.

[TurboGit]

Why not carry the iop order version the style was created/last updated with anyway, to inform the user about a version mismatch whenever the style is applied/added to/updated to give the option to cancel the operation?

Because there is no such safe order and anyway it can't be used at the risk of crashing dt. Using the iop-order of modules recorded into the style was the problem of a crash (now fixed).

[TurboGit]

Aren't these exclusive?

No, probably not clear. The first sentence "assuming the style..." is more for users. That is, for best result a style must have been created with latest developed images using latest iop-order-version. And in this case the order of the module in the style and the order of the module applied in new images will match. Best result, no crash, no issue.

In all other cases:

• style created with older edits and applied to new edits
• style created with new edits and applied to old edits

We cannot guarantee that the result will be ok.

Your patch cope with possible issues for those two last points, no crash.

And my thinking at the moment is that : we cannot do better than this.

But of course I'll be pleased to be proved wrong :)

[TurboGit]

but probably need a set of rules for each module included in the style _describing_ it's place in the pipeline.

I agree, sounds like something very delicate to get right. Do we really want to go this far?

[parafin]

I don't think this problem is solvable in general case. Here's an example:

Let's say default pipeline (forget about iop order versions for now) is a(1)->b(2)->c(3)->d(4) ( means enabled, d is some always enabled module like output color conversion). Let's move c before b, c gets priority 1.5. All modules get enabled and then style s is created from b(2) and c(1.5). Then on another picture user moves c before b, so c get's priority 1.5. Then moves d before b, d gets priority 1.75. Finally he moves b before c, so b gets priority 1.25. So pipeline now looks like that: a(1)->b(1.25)->c(1.5)->d*(1.75). Order is exactly the same as default, but priorities are different. So now let's apply style s. We have 2 options:

1. don't take iop priority from style - then the fact that c was moved before b when style was created gets lost;
2. copy iop priority from style - then order will be a(1)->c(1.5)->d(1.75)->b*(2), c is correctly before b, but now we have d in-between them, which is wrong.

Basically we had option 2, now we have option 1. While in theory option 2 shouldn't lead to crashes and that can be fixed probably, it still won't solve the fact, that iop order is wrong. I can see the only way out of this:

1. Store the whole pipeline iop order in a style and use it when applying a style. It can be optional feature for a style. Obviously it will make style incompatible with other styles with different order and user re-ordering (at least before style is applied). Also I'm not sure how new modules in future versions of darktable should be handled when applying the old order without them.

[parafin]

As for "set of rules for each module included in the style describing it's place in the pipeline" - I don't think it can be created automatically (aside from solution when order of the whole pipeline is saved) and even if we create a way for user to describe dependencies (I can't imagine UI for it) it can get tricky to resolve them if there are conflicts (probably just decline to apply a style?). This should have been thought through when iop re-ordering feature was introduced. One should expect this type of consequences when introducing such a big feature implemented just as a hack on top of existing infrastructure not designed for it. On the other hand without iop re-ordering there was only option 1 anyway, so it's not like this is a regression. Just a missing feature.

[jenshannoschwalm]

I agree, sounds like something very delicate to get right. Do we really want to go this far?

No. It will be hell to debug and maintain if even possible. Also @parafin gave more examples.

While in theory option 2 shouldn't lead to crashes and that can be fixed probably

I thought so too. But there are combinations of iop_orders that make dt crash. Keeping a "don't do" list? Also will create lot's of troubles.

This should have been thought through when iop re-ordering feature was introduced

Right. We now can only fix.

[junkyardsparkle]

On the other hand without iop re-ordering there was only option 1 anyway, so it's not like this is a regression. Just a missing feature.

Well, yes and no... from a user perspective, the fact that it's now possible to create a style which will not give expected results when applied (because it was created after relocating, for example, the lut3d module) could be seen as a regression. :-)

As far as approaching it as a "missing feature", I'll give an example of my own use case: I use keyboard with extra macro keys which I use extensively for all kinds of stuff. In darktable, before the iop ordering, one use was mapping a shortcut to a module preset, for example various different subtle vignetting presets. I find that the vignetting module gives much more "lens-like" results used much earlier in the pipe, so now I map shortcuts to styles rather than presets for the purpose of moving it there when applied. Maybe there's a better way to approach this use case than styles... but that wouldn't fix the problem mentioned above, whereas fixing that problem would also solve this one.

Random, possibly stupid half-awake idea: what about a heuristic approach, with sane fallbacks for ambiguous cases? What if, rather than storing the complete module ordering in a style, or any specific ordering value, we store the preceding and following module for each module active in the style? It seems like this could work in many cases, and degrade gracefully when it doesn't... again, sorry if I'm overlooking something obvious.

[parafin]

Feature that works only sometimes is IMHO bad design. How do you document it? How do you handle bug reports about it? It will be very hard to distinguish intended behaviour from a real bug.

[junkyardsparkle]

Feature that works only sometimes is IMHO bad design. How do you document it? How do you handle bug reports about it? It will be very hard to distinguish intended behaviour from a real bug.

This is already the case for the current situation. The idea is to improve on that. If, in the worst case, we simply fall back to the defaults, but in a typical case of somebody creating a style for use within their normal workflow, do something a little smarter, I don't think this is bad design from a user perspective, and will actually result in fewer bug reports. I could be wrong, but I don't think this behavior would be any harder to document than anything else.

[jenshannoschwalm]

I don't think this is bad design from a user perspective, and will actually result in fewer bug reports.

I disagree here. It's not about 'user perspective', the software design (related to styles) we have atm simply is not good enough and as such not maintainable if we want a reordered pipeline. I think we just have to keep it as-is and see for the next release.

So for me for dt3.0 "A style is a bag of modules including module parameters applied (or excluded) to an image.

One more point, there are styles around implementing film emulation like t3mujinpack or from the styles website. They all want default iop_orders so the all would break development somehow.

About the number of issues, @Turbogit counted trillions (if i remember correctly) related to iop_order. There will be so many issues / questions like "why does this look bad". Of course what we have is a restriction but at least we can be sure that wrong iop_orders have been done manually and not by dt itself (Did you watch aurelians video and saw him trying to understand modules reordered?)

[junkyardsparkle]

I disagree here. It's not about 'user perspective', the software design (related to styles) we have atm simply is not good enough and as such not maintainable if we want a reordered pipeline. I think we just have to keep it as-is and see for the next release.

Ok, I'll stop thinking about it for now. :-)

One more point, there are styles around implementing film emulation like t3mujinpack or from the styles website. They all want default iop_orders so the all would break development somehow.

TBH, so much has changed since those old styles were created, I think they all need to be considered broken at this point... many of them probably use modules that are now deprecated, after all.

[jenshannoschwalm]

There is one more iop_order / history problem i am aware of.
int dt_history_copy_and_paste_on_image(int32_t imgid, int32_t dest_imgid, gboolean merge, GList *ops)

We obviously have these different situations,

1. If for both images the iop_order version is identical, no problem, just go.
2. If we do an overwrite we can do so if we copy everything including the iop_order version
3. If the is a paste&merge with different iop_orders, how should this be handled. imho this will be pretty rare and using default iop_orders will probably be bad. I would opt for a "don't handle" and maybe give a warning message.

[TurboGit]

On my side I certainly do not agree with part of this discussion. The style are not broken, the styles have not 100% the same effect ok, but frankly a style need often to be adjusted for each image. It is a starting point to me.

Note that I have some old styles done with some done with dt 2.2 and they actually work ok today on my pictures developed with dt master.

[TurboGit]

@jenshannoschwalm :

For 1. : OK

For 2 : OK + update image iop_order_version

For 3 : I'll do as for style, merge into the destination image with the iop-order of its iop-order-version.

[jenshannoschwalm]

@TurboGit ok. I'll take care of it asap, ok?

[parafin]

Feature that works only sometimes is IMHO bad design. How do you document it? How do you handle bug reports about it? It will be very hard to distinguish intended behaviour from a real bug.

This is already the case for the current situation. The idea is to improve on that. If, in the worst case, we simply fall back to the defaults, but in a typical case of somebody creating a style for use within their normal workflow, do something a little smarter, I don't think this is bad design _from a user perspective_, and will actually result in _fewer_ bug reports. I could be wrong, but I don't think this behavior would be any harder to document than anything else.

Right now we can define style as applying just iop params, not iop order. Then they work reliably and it would be relatively easy to identify bug reports when more is expected from styles. Your alternative gives expectations that iop order is also handled, but when it's not it would be very hard to explain what's going on or how to fix it. Especially you have to consider a case, when several styles are applied to the same image. For example if user has a style automatically applied on export, he wouldn't expect modules rearranged because of that.

[TurboGit]

@jenshannoschwalm : yes please, and thanks again for all your work on iop-order!

[TurboGit]

@jenshannoschwalm : BTW issue #3153 may be related to this.

[jenshannoschwalm]

Unfortunately i think "yes it is". Maybe also #3123

[junkyardsparkle]

Right now we can define style as applying just iop params, not iop order. Then they work reliably and it would be relatively easy to identify bug reports when more is expected from styles.

Yes, I understand why this is attractive. I just suspect it will result in some user pain, no matter how well documented... but maybe I'm wrong, we'll see. Anyway, this being the case...

If the is a paste&merge with different iop_orders, how should this be handled. imho this will be pretty rare and using default iop_orders will probably be bad. I would opt for a "don't handle" and maybe give a warning message.

Could we please do this? I think it's absolutely sane given the way copy/paste is likely to be used, and will effectively now be the only way to use a module in a non-default position without a ridiculous amount of drag'n'dropping. IMHO not doing this would badly cripple the new reordering feature.

[jenshannoschwalm]

@TurboGit Close this issue for now? I don't see a bug any more and would suggest if anyone wants to implement another way-of-style: do it later.

[junkyardsparkle]

No discussion of that last point? I think @jenshannoschwalm is correct that using defaults will be bad in this case, but if I'm reading the response from @TurboGit correctly:

For 3 : I'll do as for style, merge into the destination image with the iop-order of its iop-order-version.

then this is what's planned. Was there ever an argument against the original proposal?

[TurboGit]

Was there ever an argument against the original proposal?

Sorry I'm lost... Not sure about what you mean or you are referencing.

[junkyardsparkle]

@TurboGit Sorry if unclear. I'm referencing this post, where @jenshannoschwalm states for point 3 that using the defaults would probably be bad, and your response two posts later, where it seems that this point is ignored (correct me if I'm reading it wrong).

EDIT: As of #3174 this seems to be working as originally described, so it's all good. Sorry for the noise. :-)

[junkyardsparkle]

@jenshannoschwalm @TurboGit It appears that maybe styles appended with the export module are still applying modules in non-default-for-destination-image positions; see #3333