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

ColormapOverlay overlays don't seem to apply to transparent keys #1425

Closed
obra opened this issue May 23, 2024 · 3 comments · Fixed by #1432
Closed

ColormapOverlay overlays don't seem to apply to transparent keys #1425

obra opened this issue May 23, 2024 · 3 comments · Fixed by #1432
Labels
bug Something isn't working

Comments

@obra
Copy link
Member

obra commented May 23, 2024

I was a little surprised, when testing out the replacement for the Numpad plugin, that setting an overlay on a key that didn't have anything defined on the numpad layer...just didn't light up the key. I'd expect that if I've explicitly defined an overlay, it wouldn't skip lighting up just because there was a transparent key on the keymap.

@EvyBongers - is this an intentional design? I'd love to understand the design intent better.

@obra obra added the bug Something isn't working label May 23, 2024
@EvyBongers
Copy link
Contributor

I don't believe that was intentional. This plugin is one of my first contributions, so I may have simply overlooked something. Will have a look this weekend

@EvyBongers
Copy link
Contributor

I don't do anything in particular to prevent the led from being set if the key isn't assigned. Basically, if an overlay is defined for the given key address on that layer, ::LEDControl.setCrgbAt is called to set the color for that particular key.

for (auto key_addr : KeyAddr::all()) {
if (ColormapOverlay::hasOverlay(key_addr)) {
::LEDControl.setCrgbAt(KeyAddr(key_addr), selectedColor);
} else {
::LEDControl.refreshAt(KeyAddr(key_addr));
}
}

I think the error I made is that I used Layer.lookupActiveLayer rather than Layer.mostRecent to find the current layer.
bool ColormapOverlay::hasOverlay(KeyAddr k) {
uint8_t layer_index = Layer.lookupActiveLayer(k);
for (uint8_t i{0}; i < overlay_count_; ++i) {
Overlay overlay = overlays_[i];
if (overlay.addr == k) {
if ((overlay.layer == layer_index) ||
(overlay.layer == layer_wildcard)) {
selectedColor = ::LEDPaletteTheme.lookupPaletteColor(overlay.palette_index);
return true;
}
}
}
return false;
}

Does that make sense to you?

@EvyBongers
Copy link
Contributor

Now I realize that this behavior may have been intentional, even it if is counterintuitive... My use-case for the overlay plugin is to highlight my layer switches. The way it's currently set up supports lighting up keys in lower layers, provided that higher layers don't have a key assigned (i.e. transparent). I suppose it should be easy enough to add additional logic to check for an assigned color overlay at the top-most layer before falling through to lower layers. I could also add a toggle to enable/disable the option to check for overlays on lower layers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants