cp2004 / OctoPrint-WS281x_LED_Status

Add some RGB LEDs to your printer for a quick status update!
https://plugins.octoprint.org/plugins/ws281x_led_status
GNU Affero General Public License v3.0
112 stars 26 forks source link

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

Closed CoryCharlton closed 1 year ago

CoryCharlton commented 1 year ago

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.

cp2004 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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