Slim down default RGB_ANIMATIONS

[Maxr1998]

Description

As it was already discussed in #7669, this PR moves some of the more specific animations into an extra define block, and slims down the default animations. To not break compatibility, I used the following simple script to find keymaps using those animations, and updated their defines to use the extended "import" where necessary. This is however in no way complete yet, and still needs some more testing on my end to verify it's complete, that's why I only marked it as draft for now.

#!/bin/bash
KEYMAP_DIR=$(realpath $1)
KEYMAP_NAME=$(basename $KEYMAP_DIR)
KEYBOARD_DIR=$(realpath $KEYMAP_DIR/../..)

cd $KEYMAP_DIR

if [[ $KEYMAP_NAME = "via" ]] || grep -qr \
    -e "RGB_M_X" -e "RGB_MODE_XMAS" \
    -e "RGB_M_T" -e "RGB_MODE_RGBTEST" *; then
    echo "Patching $KEYMAP_DIR"

    KEYMAP_CONFIG=$KEYMAP_DIR/config.h
    KEYBOARD_CONFIG=$KEYBOARD_DIR/config.h

    if [[ -f $KEYMAP_CONFIG ]] && grep -q RGBLIGHT_ANIMATIONS $KEYMAP_CONFIG; then
        sed -i 's/^\(\s*#\s*define\s*RGBLIGHT_ANIMATIONS\)\s*$/\1_ALL/g' $KEYMAP_CONFIG
    elif [[ -f $KEYBOARD_CONFIG ]] && grep -q RGBLIGHT_ANIMATIONS $KEYBOARD_CONFIG; then
        sed -i 's/^\(\s*#\s*define\s*RGBLIGHT_ANIMATIONS\)\s*$/\1_ALL/g' $KEYBOARD_CONFIG
    fi
fi

find . -type d -wholename "./keyboards/*/keymaps/*" -exec ./rgblight_fix_script.sh {} \;

Types of Changes

• Core
• Enhancement/optimization
• Documentation

Issues Fixed or Closed by This PR

• Fixes RGBLIGHT default effect list #7669

Checklist

• My change requires a change to the documentation.

[drashna]

Also, I'm not a fan of changing all of the keyboards, like this. Especially as the test animations aren't needed by most of these keyboards.

What may be better is just leaving the define as is.
Though .... we should ABSOLUTELY enable the christmas light one for this: https://github.com/qmk/qmk_firmware/tree/master/keyboards/christmas_tree

[Maxr1998]

@drashna wouldn't implicitly removing these effects from the keyboards/keymaps break the build? If not, I can of course revert the changes, if that's preferred.

[drashna]

Ah, yeah, you're right.

[Maxr1998]

@drashna just remembered this PR and applied your suggestions. Think it can be reviewed (and then merged) now.

[Maxr1998]

@drashna @fauxpark I just rebased and updated this commit to the latest master, could you have another look at it and possibly merge it then?

[zvecr]

So the PR touches user keymaps, which we either need sign off from the affected users, or it needs to run through https://docs.qmk.fm/#/breaking_changes?id=breaking-changes.

However outside those keymaps, it also silently breaks the behaviour of users who do something similar to:
keyboards/lets_split/keymaps/pitty/keymap.c: rgblight_mode(29);

Its in no way ideal that those keymaps use raw indexes, however I think that is also another case for this requiring breaking changes.

[Maxr1998]

Oh my, that's of course a valid concern. Are these modes only used inside rgblight_mode() or are there any other functions where there would be a potential hard-coding? If not, one could first fix all the hard-coded values by replacing them with the matching constants, and afterwards come back to this.

I agree that we should first get confirmation from the affected keyboard and keymap owners, but I think that'd get a bit complicated. Using the standard breaking-changes process is probably the better idea.

[drashna]

Unfortunately, the issue with the hard coded numbers is that at one point we didn't have the granular selection, and that stuff wasn't changed at that time.

For the keyboards, I don't think that we need the sign off on those. But for the user keymaps, we definitely want their signoff or to go via the breaking changes process

[Maxr1998]

I see, then it makes sense to wait until next breaking changes.

[drashna]

I see, then it makes sense to wait until next breaking changes.

You can tag the contributors in question, and if they all consent, then don't need to wait for breaking changes.

[Maxr1998]

Ok, thank you. Paging @issmirnov, @khord, @coryshaw1, @finex, and @AnthonyWharton then. Please let me know if you're fine with the changes in this PR, and maybe even check out the branch and give it a test, if everything still works.

Also, I rebased my changes onto the latest master, and also re-ran the script another time.

[AnthonyWharton]

I’m not in a position to test on my build, but it looks fine to me, thanks 👍

[issmirnov]

Thanks for the ping. I checked my keymap it works fine. TBH I should remove the unused test animations entirely, but I can handle that myself. Thanks for the reminder though!

Posting steps for other authors:

git clone https://github.com/qmk/qmk_firmware.git && cd qmk_firmware
git fetch origin +refs/pull/7680/merge && git checkout FETCH_HEAD
make git-submodule
make keebio/levinson/rev2:issmirnov

[khord]

LGTM

[Maxr1998]

@Wilba6582 thanks for the heads-up, how would you suggest to fix that problem? Does this have to happen inside the VIA source code? Or would the change be for all keyboards in the repo here, that are compatible with VIA?

[wilba]

@Maxr1998 If a keyboard currently in QMK has RGBLIGHT enabled and has a via keymap then update the base config.h to maintain the original default list of animations. IMHO that's consistent with your original intention of not breaking compatibility.

[Maxr1998]

@Wilba6582 Done. I also updated my helper script in the OP. Let me know if you spot any issues.

[Maxr1998]

So, any update on this? We'd still need feedback from @coryshaw1 and @finex, should we instead pull this one into the breaking changes process?

[finex]

So, any update on this? We'd still need feedback from @coryshaw1 and @finex, should we instead pull this one into the breaking changes process?

Ok for me.

[drashna]

@coryshaw1 any issues with the change to your code?

[coryshaw1]

Sorry for the late reply. I just built everything fine for my keymaps that use RGB. No issues here 👍

[Maxr1998]

Thanks, and no worries.

That should be everyone then! @drashna: so, final review I'd say?

[zvecr]

Personally i think this still needs breaking changes with what i mention in #7680 (review)

[noroadsleft]

Whether we go through master or future branch, this has merge conflicts which must be fixed.

[Maxr1998]

@noroadsleft thanks for the heads up, I'll rebase this once I find the time.

[Maxr1998]

@noroadsleft thanks for the heads up, I'll rebase this once I find the time.

Well, that didn't really work out.
Since there's now a docs section on how to selectively enable effects [1], this PR is likely not needed anymore and can be closed. Thoughts?

[drashna]

I think this is still useful, to be honest.

Having the option to have fewer animations by default is a good idea, IMO.

[tzarc]

No longer required.