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

Fixes issue #209 and #210 by adding additional progress effects #212

Merged
merged 8 commits into from
Jan 29, 2023

Conversation

CoryCharlton
Copy link

@CoryCharlton CoryCharlton commented Jan 27, 2023

Note that this fix does not include my changes for issue #209. I wasn't sure how you preferred PRs so they are completely separate and will need to be merged appropriately.

This change would also allow offsets to be added to the Both Ends progress effect if that was a future enhancement that was desired as there is no longer an assumption of LED 0 being a start point and all traversal is based on the actual min_pixel and max_pixel values passed to the progress function.

See discussion below for the implemented solution.

…the left/right `reverse_progress` parameter based on that
@cp2004
Copy link
Owner

cp2004 commented Jan 29, 2023

I'm debating how to go about this one - I think that an effect that grows out of the middle of the strip would be a good addition.

'Reverse progress direction' is a global setting designed for 'left-to-right' (or however you mounted the LEDs) flipping the strip for standard progress bar effects. An effect that is symmetrical doesn't have a difference when you flip it like this, and I can see a situation where you might simply flip a progress bar around but not the 'both ends' effect.

It would be relatively pain-free to add this as another progress effect option - could be called 'grow' or something (name up for debate...). Add it as an extra function here, extract & reuse the logic, and then list in constants.py should be all that's needed I think.

What do you think of adding an extra effect option instead? I can see that maybe 'Reverse progress direction' could be renamed to make it a bit clearer too.

@CoryCharlton
Copy link
Author

Good points.

In my use case my strip runs up the right vertical upright, crosses the top gantry, and ends running down the left upright. With the original effect the progress would end up top where it wasn't really visible and I was more concerned about "progress to complete" rather than "progress from start". Here's the default Heating Progress effect after my change:

20230127_180220_small

In my use case I do want all Both Ends effects to be reversed and complete at the end, closer to the bed.

I do agree that the current name of Reverse progress bar direction is not clear if it also impacts the Both Ends effect.

At the same time the existing name wasn't clear to me when the setting did not impact the Both Ends effect as I considered that to be a progress bar (although admittedly not the Progress Bar effect).

For this specific issue and PR I think you are correct that the best option is to move this logic out to a new effect. Maybe From Center or From Middle? I'll work on implementing that now.

That still leaves my original issue with the Reverse progress bar direction setting. The simplest option would be a minor UI update to change the text to Reverse **Progress Bar** effect direction. The option with the most flexibility would be to remove the global setting all together and add another Reversed Progress Bar effect.

I'll leave it up to you to decide if that is an issue that needs solving and how to do so. Also happy to implement that change.

@cp2004
Copy link
Owner

cp2004 commented Jan 29, 2023

Maybe From Center or From Middle?

I'd maybe go with 'From Center' there. Not sure why I just prefer it.

The option with the most flexibility would be to remove the global setting all together and add another Reversed Progress Bar effect.

I think this seems like the most clear/logical option at the moment to me. Way back when there was only one progress effect, it made sense, but now that has changed I think this option works well.

@CoryCharlton
Copy link
Author

All changes were implemented and there are now Reversed Progress Bar and From Center progress effects.

The Reverse progress bar direction setting was removed from the UI and from being passed into the effect in the EffectRunner but I didn't dig deeper to actually remove it from the defaults or anything else as I wanted to limit the scope of this PR and figured that leaving it, but unused, wouldn't actually hurt anything.

This PR now incorporates my fix for #209 as well so I will close PR #211

@CoryCharlton CoryCharlton changed the title Fixes issue #210 by adding the reverse parameter and setting the left/right reverse_progress parameter based on that Fixes issue #209 and #210 by adding additional progress effects Jan 29, 2023
cp2004
cp2004 previously approved these changes Jan 29, 2023
Copy link
Owner

@cp2004 cp2004 left a comment

Choose a reason for hiding this comment

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

I got bored, so I dug into this PR - it looks absolutely good to me. I'll add the settings migration & then merge this.

@cp2004 cp2004 changed the base branch from 0.8.x to devel January 29, 2023 22:34
@cp2004 cp2004 dismissed their stale review January 29, 2023 22:34

The base branch was changed.

@cp2004 cp2004 merged commit 75dc70e into cp2004:devel Jan 29, 2023
@CoryCharlton CoryCharlton deleted the issue_210 branch January 30, 2023 17:41
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