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

[Feature] Secondary RGB configuration (incomplete - need feedback) #7919

Open
wants to merge 13 commits into
base: master
from

Conversation

@robhaswell
Copy link
Contributor

robhaswell commented Jan 16, 2020

Description

When working on RGB effects I realised the current effects library would benefit from an additional dimension of configuration. For example:

  • Reactive effects could have a base colour.
  • Cycle effects could have the "gradient width" changed.
  • Almost all effects could benefit from separate configuration of underglow LEDs.

Note: This branch is currently a proof-of-concept of this idea, it is NOT FINISHED.

This branch introduces a new type rgb_alt_config_t with familiar hsv and speed values. These are manipulated using the same RGB_HUI keycodes, when combined with an ALT modifier. You can decrease with ALT+SHIFT as before.

The alt config has zero defined for all values by default, and in general operates by applying an offset to values in effects, thereby having no effect unless the users changes the values from zero. I have implemented the following effects:

  • SOLID_COLOR, ALPHA_MODS, GRADIENT_UP_DOWN & GRADIENT_LEFT_RIGHT - The alt config controls the underglow, so the underglow is completely independent from the keylight effect. The brightness can also be controlled individually.
  • CYCLE_LEFT_RIGHT - The alt speed configures the width of the gradient, so the user can have a really tight or really wide gradient moving either quickly or slowly. I find that low values for both is particularly pleasing and can't be replicated with the previous implementation. Additionally, the alt hue controls the offset for the underglow. A wide gradient + alt hue of 127 gives a nice RGB effect without looking "full-rainbow".

If the concept of this feature is suitable for inclusion I will do the work to implement the alt config in other effects where applicable, however aside from missing effects & documentation there are a few other issues:

  • EEPROM support - I have tried to implement this but can't test it and don't really know what I am doing.
  • Value overflow - as implemented every alt config value is a uint8 offset which overflows as it is modified, like the hue. This actually works well in practice, but at some point the value overflows or underflows which results in a jarring change of effect for the user. I think a fix for this might be for the values sat, val and speed to default to 127, prevent it from overflowing during changes, and then subtract 127 during effect calculation.
  • Max brightness - this is completely not respected in this POC but could be fixed for the final implementation.

At this point I'm going to wait for further feedback before progressing more with this branch, but I request that the maintainers try it out. Thanks.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).
@drashna

This comment has been minimized.

Copy link
Member

drashna commented Jan 17, 2020

I think this may be a duplicate of #5851, different implementation, but same end goal.

@robhaswell

This comment has been minimized.

Copy link
Contributor Author

robhaswell commented Jan 17, 2020

@drashna definitely similar goals, but different. #5851 seems to be designed to allow a different lighting effect for different LED types, and certainly my branch also lets you do the same thing for some effects - e.g. the solid, gradient and cycle lets you configure the underglow differently.

However the main goal of my branch is to provide a secondary configuration dimension, which some effects would let you configure the underglow for, but not all. For example I intend to allow the reactive effects (simple, cross, nexus) to have alt configs which set a solid or gradient background to be combined with the reactive effects.

With respect to #5851, my branch conflicts with it but the features are probably mergeable, it would need the alt config threading through all the effects. That does seem to be stale however.

Anyway, let me know where you want to go next. I could create the reactive alt configs so you can see how that work would in practice? Or if you don't feel like you want this feature I will stop working on it

@drashna drashna requested a review from qmk/collaborators Jan 18, 2020
@robhaswell robhaswell force-pushed the robhaswell:feature/rgb-alt-config branch from 534d0ba to 36c6d29 Jan 20, 2020
@drashna drashna requested a review from qmk/collaborators Jan 22, 2020
@drashna

This comment has been minimized.

Copy link
Member

drashna commented Jan 22, 2020

The only issu ew ith this, is there are at least 3 regions: alphas (keylight) modifiers, and underglow. And this doesn't appear to to not include those.

Additionally, in theory, eacd region would need it's own eeprom block.

@robhaswell

This comment has been minimized.

Copy link
Contributor Author

robhaswell commented Jan 29, 2020

@drashna I'm not intending to address all regions, in the same way that the current RGB matrix effects do not address all regions. I'm proposing that each effect have a 2nd axis of configuration - for some effects this would control the underglow, in other effects it would be something different (such as a background colour for nexus/reactive). The different effects could make artistic choices about what the secondary configuration controls.

I feel like you are confusing the intention of this with that of the other PR you mentioned, which definitely does want to offer precise control over each region.

@stale

This comment has been minimized.

Copy link

stale bot commented Mar 14, 2020

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@stale stale bot added the awaiting changes label Mar 14, 2020
@robhaswell

This comment has been minimized.

Copy link
Contributor Author

robhaswell commented Mar 16, 2020

Hi @drashna, just picking this up again.

I feel like last time we spoke you didn't grasp my description of how this is different to other branches, but that's not to say that I think it deserves to be in the main codebase. I have implemented this feature in my own keymap and effects for my personal use, so I have no special desire for it to be included. If you feel like this doesn't belong in the codebase I am happy to close this PR.

Let me know your thoughts.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.