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

Switch to a quadratic curve for note brightness based on note velocity #3260

Draft
wants to merge 10 commits into
base: community
Choose a base branch
from

Conversation

Metamere
Copy link
Contributor

@Metamere Metamere commented Jan 12, 2025

Switched to a quadratic curve to make the brightness gradient based on note velocity much more apparent. Float calculations made this much simpler, and means that the adjustFractional() function is not needed anymore. This is similar to what was done for PR #3132 for the kit drum velocity keyboard view updates. See comparison photos below, which were taken with the same exposure settings.

Comparing before/after changes for green, blue, and red.
G-linear
G-Quadratic
B-linear
B-Quadratic
R-linear
R-Quadratic

And here it is at the lowest system brightness setting possible, with a drastically longer exposure time needed to capture the images.
R-low-linear
R-low-quadratic
It is still well visible at the lowest brightness settings in a dark room. Not that those super low velocities and settings are commonly used, but it still makes for a good baseline check.

Switched to a quadratic curve to make the brightness gradient based on note velocity much more apparent. Float calculations made this much simpler, and means that the adjustFractional() function is not needed anymore.
Copy link
Contributor

github-actions bot commented Jan 12, 2025

Test Results

107 tests  +1   107 ✅ +1   0s ⏱️ -1s
 16 suites ±0     0 💤 ±0 
 16 files   ±0     0 ❌ ±0 

Results for commit e3b0492. ± Comparison against base commit d6818f9.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@stellar-aria stellar-aria left a comment

Choose a reason for hiding this comment

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

LGTM pending the small cast nit

src/deluge/model/note/note_row.cpp Outdated Show resolved Hide resolved
// The brightness is reduced as necessary to be proportional to the note velocity.
// below .03 (~4/127) it is not visible enough, so we set that as the minimum
// A quadratic curve makes the brightness gradient appear more linear.
pixel = rowColour.transform([colour_intensity](uint8_t chan) { return (chan * colour_intensity); });
Copy link
Collaborator

@stellar-aria stellar-aria Jan 12, 2025

Choose a reason for hiding this comment

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

If you want to, instead of using the small transform here, you can add an

[[nodiscard]] constexpr RGB operator*(float value) const { 
    return RGB(r * value, g * value, b * value); 
}

to the RGB class which simply returns a new RGB with each of the channels multiplied by the input value.
It would turn this line into

pixel = rowColour * colour_intensity;

which I think is much easier to understand

@seangoodvibes
Copy link
Collaborator

Do we know how this looks with blurred notes and tails?

I think in the previous implementation the note head was still distinguishable from the note tail.

Also would be curious what happens with blurred notes.

@Metamere
Copy link
Contributor Author

Metamere commented Jan 12, 2025

Do we know how this looks with blurred notes and tails?
I think in the previous implementation the note head was still distinguishable from the note tail.

Fine with blurred notes. Not great with tails, as the dimmer notes become difficult to distinguish from the tails. After Sean and I experimented with different methods of addressing that issue, it seems it will need some more consideration as to how to address this issue. It will probably be possible, but if no satisfactory solution can be found, we may need to discuss whether we want to incorporate this change or even whether we should eliminate this feature.

@seangoodvibes
Copy link
Collaborator

Do we know how this looks with blurred notes and tails?

I think in the previous implementation the note head was still distinguishable from the note tail.

Fine with blurred notes. Not great with tails, as the dimmer notes become difficult to distinguish from the tails. After Sean and I experimented with different methods of addressing that issue, it seems it will need some more consideration as to how to address this issue. It will probably be possible, but if no satisfactory solution can be found, we may need to discuss whether we want to incorporate this change or even whether we should eliminate this feature.

Hey so after speaking with a couple folks on the discord, let's try out this change with just the quadratic curve and then go from there. We'll see what people think.

Metamere and others added 3 commits January 12, 2025 13:31
Co-authored-by: Katherine Whitlock <[email protected]>
simplifies things when you just want to multiply by a single float value.
@seangoodvibes
Copy link
Collaborator

Switching PR to draft just to prevent accidentally merging this until @Metamere gives the go ahead

@seangoodvibes seangoodvibes marked this pull request as draft January 12, 2025 20:12
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.

3 participants