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

Newlocallab2 : Local adjustments - Improve GUI and fixed some bugs #5755

Merged
merged 1,582 commits into from
Jun 17, 2020
Merged

Conversation

Desmis
Copy link
Collaborator

@Desmis Desmis commented May 14, 2020

No description provided.

@heckflosse
Copy link
Collaborator

@Pandagrapher

You can merge into dev whenever you want.

Just a small detail: Jacques, who did the major work on this, should push the merge button, not me ;-)

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 16, 2020

I will push the button…tomorrow…at 12h00....if no objections :)

jacques

@heckflosse
Copy link
Collaborator

@Desmis Jacques, I just found a bottle of good Champagne in cellar to celebrate. No need to wait until 12:00 ;-)

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 16, 2020

@heckflosse

Good drink... with Petra :)

Thank you again :)

jacques

@Thanatomanic
Copy link
Contributor

@Desmis Best of luck pressing the magic button :-)

(And is there any objections against squash and merge, instead of a simple merge? Squashing makes the commit log much more manageable...)

@heckflosse
Copy link
Collaborator

heckflosse commented Jun 16, 2020

@Thanatomanic

Squashing makes the commit log much more manageable...)

I have no experience with squashing. How it does behave with git bisect?

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 16, 2020

@Thanatomanic @heckflosse
For me, it's chinese :)
I don't understand...and how to do ?

jacques

@Thanatomanic
Copy link
Contributor

@Desmis
image

@heckflosse Since all commits from a PR are squashed into a single commit, there is only one entry in the commit log. So bisecting means something happened either just before or just after introduction of Local adjustments.
That may make bisecting specific issues with Local adjustments harder, but bisecting issues anywhere else in RT easier. Maybe things can still be bisected inside the branch if that remains? I'm not sure about that...

@Thanatomanic
Copy link
Contributor

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 16, 2020

So if i understand correctly, just select "squash and merge"....and what happens after ?

is there a risk of error, where I don't know what others ?

At least for me, I've never heard of... and never I have seen (recently) use in Rawtherapee

In the label...i see "the 250+ commits"... here there are 1582...is it a problem ?

Jacques

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 16, 2020

I Think, in this case, I would prefer someone else to do it, because what do I do, if there is a bug;

I find it curious, but this is the remark of someone ignorant, to do this now

I give my power to @Thanatomanic

Jacques

@heckflosse
Copy link
Collaborator

@Thanatomanic

That may make bisecting specific issues with Local adjustments harder, but bisecting issues anywhere else in RT easier.

In this case I would prefer a simple merge, because issues anywhere else in RT are less likely

@Thanatomanic
Copy link
Contributor

@heckflosse All right, in that case the honour of the merge is certainly for Jacques!
Just be aware that the result is a huge number of small commits starting from Feb 1 all spread throughout the commit log. Maybe not too problematic :-)

@heckflosse
Copy link
Collaborator

@Thanatomanic Am I right, that we will have no newlocallab2 commit history when using squash?

In this case I even vote against using squash

@Thanatomanic
Copy link
Contributor

@heckflosse The commit history may still exist in the branch, I'm not sure (no git wizard here as well!). The commit history will only not be present in the dev branch: the newlocallab2 merge will show up as 1 single commit.

@Thanatomanic
Copy link
Contributor

@Desmis @heckflosse
A final pondering: why is rtdata/dcpprofiles/NIKON D50.dcp part of this PR?

And in rtengine/guidedfilter.cc and rtengine/guidedfilter.h there are some minor reverted changes in the wording of the GNU license (http instead of https), and Ingo's name is somehow removed.

Maybe still to little things to check?

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 16, 2020

Ok, good night...

I'm going to watch a movie with my wife, the holidays with my grandchildren are approaching, the weather is fine :)
and my english so bad...

I will see tomorrow what has been decided ... knowing - I repeat - that I suck in IT....
I do algorithms, but managing github is beyond me

My only training dates back to 1969... Fortran IV with IBM 1130, no monitor, punch cards

jacques

@heckflosse
Copy link
Collaborator

heckflosse commented Jun 16, 2020

@Thanatomanic

And in rtengine/guidedfilter.cc and rtengine/guidedfilter.h there are some minor reverted changes in the wording of the GNU license (http instead of https), and Ingo's name is somehow removed.
That means, we will have no history for this massive commit in dev. Not a good idea imho.

Hmm, you are right. Kind of strange. Thinking...

@heckflosse
Copy link
Collaborator

My only training dates back to 1969... Fortran IV with IBM 1130, no monitor, punch cards

I learned to read punch cards in 1973 ;-)

@heckflosse
Copy link
Collaborator

@Thanatomanic Roel, I agree that something is wrong. As you mentiomed https => http I can comfirm this one
bf1cae6#diff-9c79a1e961683992d04ec39c0ddcbdc0L18

......

@heckflosse
Copy link
Collaborator

@Desmis Jacques, please wait with merge until we have a solution for the mysteries correctly detected by @Thanatomanic

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 17, 2020

@heckflosse
Ingo

No problem to wait :)

jacques

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 17, 2020

@heckflosse @Thanatomanic

After an investigation worthy of Sherlock Holmes, the reason for the differences is simple.
To take advantage of "guidefilterlog" features, I copied / pasted the guidefilter files (*h *.cc) coming from ART.

In ART there is no citation "Optimized 2019 Ingo Weyrich [email protected]"
and GNU is write as : http://www.gnu.org/licenses/ whithout "s"

In copy and paste, I don't verified the "copyright and citation"... sorry :)

I am too respectful of copyrights and the word given, wherever I use someone's code (when it is explicit), I quote it systematically

jacques

@heckflosse
Copy link
Collaborator

@Desmis Push the button 👍

@Desmis Desmis merged commit 5705de6 into dev Jun 17, 2020
@Desmis
Copy link
Collaborator Author

Desmis commented Jun 17, 2020

It is 11h35...I push with 25mn in advance

jacques

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 17, 2020

Done :)

I have create a new topic in "pixls.us"

Thank you all...
For me it is a "great day"... waiting for more than 4 years

Jacques

@Floessie
Copy link
Collaborator

@Desmis Félicitations pour cette fonctionnalité formidable, attendue depuis longtemps et arrivée à maturité. 🎉

@Desmis
Copy link
Collaborator Author

Desmis commented Jun 17, 2020

@Floessie

Merci, c'est sympa

jacques

@Beep6581
Copy link
Owner

Hey, I'm late to the party - hooray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants