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

fix linear2db returning null #61935

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

DarkeyPro
Copy link

@DarkeyPro DarkeyPro commented Jun 11, 2022

this is a tempt to fix linear2db returns null when p_linear=0.0.

@DarkeyPro DarkeyPro requested a review from a team as a code owner June 11, 2022 15:07
@DarkeyPro DarkeyPro changed the title Update math_funcs.h fix linear2db returning null Jun 11, 2022
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use a ternary operator instead of if for readability. We do not use single-line if statements in Godot's codebase.

@Calinou Calinou added bug topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jun 11, 2022
@Calinou Calinou added this to the 4.0 milestone Jun 11, 2022
@DarkeyPro
Copy link
Author

DarkeyPro commented Jun 11, 2022

I've updated it to use ternary operator.

@DarkeyPro DarkeyPro requested a review from Calinou June 11, 2022 15:39
Copy link
Contributor

@AaronRecord AaronRecord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the formatting

You'll also want to squash the commits when you're done

core/math/math_funcs.h Outdated Show resolved Hide resolved
core/math/math_funcs.h Outdated Show resolved Hide resolved
linear2db return -80.0 when p_linear = 0.0
@timothyqiu
Copy link
Member

IIRC, linear2db(0.0) returns -INF currently, and db2linear(-INF) is 0.0. If we change it to return -80.0, then it doesn't match after the inverse operation.

-INF is technically correct but very cumbersome to use as we have poor infinite value handling in APIs like AudioStreamPlayer volume and Slider value :(

@DarkeyPro
Copy link
Author

It should return -80.0 because its the lowest value that audio bus and AudioStreamPlayer volume should take .

@DarkeyPro
Copy link
Author

Also should i clamp the log min/max value to audio bus and AudioStreamPlayer volume.

@fire
Copy link
Member

fire commented Jun 15, 2022

I think that linear2db(value) and db2linear(value) where the value is 0 should give the same result.

@DarkeyPro
Copy link
Author

db2linear is working fine only linear2db returns null on p_linear=0.0.

@fire
Copy link
Member

fire commented Jun 23, 2022

We discussed in the the proposal meeting.

Juan liked the idea of reversible operations.

A design was suggested that changes -inf to 0 when using linear2db. (I think this is the current design?)

There's a cut off constant in the editor to stop processing. I don't remember if it was -40DB or something.

@DarkeyPro
Copy link
Author

yeah that would be a better way to fix it.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 13, 2023
@MewPurPur
Copy link
Contributor

Is there any resolution to this? It also needs a rebase as this is now linear_to_db()

@fire
Copy link
Member

fire commented Apr 12, 2023

Has anything changed since #61935 (comment)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release needs work topic:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants