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

Modified ColorFillEffect to optionally ignore global color #539

Closed

Conversation

revision29
Copy link
Contributor

Description

Added a "new" strip effect for filling all LEDs with the global color. Mostly, all I did was copy the code for the ColorFillEffect and modify it to read the global color. I was going to modify the existing solid color fill effect to pull the global color if applyGlobalColors was true. But, someone might want to have a color fill that is not impacted by the global color.

Contributing requirements

  • I read the contribution guidelines in CONTRIBUTING.md.
  • I understand the BlinkenPerBit metric, and maximized it in this PR.
  • I selected main as the target branch.
  • All code herein is subjected to the license terms in COPYING.txt.

@rbergen
Copy link
Collaborator

rbergen commented Nov 23, 2023

To be honest, also considering #485 and your own question in #538, I don't think this is the way to go.
I'd say we should:

  • change the existing GlobalFillColor to adopt the global color if DeviceConfig::ApplyGlobalColor() returns true,
  • stop forcing the current effect to it for the "no USE_HUB75, no ENABLE_AUDIO" scenario and
  • get going with making other effects adopt/incorporate whatever the global colors are.

@revision29
Copy link
Contributor Author

I in part agree with your 3 points. The problem is dynamically toggling applyGlobalColors from a remote. I don't think the way the current remote code handles that is intuitive or the right way to go.

But, I can imagine a person might have multiple color fill effects set in the effects manager that they might want to rotate through instead of triggering specific colors from the remote. They might also want a color fill they want to dynamically change from the remote. The way the current applyGlobalColors works is that all of those colors in the queue would be overridden by the global color. Trying to toggle applyGlobalColors to work around this scenario is unworkable from an IR remote.

I have another solution in mind. That is add another variable to the effect called ignoreGlobalColor with a default value of false. So, I could create a series of color fill effects with ignoreGlobalColor set to true and it would ignore the global color even if applyGlobalColors is true. I think this approach might actually be good for other effects as well.

@rbergen
Copy link
Collaborator

rbergen commented Nov 23, 2023

The problem is dynamically toggling applyGlobalColors from a remote. I don't think the way the current remote code handles that is intuitive or the right way to go.

In that case that's what should be fixed. Working around it by introducing almost indentical duplicate effects is wagging the dog, IMHO.

That is add another variable to the effect called ignoreGlobalColor with a default value of false.

That could work. There's just too much duplication of code with this particular approach.

@revision29
Copy link
Contributor Author

Okay, I am about to push another commit. I am trying to clean up what gets pushed but GitHub desktop is being annoying. It's not ignoring .gitignore or some items I have in there. I might have to force ignore some files from the web interface after I push the commit. Just wanted to issue the warning in case the commit looks messy. It should only impact 2 files.

Removed the global color fill effect that I originally added. Changed the ColorFillEffect to add a flag to ignore the global color.

Updated the comment label for the effect.
@revision29 revision29 changed the title New effect GlobalColorFillEffect Modified ColorFillEffect to optionally ignore global color Nov 23, 2023
@revision29
Copy link
Contributor Author

Going to kill because git is being annoying.

@revision29 revision29 closed this Nov 23, 2023
@rbergen
Copy link
Collaborator

rbergen commented Nov 23, 2023

I may be barking up the wrong tree, but adding a file to .gitignore after it's already been committed doesn't work. You then also have to run git rm --cached <filename> to make git stop tracking the file in question, or run git rm -r --cached . if you want to "untrack" all files in .gitignore.

@revision29
Copy link
Contributor Author

@rbergen thanks! Will keep that on hand for later. I got flustered and made things progressively worse. The PR had to get pulled anyway.

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.

2 participants