Bauhaus sliders recompute pipeline more often than presumably necessary

[AxelG-DE]

Last night our very appreciated developer of the recent modules with regards to colour management has had yet another nervous breakdown on the IRC-Channel.

Please let us not discuss about that particular part of communication. We, the channel members, almost all of us tried many times to guide him to rational and respectful communication. Trying not to judge, formulate carefully: ...still some way to go...

I am not hiding the backgrounds to you guys, as it may shine thru here and there anyways. But please let us ignore this part. Try to keep the hard facts (which probably I cannot judge)

You guys know, I am not a coder. Please do not challenge me for technical correctness (actually finally I merely copied all of the context)

I do this for the sake of darktable. I was not participating this communication, I found it by reading backlog and also ongoing discussions today and liking dt a lot, I feel the necessity to speak up

I will copy things here and for those qoutes I try to make them legible as such, even they are not quoted from here but from #darktable.

bauhaus sliders set the values on button_pressed event AND on button_released too, meaning 2 pipelines get recomputed.

I just found out by accident while working on something else 23:07 (something else being : why the f*** do the slider take 3 s to redraw on heavy edit histories while GUI and pipeline live in different threads)

the sequence is set slider value on button_pressed, recompute pipeline, set slider value on button_released, recompute pipeline so the slider is actually redrawn only on the 2nd setting, which happens after the first pipeline recompute but even fixing that doesn't make the slider response much faster, so there should be something else and you get 2 pipelines recompute for free, in case your appartment was not hot enough that's what you get with copy-pasting shit you don't understand that code is 4 months old or so also the mouse coordinates over sliders are wrong

it's a very difficult computation : take widget width, subtract left and right margin, then subtract the color-picker width. That last one was forgotten

I might have seen it if I got requested for a review, but code changes are so many and get merged so fast...

--> later he said, he fixed it in his code base, so I think a "diff" should be possible to see, which solution was chosen. Still he said, it is not fully fixed yet...

Again, Ladies and Gentlemen, let us take the rational part only!!!!

What I got from the further communication, which I wouldn't like to paste here more as it brings no value add:

• it would be highly appreciated, if @TurboGit gives code sanity even higher priority than maybe already given
• @AlicVB would be very much appreciated to invest some energy for having the code KISS (keep it stupid simple) and have it in a way somebody with less engineering capabilities could maintain the code later
• and yes, maybe someone can find above technical topic in the code and amend it here, so that the rational background is getting solved, too

I am calling out for @Jiyone and @elstoc to backup here, as I cannot fully follow up for personal reasons

Let us give that Gentleman a rest. Don't even talk to him, I feel he is just burnout (not justifying anything). He is still part of our community!

[TurboGit]

Thanks @AxelG-DE for the straight message. I'm actually thinking I'm already doing my best for the code review, it is certainly harder to do better as there is many needed expertise on such a complex code as dt (SQL, shortcuts, Gtk, OpenMP, OpenCL, SSE - hopefully this is being removed -, Lua binding, JSON, ...). I'm not saying that we should strip down some parts, just that it requires expertise on many areas. At the moment I'm reviewing all code but I tend to do lighter review on code change from developer I trust as long time contributors and doing great stuff.

For sure I'm testing everything, and since many versions we have a more responsive UI. Yes there is many parts triggered for some actions, but at then end it is still "ok" and not workable as-is. It will improve when we fell that it becomes hard to work with dt. So some points are probably true, but it looks like "our very appreciated developer of the recent modules with regards to colour management" is just trying hard to harm the project at this time and discredit our work in favor of his fork.

At the end I just do not listen to him, and I encourage everyone to do the same. I also fell is just burnout.

[AxelG-DE]

@TurboGit I didn't mean to criticise you! Hope you got this right. ;)

Appreciate your honesty and acknowledge your outstanding work. Keeping a team together ain't easy.

We told him many times, "like this, nobody gonny listen to you, regardless how right you are"

But again, let's not digress into personal topics. I hope one day all reconciles.

Keep up with the potential bug if possible :)

[TurboGit]

@TurboGit I didn't mean to criticise you! Hope you got this right. ;)

Yes, I did got this right. No problem.

[HansBull]

Here is the corresponding issue in his fork: https://github.com/Aurelien-Pierre/R-Darktable/issues/29

[jenshannoschwalm]

@AxelG-DE thanks for reporting here :-) This issue certainly needs investigation and care.

(don't know how many dt devs are still on irc, i was for some weeks but dropped my account. Simply didn't want to be part of that game any more)

[dterrahe]

--> later he said, he fixed it in his code base, so I think a "diff" should be possible to see, which solution was chosen. Still he said, it is not fully fixed yet...

Recently a lot of variables in the slider code were renamed in the fork. This unintentionally makes it harder to see if this specific issue was addressed or to "pull" the fix into upstream dt. Especially when these changes overlap with other deviations in the fork.

Here is the corresponding issue in his fork: Aurelien-Pierre#29

The suggested solution to "set values only on button_released" would have as a consequence that the click that starts a drag would not trigger an update. So if you wanted to see if a click immediately gives the right result (and if so, release the button) and otherwise make small adjustment by dragging (before releasing) this would not work. Also, while dragging, several updates will be generated and then upon release another one, potentially from an unchanged spot.

It seems to me that if the press and release are in exactly the same location, then a pipeline redraw would not be generated, because no inputs have changed. So it looks to me like either the value calculation is slightly different (maybe the color-picker width was only forgotten in one of press/release) or the caching code fails.

[jenshannoschwalm]

I observe a difference if I scroll by the mouse wheel. If I drag there is a very bad delay. (did not try to get the reason for this yet)

[dterrahe]

Now that I'm home and able to test this behind the devel machine, I'm not really sure what this issue is about. I'm not seeing a separate "working..." when I release the mouse button (without dragging). The recompute gets triggered immediately on button press, as intended, and on button release the cache mechanism detects there are no changes so it doesn't recompute.

It would be helpful if the OP could be rewritten without the irc etc background and just explain the actual issue (maybe with a video). Thanks!

[elstoc]

Yes, we did ask him to raise an issue but he didn't

[dterrahe]

OK, so while I still don't know what the actual complaint here is, I've been looking at the gtk gui refresh process to see if it is possible to bring forward the moment the slider widget itself gets redrawn after a new drag event has been processed. And if we actually need to build in a delay to wait for the pixel pipe to have recalculated or if we can just update the values in the gui idle loop and have the pixel pipe pick them up whenever it is ready. Some simple experiments seem to work a little smoother on my side and I haven't noticed them causing problems. But someone with a fast gpu should till me if this actually makes a difference.

So @AxelG-DE would you be able to add a line immediately after https://github.com/darktable-org/darktable/blob/bfb4414f6b867a05635ff1e12d01d272edfa4333/src/bauhaus/bauhaus.c#L3138 so that it reads:

    }
    while(g_main_context_pending(NULL)) g_main_context_iteration(NULL, TRUE);
  }

and change https://github.com/darktable-org/darktable/blob/bfb4414f6b867a05635ff1e12d01d272edfa4333/src/bauhaus/bauhaus.c#L2887 to d->timeout_handle = g_timeout_add(0*delay, _bauhaus_slider_value_change_dragging, w); (i.e. multiply delay by 0)

Clearly the first change is a hacky way to try to temporarily increase the priority of the widget redraw while all processors are busy processing the pixel pipe in parallel. I haven't come across a clean way to tell gtk to redraw this widget now (without repeating a chunk of the main loop). If somebody does know a better way, please step forward. We could also reduce the number of processors allocated to OpenMP (and reserve 1 for the gui) but this seems undesirable to me.

The second change removes a long standing piece of code that limits the number of updates while dragging. I don't know if this still serves any purpose. It may have been superseded by a better caching mechanism, in which case replacing g_timeout_add with g_idle_add (which is basically what this does, I think) should be fine. Or there may still be another reason why the delay is required, in which case this would be playing with fire.

[dterrahe]

Yes, we did ask him to raise an issue but he didn't

@AxelG-DE seems to be able to reproduce the issue? I find that with discussions like this, different participants might have different ideas of what is being discussed. And since a lot of the copy/paste bit from irc is at the minimum a misdiagnosis (and at worst sees an issue where there isn't one) having this approached cleanly without unnecessary baggage would help.

[AxelG-DE]

@dterrahe I merely observed that dispute, smelled it could be important, also saw, there is at that point of time no way AP would report it here, so I just brought it here as a messanger.

Me, I cannot reproduce.

[jenshannoschwalm]

Or there may still be another reason why the delay is required, in which case this would be playing with fire.

Don't we do this to avoid reprocessing if only subtle parameter changes are pending?

[dterrahe]

Me, I cannot reproduce

Sorry, I misunderstood. An important requirement for a useful bug report is having someone interested in testing proposed improvements. I'm not going to try to make someone happy who has shown little inclination to return favors.

[elstoc]

I'm not going to try to make someone happy who has shown little inclination to return favors

If it is possible to improve performance that will make everyone happy.

[dterrahe]

improve performance

Define and I could have a look. Or you could report if the "fixes" I suggested above do anything to improve at least perceived performance. I can't reproduce the "double processing" on click/release, so can't fix.

The "problem" identified in the irc log seems to be based on a misunderstanding of slider updates and more of a general unhappiness with the level of intelligence of fellow collaborators. Which I can sympathize with, but unfortunately not provide a solution for, since I don't have a degree in psychology. It would surprise me if the proposed "solution" (ignoring button presses) would not make many people very unhappy.

[jenshannoschwalm]

I don't have access to my comp right now but when I tried to reproduce some days ago I couldn't. There might be a problem though

1. Could some devices "go wild" by reporting clicks&release instead of a drag?
2. Could this be an gtk/K-system issue?
3. In some heavy computing modules we might want to restrict the slider resolution to avoid unnecessary recalculations.

[dterrahe]

There might be a problem though

"Solutions" for imaginary problems can cause real problems in use cases they have not been tested with. And they will be hard to fix again because many years later people will assume there must have been a "good" reason for them and be afraid to touch them (justifiably, because they won't know how to test if the thing they were intended to fix still "breaks". See my question above about the delaying code and your answer to it).

1. Could some devices "go wild" by reporting clicks&release instead of a drag?

It is not "instead of". If there is a drag, it will be acted on. The question posed (and answered in the negative with infinite wisdom) was whether the click should be acted on. The correct answer is yes, because otherwise you'd introduce a delay if people click slowly. Or if they click and wait to see if a drag is required for further minor correction; if you don't process the click they'd wait indefinitely for nothing to happen.

Now what could be happening (a real bug report from someone who actually experiences a problem would be helpful here) is that a sensitive mouse (or pen; some people are known to occasionally use tablets) sends small drag (motion) events when just a simple click&release is intended. So you get click&drag&release and, since the value will now have changed at least twice, multiple recalcs. This should be dealt with in gtk/xwacom sensitivity or threshold settings since it would impact any other application as well. In gtk you'd use gtk-dnd-drag-threshold, I believe; @elstoc may know more because I think he had similar issues in the past.

Of course we could reimplement this functionality in dt itself (ignore small moves if they come too soon after the initial click), but as I said above, one persons fix is another persons bug, reducing responsiveness; there could be unintended interactions/overrides with OS functionality and we'd have to provide a whole set of configuration parameters so everyone can configure (and mess up!) to their heart's content. All while simply duplicating functionality that already exists in the OS/drivers...

The docs could have a section collecting configuration options in desktop environments that could be useful. Or maybe there already is one? Next to the section on how to best configure pens/tablets (once that is figured out).

2. Could this be an gtk/K-system issue?

Yep, possible. Impossible to know though without a full bug report.

3. In some heavy computing modules we might want to restrict the slider resolution to avoid unnecessary recalculations.

Moving a slider in an early ("light") module triggers recalculation in later ("heavier") modules in the pipe, so this would have to apply to all modules. And yes, the resolution is already limited/rounded to the number of digits shown (dt_bauhaus_slider_set_digits). People want high "accuracy" though, so good luck proposing to reduce the number of digits. Or some over-engineered way to make this configurable so everyone can be happy.

If you want to temporarily make the slider less sensitive, you can "zoom in" (make the soft min/max range narrower) by right-clicking and scrolling up. If you want to drag with discrete steps, you can hold alt/ctrl/shift. Or use mouse scroll. Pen/tablet scrolling is tricky though, so if they are the problem, this would not be a solution.

[jenshannoschwalm]

The issue might be related to hires screen also, the gtk threshold might want double value.

[dterrahe]

Or the phase of the moon. We won't know until someone who can reproduce the problem contributes a detailed bug report and offers to test remedies. Nobody involved in this discussion so far seems to fall into this category; I've not noticed anything untoward on my 4k screen myself.

[jenshannoschwalm]

And yes, the resolution is already limited/rounded to the number of digits shown (dt_bauhaus_slider_set_digits). People want high "accuracy" though, so good luck proposing to reduce the number of digits

We also have dt_bauhaus_slider_set_step, there are certainly module parameters where we could restrict precision without loosing accuracy.

[dterrahe]

dt_bauhaus_slider_set_step

I assume you are aware step size is ignored when clicking/dragging (unless you hold alt/ctrl/shift). It is used only for scrolling and shortcuts. Are you proposing to change that? People love fundamental changes in core functionality, so I suggest you don your teflon suit first.

there are certainly module parameters where we could restrict precision without loosing accuracy.

I would definitely agree with you that false "precision" is useless, but in the past adding precision has been much easier than reducing it. Then again, if you are wearing teflon anyway, why not!

Still, and I'm just repeating myself here, wouldn't it be better to clearly define a problem here first so that we can then test (and explain, when challenged!) that changes made improve the situation? The scope of code changes may be minor, but they would impact almost every UI interaction, so the parallel discussion about a process to pre-approve "large changes" would have some bearing here. Personally, I think I've had as much fun in this area as I can stand. Enjoy!

[aurelienpierre]

For sure I'm testing everything, and since many versions we have a more responsive UI.

Talk for yourself.

Yes there is many parts triggered for some actions, but at then end it is still "ok" and not workable as-is. It will improve when we fell that it becomes hard to work with dt.

That part of the GUI used to work flawlessly 4 years ago. It's inexcusable that it doesn't now.

So some points are probably true, but it looks like "our very appreciated developer of the recent modules with regards to colour management" is just trying hard to harm the project at this time and discredit our work in favor of his fork.

You have harmed to project beyond repair and reading the code is enough to discredit the work of the bunch of morons who are playing with their keyboard here. Problem is few people read C so everything looks alright. It's not alright. darktable is far from stable.

Fixing these regressions here is pointless since there is no protocol to prevent people to break them again in the future for the sake of useless GUI "improvement" designed with the ass to keep the geeks busy on weekends.

The only thing I'm doing on my fork is to fix your shit in way that ensures you don't merge more new bugs before I'm done fixing the old ones. Which is the current management style.

[dterrahe]

It's inexcusable that it doesn't now.

True. Except perhaps by again pointing out that it is hard to fix bugs that are not reported. The changes that I personally made to the behavior of bauhaus sliders were intended to not at all change the default behavior. The lack of bug reports has led me to believe (falsely, apparently) that I succeeded.

before I'm done fixing the old ones

I'm eagerly watching your repo for a fix to land there but you seem to be keeping it secret for now. The beauty of open source is that I might be able to help you understand the problem a little bit better if you explain to me what you think it is.

You have harmed bla bla

Of course there is no need for me to defend the maintainer, who has the thankless job of keeping everyone happy, with "help" usually only arriving after he spent months trying to massage PRs into shape and finally merging it. I assume he would be glad to accept offers of help to thoroughly review changes to particular areas and if I had done so for each large PR touching the gui, I would have felt more entitled to complain about the occasional small PR that got merged within a few hours without me completely agreeing with it.

Free software being what it is, everyone is entitled to try to do better in their own fork. Everyone can then decide, based on the actual code and the number of contributors attracted (not just coders, who may not be welcome, but also testers and documenters) which is the healthier project for their particular needs. And everyone will benefit, with good ideas and fixes staying portable for quite a while. Until of course the code bases start to diverge so significantly that merges are becoming more tedious and more likely to introduce fork-specific bugs and each branch has to stand on its own legs.

[dterrahe]

Obviously I missed https://github.com/Aurelien-Pierre/R-Darktable/pull/32 which was posted just before I sent the above.

It would be useful to get feedback on this from R&D users. Is it indeed unnecessary to update the image while dragging sliders, as dt has always done? I myself like to get immediate feedback while I'm for example adjusting exposure, but maybe the average user just finds this annoying or is willing to give it up to avoid the risk of multiple slow updates (if there are heavy modules later in the pipe) when accidentally dragging if just a click is meant. Would also like to get a feel for the prevalence of those accidental drags and what might be causing them.

Until of course the code bases start to diverge so significantly that merges are becoming more tedious

Automatic reformats will do that. Oh well.

[TurboGit]

Is it indeed unnecessary to update the image while dragging sliders, as dt has always done?

Yes to me. Otherwise why dragging? Just click on the slider randomly to different positions. It is for me very important to get feedback while dragging. As you say this has always been done and for good reason to me.

[TurboGit]

And not only for exposure, think about the crop module without direct feedback.

[AxelG-DE]

I am not sure, delay instant update was the aim of Aurélien's initial issue. It is really a big pity that he is not just clearly discribing what he found in a plain issue, like we all do :-/

• feedback indeed I want instant feedback and as performant as possible...
• User experience I do have a general issue handling a mouse. Moving to a place, stop motion, clicking... That move-stop-click, sometimes I mess the sequence, consequence: double clicks are sometimes not acknowledged. Related issue, the moment I want to click, accidently I move the mouse (not very sensitive setting)

All maybe not related.... :-)

[dterrahe]

• That move-stop-click, sometimes I mess the sequence, consequence: double clicks are sometimes not acknowledged.

Understood. To a large extend, we are relying on gtk to identify what is a click, a drag, a double-click etc. If we want to override/fine-tune some of its behavior, we often end up fighting it. The alternative would be to completely reimplement mouse handling but that would just be a nightmare (introducing bugs and subtle changes in behavior that people will hate).

The problem of clicking while the mouse is still moving (and thereby accidentally generating a drag and multiple updates) can be solved as https://github.com/Aurelien-Pierre/R-Darktable/pull/32 does by ignoring the initial click and the drag. Or we could slightly delay the response to it. This would not be too hard, but one person's "correction for oversensitivity" is another person's "slowness and unresponsiveness". When making a change we either have to get it perfect (which is impossible because people are different) or we'll upset everyone who has learned how to live with the old situation. Or we have to introduce more dreaded configuration parameters.

Another idea (that people will hate!) is showing the value that the mouse is hovering over, even before clicking and dragging, so the user can click only when they are at the exact location they want. I don't know if anyone would be helped by this, but just throwing it out there.

[elstoc]

Another idea (that people will hate!) is showing the value that the mouse is hovering over, even before clicking and dragging, so the user can click only when they are at the exact location they want.

I almost never know the exact number I want a slider to be at. I judge by what happens to the image when I change it.

[wilecoyote2015]

Is it indeed unnecessary to update the image while dragging sliders, as dt has always done?

Yes, I use this very often and I consider it essential.

[AxelG-DE]

@wilecoyote2015 you maybe want to edit your post and insert an empty line between quote and your text, to have it well separated

[Jiyone]

Another idea (that people will hate!) is showing the value that the mouse is hovering over, even before clicking and dragging, so the user can click only when they are at the exact location they want. I don't know if anyone would be helped by this, but just throwing it out there.

I don’t look at the value when clicking the slider, I keep looking at the pictures to catch the change.

Also about dragging the slider: I dont do it since the picture takes a long time to refresh.

[jenshannoschwalm]

I now mostly use rotate knobs on a midi device

[AxelG-DE]

Thanks @Jiyone to inform me about:

https://github.com/Aurelien-Pierre/R-Darktable/issues/29#issuecomment-1224344807

[AxelG-DE]

@dterrahe I didn't want to annoy you with connecting this. My motivation was, it is the first tangible statement in this regards.

So me, for my self I descriminated the rationales and the superfluously...

[dterrahe]

I didn't want to annoy you with connecting this.

Yes, that comment is disrespectful, upsetting and sad.

it is the first tangible statement in this regards.

No it isn't. It is a rant about coding style, quality and maintainability, but it doesn't point out any current "bugs" that can be "fixed" to alleviate the "issue" that only he himself seems to have.

I descriminated the rationales and the superfluously...

With respect, that is hard to do for a non-coder. You may notice that there are very few coders responding to his rants. That's not because we "admit" that he is "right". It is because we don't want to get down in the dirt with him and have technical "discussions" shouting insults back and forth. There's a trend in society where those who shout the feeblest arguments loudest claim victory because the other side finds nothing to even dispute. In open source, the code speaks. You want something fixed, fix it.

But my main issue is, quoting his last line:

This needs a rewrite and some heads to fall.

He is acting like people should be "fired" for not doing their "job". There is only one person actually getting paid to work on dt. It is a collective effort and if you see something going "wrong" you should do something about it; either fix it or influence others to change their behavior. If you don't pay attention for years to PRs that hit areas that you yourself have worked on in the past (and presumably have some expertise in) then you are as much to blame as anyone.

But all that aside, if he now wants to put an effort in and improve things, that would be great, of course. But he has decided to go his own way, making changes in his fork that will be hard to merge back in the main project because they are mixed with all kinds of irrelevance, like removing features he personally doesn't like and reformatting. He can be the ultimate authority there, but it means nobody will work "for" him because any work they do can be tossed out on a whim. If in the past he didn't have time to occasionally look at PRs and give people suggestions for improvements, how is he then going to be able to move the whole program forward on his own? All his changes, and merging back features and bug fixes in the main branch, after "cleaning them up", require extensive care and testing. And for us to pick up anything useful from his changes, we'd have to wade through pages of diatribe, like the link you provided, to even understand what he is trying to achieve.

There's a reason forks usually die. Without a clear mission, that's different from the main program, and a motivated community, they become dead ends. And in the process, aggressive rhetoric can do damage to the whole project. That's why we avoid engaging. And we may need to accept that he has put himself in a position where it is hard to imagine him contributing in a more constructive way again.

I'd love to be proven wrong.

[jenshannoschwalm]

In open source, the code speaks. You want something fixed, fix it.

That's the argument why I occasionally look into what's merged in the ranting fork.

[Mark-64]

I went into this bug report out of curiosity and read all the comments, but I am not quite sure if there is a problem or not. In the linked issue of the forked DT, two different problems are mentioned: 1) bauhaus sliders trigger a pipeline recompute first, then a GUI redraw 2) both button_pressed and button_released events trigger a new pipeline recompute

My reflections, testing on DT master on Windows 11, changing exposure on a heavy edit: About 1), the central image gets updated way after the slider, not before About 2), changing exposure by a single click / release on place does trigger exactly one (preview + image) pipeline recompute

So I would say I can't reproduce.

By the way, one curiosity that I have (and sorry if it looks stupid): when triggering multiple updates by dragging a slider, are all recomputes executed sequentially or instead an ongoing recompute is aborted when a new one is coming? The latter would be more efficient I guess.

[github-actions[bot]]

This issue did not get any activity in the past 60 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[github-actions[bot]]

This issue was closed because it has been inactive for 300 days since being marked as stale. Please check if the newest release or nightly build has it fixed. Please, create a new issue if the issue is not fixed.