Skip to content
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

Darkroom second window #2365

Merged
merged 6 commits into from Apr 12, 2019

Conversation

Projects
None yet
4 participants
@edgardoh
Copy link
Contributor

commented Apr 9, 2019

This add a second window to darkroom.

I have created a new pipe, preview2, that is used exclusively for this.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

It still needs more testing, in particular the zoom, not sure how it works on modules that uses the preview pipe for the full image.

@TurboGit TurboGit self-requested a review Apr 9, 2019

@TurboGit TurboGit self-assigned this Apr 9, 2019

@TurboGit TurboGit added the enhancement label Apr 9, 2019

@TurboGit TurboGit added this to the 2.8 milestone Apr 9, 2019

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Looks good and working on my side. This is exactly what I had in mind for dt, better than detachable panels to me.

The only thing is that the second window does use the color profile of the main screen, so it is color managed but a single profile is used for both window. I suppose we want to be able to choose a profile for this second window and use it in the corresponding new preview2 pipe. A menu to select the profile on the new window icon like the menu on the overexpose button would be nice.

What do you think?

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

For me there's a big issue: I don't have a way to to test it, and I don't like to fly blind (my monitor is not calibrated and I won't buy/rent the required hardware in the near future).

Sure, I can test this and help in the implementation when the circuitry is in place.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

But is not that easy, right now the system profile detects the profile of the display where the main window is, we should do the same with the system profile2 on this new window

I won't do that (at least for the first implementation). I think a choice of profile to select will already be good.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

probably add a new display profile2 below the display profile combo with a 'system profile2' entry and then all the other profiles.

Right, or as I proposed to add this on the new icon you've added to activate the second display?

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

And for the detachable panels I don't think this will work at all.

I don't think we want detachable panels. Really you're work on a second display is far more promising to me.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

So if you agree let's add proper profile on second display and I can debug and finalize the implementation on my two screen setup.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Do you mind if I ask why? If we're going to do it, better to have it from the start. El mié., 10 abr. 2019 a las 10:09, Pascal Obry (notifications@github.com) escribió:

But is not that easy, right now the system profile detects the profile of the display where the main window is, we should do the same with the system profile2 on this new window I won't do that (at least for the first implementation). I think a choice of profile to select will already be good. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2365 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/APq1pZkKKPeOrYa_G4pTKNzALmqqLnu-ks5vfeJ1gaJpZM4clj9V .

To me this is quite difficult to do as we need to get the screen for this second window. I haven't look at the code, so if you think it is easy let's do it...

@edgardoh edgardoh force-pushed the edgardoh:second_window branch from 6765062 to c83ff05 Apr 11, 2019

@edgardoh edgardoh force-pushed the edgardoh:second_window branch from fe754f0 to 55f8a70 Apr 11, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@TurboGit , still needs more testing but it seems to be working fine.

The GUI still need work, the image should be in the center of the window with the background of a configurable color (same as darkroom), but I don't know which is the right way to do it. If you want to take a look is done here:
https://github.com/darktable-org/darktable/pull/2365/files?file-filters%5B%5D=.c#diff-8d8b4015df7ee4b7600cc328eded44c9R3831

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@edgardoh : the profile seems to work fine between two screens. Moving the second window on different screen does pick the right color profile. So to me all is fine. But then I do not understand your last comment, what is not working for you? I'm ready to help, but I still need to see the issue :)

In any case thanks again for this nice piece of work! A must since a long time and you made it a reality for the community.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

One thing that need to be done, when clicking on the secondary window icon if opened it should be closed. But on my side it does nothing. At least it should be hided and the secondary pipe disabled.

I suppose we can configure the delay to trigger the pipe to refresh the secondary window?

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@edgardoh edgardoh force-pushed the edgardoh:second_window branch 2 times, most recently from 1b79500 to 2e5521a Apr 11, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I just added the turn on & off, but there's a glich when the window is displayed, I'll see if I can do better.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@edgardoh edgardoh force-pushed the edgardoh:second_window branch from f3a2108 to 5855097 Apr 11, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Just added the toggle button and it seems to work fine, let's see how it behaves on other systems.

Some known issues:

-When displaying the second window maximized, it first displays an un-maximized window for an instant. Same happens with the main window, no idea how to bypass that.

-The toggle button can get out of sync sometimes, I can't reproduce it yet. In that case the user will have to click twice to show/hide the window.

-I haven't tested it with opencl and I did not updated the scheduling profile, that probably will require a lot of testing on different configurations.

@PaoloAst

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

I apologize in advance for a possible silly question. How it is possible to test this feature? I don't see any option in the preferences nor on the main screen.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

This is a PR, (pull request). The code is not yet merged. To test this you need to build from source from current master branch and apply this patch present here. And note that you won't be able to downgrade to 2.6.x except if you do a backup.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

-The toggle button can get out of sync sometimes, I can't reproduce it yet. In that case the user will have to click twice to show/hide the window.

I have experienced this while testing, but was not able to reproduce. Apart from that, this seems to be working fine.

I'm wondering why the name is "system display2 profile", why not just "system display profile" in the drop down box? The label of the box is : "display2 profile" ? I would also certain rename "display2 profile" to "preview display profile". Or best move this on the new icon and just "display profile". The later is probably the best option to me.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Sure. The new window can also be renamed, how about darktable - darkroom preview ?

Looks good.

For me it make sense to have all the profiles together, but I can display the popup on that button too

Sounds a good compromise, yes!

@edgardoh edgardoh force-pushed the edgardoh:second_window branch from a8213d4 to 0c31c64 Apr 12, 2019

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

GUI changes should be ready.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Looks good to me! Seems to be working fine, so I'll merge to encourage more testing. Thanks.

@TurboGit TurboGit merged commit 2c0de69 into darktable-org:master Apr 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@AxelG-DE

This comment has been minimized.

Copy link

commented Apr 13, 2019

I love opensource. @edgardoh thank you so much!

As I cannot do coding, seems the minimum I should do is testing and reporting. As I see my self as one of the fathers of this idea 2323, even more I feel the responsibility to do so :-)

I am basically also perfectly fine with this approach. I guess I am also a good tester, as my two monitors are calibrated, yes, but have different technologies for background lighting and different resolution, hence the most critical, I assume....

Sorry if that question appears silly, where shall we have the discussions about experiences with this topic? I strongly assume, not here?

One thing I can say already:
understandably it costs lot of performance (double). My core i9 9900k with two GTX1060, I would have strong needs/wishes, to control the 2nd pixelpipe goes definitaly to the other GPU (I assume, today the prioirties in darktablerc do not recognize this yet)

Cheers
Axel

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@AxelG-DE , for a discussion here is fine, if you find any issue you should open an issue.

I haven't updated the scheduling but it should be reading it from darktablerc, if its not and you know how it should work let me know and we can work on that.

@AxelG-DE

This comment has been minimized.

Copy link

commented Apr 13, 2019

Currently I have the issue, that once I open the second window, during white balance I have a CPU-load of ~75% (all cores). I didn't try with -d perf yet, as I am in a hurry now...
my prio is unlike standard without this "!0", so each and anything shall go to GPUs

[update 1, next morning] 😄
Seems has nothing to do with this second preview window, rather in general opencl is not the same performant than before, hence I opend this one #2404

Some whises I would have anyhow. I would like to see the F11 (borderless) function on the 2nd preview as well and I wanna change the background colour to somewhat darker (I may find it in openrc, I guess) For this I will have to open a feature request....

[update 2]

done #2405 😊

@AxelG-DE

This comment has been minimized.

Copy link

commented Apr 16, 2019

Hi @edgardoh

again I am very grateful for what is possible with this wonderful open-source team and especially what you did for this PR.

Currently the second window technically seems to work to what it is intended by today (2nd preview). But you cannot really use it as working screen e.g. for white balance or check the masks

When I asked for detachable side panels, what was in my mind is the following:

  • my main monitor is on the left with 27", here I wanna have the main working window
  • and then detach the right panel (master-panel) and place it on my right hand side monitor 24" with its lower resolution and different back light technology hence not really the same impression even both calibrated
  • whether or not detach left panel and place it most far right, would be another try...
  • finally work on the 27" with the "master panel" on my 24" but on its left border next to the 27"

This workflow is not really possible at the moment. 2nd preview not really workable and have the full main window on the right hand side 24" is not that nice as the master-panel is too far away in that case.

In the dream case of detachable panels we'd have merely one pixelpipe which is way more performant. Now we have two pixepipes (I understand, is the best way at the moment) and the impact to the performance is huge, so I don't actually use the 2nd preview window even have a quite strong machine.

It somehow comes together with my other bug #2404

Again thanks for everything and let's see where all this brings us to...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.