-
-
Notifications
You must be signed in to change notification settings - Fork 549
feat: expression blink function #3831
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
base: main
Are you sure you want to change the base?
Conversation
|
I see that this gives a lot of freedom. I wouldn't have thought of using an expression function but I can see potential benefits over the solution I suggested somewhere earlier (expanding the invert switch to a dropdown with several blink options). Also it might be good to return 0 and 1 instead of a boolean. This should easily cast to a boolean but makes it easier to use in other mathematical expressions ( I guess when the time parameter is 0 or some invalid type the output is constantly off? |
honestly I forgot about that discussion, and didnt really read that issue. I do see that this approach has the 'risk'/complexity of specifying the interval everywhere, which will make it riskier that users will make confusing non-aligned flashing.
I wouldnt be opposed to calling this
Makes sense to me, and I agree it will probably just work
Yes a non numeric input or <= 0 is fixed false.
Instead of conditionalise, I would use the |
|
A blink function of some description could be an interesting approach, and more easily allow other modules to do what I wrote for the vMix module with the Shift button and blinking feedback through 'shift layers' (you could have a row of buttons for preview bus on inputs 1 to 8, then hold a 'shift' action and they now do 9 to 16, and the feedback I wrote in the vMix module does solid feedback if the current input is active, but blinks if the input on the other 'shift layer' is active), all with this sort of style of doing blinking as opposed to specific stuff written in to modules. As for performance, a max blink frequency of 50ms is reasonable, slower systems such as a Pi may get a bit sluggish but it should still be responsive enough to adjust the blink to something slower rather than the system just locking up. In general I've found for blinking 333ms works quite nicely, as I used a timer to see how fast an ATEM panel blinks and went off of that as it seemed to look good :D Once you start getting blinking around 1000ms and slower, you start to run in to issues of the feedback staying on/off for such a long period that a user may glance down at a streamdeck, or when they're browsing through pages, and think the feedback is solid on, or solid off, rather thank blinking (which is one of the issues with the Generic Blink module IIRC, it doesn't do sub 1s blinking). |
|
First of all, @Julusian, this is brilliant!
|
|
I'm fine with I would assume that the time parameter is for the full period. For a square wave that is the on time plus the off time, for a (co-)sine it is both halfs and for a triangle/sawtooth/ramp it is one tooth. I would still prefer a time (= wavelength) over frequency. I see the appeal of a frequency in many situations, but we are using ms everywhere else and I would find it confusing to have in one button e.g. For the on/off fraction parameter, I strongly discourage using 1 for 1:1 or 2 for 1:2. The simple reason is that this is not linear. With such a parameter a square wave is turned into a PWM (pulse width modulation). With a fraction it is not possible to reach full off or full on as it would be 1:infinite and 1:0. I think the best solution is 0.0 -1.0 with a default of 0.5. My suggestion is to have an optional waveform parameter at second position and optional divison parameter at third position. We may have only Another possible addition would be a |
The issue I have with this is that it differs from existing blink functionality in modules, where the time value is the time between changes in state, rather than a full cycle and back to its original state. If we also want to consider the potential for more than 2 states. For example, I think it'll be easier to cycle through 0 to 7 every 500ms with something like
100% agree here, users are already more familiar with using time, such as with the existing generic blink module as well as delays/waits and the like. |
I don't think that cycling through discrete values should be done in a oscillate function. Oscillate for me is a good name for the classical waveforms: sine, square, triangle... usually a waveform that oscillates between two values. The function should be normalized to 0-1 like e.g. mathematical sin() and the two maxima should be applied externally, e.g. So for oscillate I stand with full wavelength in ms, waveform as string and fraction in 0-1. This seems to be the best combination of parameters to tweak the waveform, be independent and ensure that the oscillators can appear in sync when they are meant to be. The cycle thing I would probably call |
|
Talking about generating a sine wave I think is a different topic. I see it as being a completely separate implementation, as instead of being an occasional change like this, we would want to be re-evaluating the expression at some regular framerate. While that could be done with this implementation, I worry that the cost of doing that through global variables will be high. But if we do want to have sine wave generation in the future, then I feel like that would make more sense to be the default behaviour of an Then everything dorian is saying about that A thought which I am certain will be shot down and I am not at all attached to; |
|
Perhaps this topic should be kept specifically to 'blink' type functionality that, and the functions to achieve that, and both cycling between different values and generating wave forms can be entirely separate PR's.
Would that result in much benefit though? It seems like if a user is writing |
|
|
A footnote to my last comment regarding In addition it would be difficult-to-impossible to allow the user to choose between ms and Hz with Incidentally, if we did want to give the user the option of using Hz or ms, I would suggest requiring the unit to be specified explicitly, for example define: |
📝 WalkthroughWalkthroughAdds a VariablesBlinker class and threads it through expression parsing and evaluation. executeExpression now accepts a blinker and exposes a new blink(interval, dutyCycle) expression function returning 0/1 pulses. Tests, docs, and editor completions updated to cover the new API and behavior. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
ok, I've let this sit for a while, and I still like this approach. While I would like to look into the performance implications of this, that can come later. I have changed it to be I am going to keep this as blink, as I still think that oscillate will need to be done differently. If a button is using oscillate, there is no point piping those invalidations through the global variables system; instead we should have a cheap invalidation loop for these fast changing things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/user-guide/4_expressions/functions.md (1)
192-203: Consider moving theblinkfunction to a more appropriate section.The
blinkfunction is currently placed at the end of the "String operations" section (afterdecodeURIComponent), but it's not a string operation. Looking at the Monaco completions inExpression.monarch.ts, it's categorized under "Variable operations" alongsideparseVariablesandgetVariable.You might want to move this documentation block to be grouped with those functions (around line 204, before the
parseVariablesentry) for consistency with the UI categorization.The documentation content itself looks great though! Clear examples and the tip about boolean treatment is helpful. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
companion/lib/Variables/VariablesBlinker.ts (1)
74-91: Great approach to synchronized blinking!Aligning to unix epoch is a clever way to keep all intervals in sync across restarts and between different blink instances. The handling of edge cases for duty cycles of 0 or 1 (lines 81-91) addresses the previously flagged concern nicely.
🧹 Nitpick comments (1)
companion/lib/Variables/VariablesBlinker.ts (1)
22-33: Consider adding a dispose method for resource cleanup.The cleanup interval started in the constructor runs indefinitely. If you ever need to dispose of this class (e.g., during testing or graceful shutdown), having a way to stop it would be helpful.
This isn't blocking for the current use case if
VariablesBlinkeris a long-lived singleton, but it's worth considering for testability.💡 Optional improvement
export class VariablesBlinker { readonly `#logger` = LogController.createLogger('Variables/Blinker') readonly `#emitChange`: (values: VariableValueEntry[]) => void readonly `#intervals` = new Map<IntervalId, BlinkingInterval>() + readonly `#cleanupHandle`: NodeJS.Timeout constructor(emitChange: (values: VariableValueEntry[]) => void) { this.#emitChange = emitChange // Start a cleanup routine, to stop any unused intervals - setInterval(() => { + this.#cleanupHandle = setInterval(() => { // ... existing cleanup logic }, CLEANUP_INTERVAL) } + + dispose(): void { + clearInterval(this.#cleanupHandle) + for (const entry of this.#intervals.values()) { + if (entry.handle) clearTimeout(entry.handle) + entry.aborted = true + } + this.#intervals.clear() + }
|
Good compromise Julian. Thank you for persevering through the thicket of opinions! |

A 'simple' appearing expression function which can be used in feedbacks or elsewhere, which returns a boolean
All the blink usages join a shared bus to react to the blinking in sync (I have been warned by someone with much more experience than me that for good UX blinking should be in sync).
All timers are run from the same starting reference point in time, so that multiples will be in sync, and stopping and starting one will keep its alignment (at the cost of the first blink may not be perfectly even)
Which allows for easily producing fun patterns like:
Screencast.From.2025-12-10.23-16-55.mp4
This is piping these updates through the global variables system, so I am a bit worried about the performance impact. I think we might want to look into ways to reduce the sprawl of the variable change events before considering merging this, but I was inspired to look at this.
Summary by CodeRabbit
New Features
Documentation
Editor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.