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

Replace "Gamma" and "Tonemapping" with "transfer function" #178

Open
ilia3101 opened this issue Sep 9, 2019 · 14 comments
Open

Replace "Gamma" and "Tonemapping" with "transfer function" #178

ilia3101 opened this issue Sep 9, 2019 · 14 comments
Assignees

Comments

@ilia3101
Copy link
Owner

ilia3101 commented Sep 9, 2019

I am going to implement a method for passing a string math expression to create custom tonemapping functions, something I have wanted so long. It will be amazing for experimentation. Also quite easy to do, especially if I use some premade expression parser library.

This seems like a great option: https://github.com/jamesgregson/expression_parser

@masc4ii masc4ii changed the title New tonemapping feature for 1.10 New tonemapping feature for 1.1x Dec 10, 2019
@ilia3101 ilia3101 changed the title New tonemapping feature for 1.1x Replace "Gamma" and "Tonemapping" with "transfer function" May 12, 2020
@ilia3101
Copy link
Owner Author

ilia3101 commented May 13, 2020

@masc4ii Can you help me add https://github.com/codeplea/tinyexpr to MLV App's source code, in the processing folder (just the two files tinyexpr.c and tinyexpr.h), I am not sure how to make Qt compile them.

@masc4ii
Copy link
Collaborator

masc4ii commented May 14, 2020

Sure. f56f841 Here you are.

@ilia3101
Copy link
Owner Author

ilia3101 commented May 14, 2020

This will require a change to Qt interface and MASXML format. Gamma and tonemapping function will be replaced by "Transfer function" expression string, because "Transfer function" is a more correct term and gamma is a meaningless word. This way it will be much more customisable as well. Maybe we should also add a box for transfer function presets, as it's not easy to memorise them.

For updating old MASXML, I will write a function that converts a combination of "Gamma" and "Tonemapping function" in to a transfer function expression string.

Examples: "x" = linear, "pow(1/(x+1), 1/3.15)" = old reinhard preset

@masc4ii
Copy link
Collaborator

masc4ii commented May 14, 2020

Yes, such a function would be good. I think 99% of the people just use those presets. So it will be good to still have this combobox, as preset selection. No problem, if we could change the formula then.
Or is it maybe better if we could select between what we have now and this new function style? So we would just have to add a flag and the formula as string (to the MASXML).

@ilia3101
Copy link
Owner Author

I think 99% of the people just use those presets.

Yep, and those who want to, will be able to add their own functions or log curves.

Or is it maybe better if we could select between what we have now and this new function style?

Maybee, but it's going to be more difficult to implement, also more difficult on the interface side.

@masc4ii
Copy link
Collaborator

masc4ii commented May 14, 2020

Maybee, but it's going to be more difficult to implement, also more difficult on the interface side.

That is true. But maybe it is worth it...
Would it be possible to "automate the written function" by a gamma slider and the curve combo? I think yes. So we could setup the function like in the past (but now you see what the corresponding function is), or set to custom, which disables the gamma slider and enables the edit on the function.

@ilia3101
Copy link
Owner Author

ilia3101 commented May 14, 2020

Ah yes it could be done from the interface, using the function that converts gamma and tonemap function to a string.

I'll be honest. the thing that motivated me to do this was zeek using gamma and log 😂

@ilia3101
Copy link
Owner Author

ilia3101 commented May 14, 2020

6277c35

Now I need to add a conversion from "Gamma" and "Tonemaping function" to transfer function expression string (so we can have MASXML compatibility, and maybe have old mode in GUI if you want to add that).

@ilia3101 ilia3101 pinned this issue May 19, 2020
@masc4ii
Copy link
Collaborator

masc4ii commented May 25, 2020

87370c9
Now you can test it. I disabled visible flag for the other GUI elements, it seems the elements have no function right now.

@ilia3101
Copy link
Owner Author

Thank you

@ilia3101
Copy link
Owner Author

ilia3101 commented Jan 8, 2021

Happy new year everyone!

Sorry about the massive disappearance recently.

This needs to be finished :) I have left MLV App in a somewhat unusable state. But now I think it's time to fix it and release soon, as it's a new year and there is so much exciting Magic Lantern news. Will be committing plenty of improvements soon.

Or is it maybe better if we could select between what we have now and this new function style? So we would just have to add a flag and the formula as string (to the MASXML).

Yes! We should definitely do it like this. Are you ready to implement this? You can use the void processingSetGammaAndTonemapping(processingObject_t * processing, double gamma, int tonemapping); to support the old way. Currently there isn't a getter for those values, as they aren't stored in processingObject now, as it internally always uses the string. Would you like there to be one?

@masc4ii

@masc4ii
Copy link
Collaborator

masc4ii commented Jan 8, 2021

Happy new year to you too!

Nice to have you back!

I am ready, yes. I was already working with this function, but it makes life hard, because I need some logic to ask for tonemapping when changing gamma and the other way. And I think I had a problem with this function, because sometimes gamma is 1/x and sometimes it is x. So the try to use it failed.
Having the old way would be nice, and additionally this new feature.

@masc4ii
Copy link
Collaborator

masc4ii commented Jan 8, 2021

Do these two changes to the code and try yourself:
Bildschirmfoto 2021-01-08 um 17 06 41
Bildschirmfoto 2021-01-08 um 17 07 28

@ilia3101
Copy link
Owner Author

ilia3101 commented Jan 9, 2021

Trying that.

@ilia3101 ilia3101 unpinned this issue Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants