-
Notifications
You must be signed in to change notification settings - Fork 55
Feat: add useStateMachineInputs hook #310
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?
Changes from 3 commits
ec8c4eb
fa9e7de
2a26db1
fc0c547
f25dc49
79ca848
0fc363e
9a7b3ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { EventType, StateMachineInput, Rive } from '@rive-app/canvas'; | ||
| import { useCallback, useEffect, useMemo, useState } from 'react'; | ||
|
|
||
| /** | ||
| * Custom hook for fetching multiple stateMachine inputs from a rive file. | ||
| * Particularly useful for fetching multiple inputs from a variable number of input names. | ||
| * | ||
| * @param rive - Rive instance | ||
| * @param stateMachineName - Name of the state machine | ||
| * @param inputNames - Name and initial value of the inputs | ||
| * @returns StateMachineInput[] | ||
| */ | ||
| export default function useStateMachineInputs( | ||
| rive: Rive | null, | ||
| stateMachineName?: string, | ||
| inputNames?: { | ||
| name: string; | ||
| initialValue?: number | boolean; | ||
| }[] | ||
| ) { | ||
| const [inputMap, setInputMap] = useState<Map<string, StateMachineInput>>(new Map()); | ||
|
|
||
| const syncInputs = useCallback(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. out of curiosity, in the previous implementation this method was defined in the useEffect hook directly. Was there a reason for moving it outside?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true that declaring functions that are only used in a useEffect within the hook is helpful for encapsulation, but I think it kind of hurts the readability of the code in many cases. If putting functions within useEffects is the convention Rive follows, I'm more than happy to move it back into the useEffect! Let me know what you think!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hey! Sorry about the delay. Yes it'd be good to have the method inside the useEffect itself.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 0fc363e Done!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! |
||
| if (!rive || !stateMachineName || !inputNames) return; | ||
|
|
||
| const riveInputs = rive.stateMachineInputs(stateMachineName); | ||
| if (!riveInputs) return; | ||
|
|
||
| // To optimize lookup time from O(n) to O(1) in the following loop | ||
| const riveInputLookup = new Map<string, StateMachineInput>(riveInputs.map(input => [input.name, input])); | ||
|
|
||
| setInputMap(() => { | ||
| const newMap = new Map<string, StateMachineInput>(); | ||
|
|
||
| // Iterate over inputNames instead of riveInputs to preserve array order | ||
| inputNames.forEach(inputName => { | ||
| const riveInput = riveInputLookup.get(inputName.name); | ||
| if (!riveInput) return; | ||
|
|
||
| if (inputName.initialValue !== undefined) { | ||
| riveInput.value = inputName.initialValue; | ||
| } | ||
|
|
||
| newMap.set(inputName.name, riveInput); | ||
| }); | ||
|
|
||
| return newMap; | ||
| }); | ||
| }, [inputNames, rive, stateMachineName]); | ||
|
|
||
| useEffect(() => { | ||
| syncInputs(); | ||
| if (rive) { | ||
| rive.on(EventType.Load, syncInputs); | ||
|
|
||
| return () => { | ||
| rive.off(EventType.Load, syncInputs); | ||
| }; | ||
| } | ||
| }, [rive, stateMachineName, inputNames, syncInputs]); | ||
|
|
||
| return useMemo(() => Array.from(inputMap.values()), [inputMap]); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
is there a reason why the map needs to be stored in the state instead of the array?
It's great that it is being used as an intermediate store while building the inputs for performance, but that seems like an implementation detail of
syncInputs.In the end it's exposed as an array and It forces to use the useMemo hook.
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.
Nope not at all anymore.
It was a
Mapfor the initial implementation I planned, which is returning theMapinstead of anArray. I thought it would be a better implementation as the user gets to choose which input they want to use, without having to map over the whole array again to find it.But I realized that could be handled by preprocessing the
inputNamesarray passed to the hook. ex) filtering out the unnecessary inputNames before calling the hook.So yeah, there's no reason for it to be a
Mapinstead of anArraynow.I'll change that! Thanks for pointing it out!