-
Notifications
You must be signed in to change notification settings - Fork 1
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
Gracefully scheduling the same runnable twice #1
Comments
Imo scheduled functions are resources with a lifetime (add and remove) and therefore for the The use case I saw in our meeting regarding ECS, where the Entity react-component schedules the system (if I remember correctly), would be, as I understand, the problem. Can you post the API you showed me that would cause an issue in this case? In general, scheduling the same function twice seems like an anti-pattern from my gut feeling, but the innovations I saw in your presentation might change my gut feeling :) |
The case we are talking about right now is a classic spawn pattern. You have a React component that encapsulates behavior and view for that feature. Here it is a body in my n-body simulation. It schedules an atomic runnable that syncs the simulation data with the Three scene created by R3F. (I know the implementation details are sparse here but let's not get bogged down in that.) It is added inside the function Body({ children}) {
// Schedule an atomic sync runnable.
useSchedule(syncBodiesWithThree, { after: 'update', before: 'render' })
return (
<mesh>
<circleGeometry args={[1, 32]} />
<meshBasicMaterial color="red" />
{children}
</mesh>
)
} And then is spawned up the tree as a list. // Just creates a list of 100 Body instances.
function Bodies() {
return spawn(Body, 100)
} Now the question is if this is correct. Coming from the perspective of "I just want to mount The alternative to to force this runnable to be mounted separate from the function Bodies() {
// Called only once. If Bodies is mounted twice, it needs to be moved up again.
useSchedule(syncBodiesWithThree, { after: 'update', before: 'render' })
return spawn(Body, 100)
}
function Body({ children}) {
return (
<mesh>
<circleGeometry args={[1, 32]} />
<meshBasicMaterial color="red" />
{children}
</mesh>
)
} This is technically more correct, but also means that there is a DX challenge where it is no longer enough to mount the React component for it to work. You have an additional function you need to import and schedule at a parent level to get it to work, just like a context provider. This isn't terrible, but I have been resistant. Think of it in terms of a There might be an alternative solution though that doesn't violate React principles where components like a |
Following up on this last idea, here is a pseudocode sketch of what that could look like. function FollowCamera() {
const ref = useRef(null)
// Has some global store, eg Zustand or ECS
const { target } = useGlobalStore()
// Get the schedule from the R3F state
const schedule = useThree().schedule
// Detect if global system is loaded
const hasGlobalystem = schedule.has(updateFolowCamera)
// If the global system is not loaded, run a local system instead
useFrame(!hasGlobalystem && () => {
// Run a local system to follow the target.
})
return <perspectiveCamera ref={ref} />
} @akdjr What do you think? As a pattern it would ensure that React components run pure local systems unless a global system is loaded at some root scope in the React tree. I think this would address both of our concerns. |
I've personally always preferred to have my systems live outside the "Entity React Component". It feels anti-ecs to couple global systems together with the view, that's why I've always done sth like the following and treated anything living inside an entity React components as purely local (i.e. a classical useFrame – with the option to be scheduled in between certain global systems): function Player() {
useFrame(() => {
// do something here
// gets mounted as many times as <Player> mount
}, {after: 'UpdatePositions', before: 'render'})
return ...
}
function Scene() {
return (
<>
{/*scene and world content*/}
<Player />
<Enemies />
{/*global systems – all in one place*/}
<UpdatePositions.System />
<UpdateAI.System/>
<CalcHitPoints.System />
</>
)
} But I can see how supporting an ecosystem has different demands, where people would want their package to be consumed as easily as possible. For this I think your last proposal works quite well, scheduling a local system given that a certain global one doesn't exist. Regarding throwing an error on duplicate mounting: Actually throwing an error if global systems get loaded up multiple times could help in this regard as it forces you to mount the global system on a higher level of the tree (just like Context) and keep the inner content of your entity React components limited to local systems. Neither from the pov of users nor library authors does this seem terribly complex. |
Imo the One case I can imagine with the FollowCamera, is if the ref to the FollowCamera was written to a global store. Now scheduling a global function that uses the global store to update the camera would work and using the FollowCamera component twice would be a problem. However, storing reference into a global store seems like a anti pattern and scheduling a local function that updates the local ref is much more robust because its more aligned with react. As a result I still have no strong opinion, I that the the developer could have the freedom to check if something is scheduled, but I'd also make sure not to advertise these patterns. |
Okay, general consensus so far is don't hide global subscriptions inside of React components. Let this be something the user explicitly does themselves so they have full visibility of global behavior. In a sense, the scheduler loop is a model and React is the view and we are stating that there are computations that are not appropriately described in the view itself.
This isn't an anti-pattern -- we have all sorts of uses for needing a ref to the view. A follow camera is a classic example since you need the target ref to even begin the algo. Traditionally you would create a context and pass the ref up and then read from it for the FollowCamera, or you would use Zustand to store it and read it directly. ECS cuts out of the middle-man by making the entire view queryable. In a typical document-like scenario you can usually colocate whatever needs references to each other in the same branch of the tree, but in a dynamic sim like a 3D world there is likely no parent-child relation. In our React game examples this kind of pattern is very common. Another benefit is that any behavior extracted as an atomic function can now be reused anywhere, not just inside of React. So I expect users making games or any app that needs to work with an authoritative server or workers to work in this direction. Having a pattern for how to deal with this incremental enhancement will be important, not even just for ECS. This is a problem right now with Zustand, Jotai, etc., where these are global stores and people bake some behavior into the compute functions, but if they need to run a loop instead of something event based they are at a loss. Here we offer a pattern for how to work with global stores per tick in a React-compliant way. |
@akdjr I ran into an annoying side effect of the erroring: HMR. Each time a file that schedules tries to HMR it errors. |
Currently if the same runnable is scheduled twice it throws an error.
In imperative apps this probably isn't an issue, you just check to make sure you are scheduling the runnable once, but in dynamic apps like React this can be a real issue. Here are two scenarios where I think it is legitimate to handle this case more gracefully than an error:
(2) is relevant since a React component is intended to have all of its behavior and state included with the view. The scheduler, especially with a hook, allows for including global level behavior, usually to do with a simulation.
My proposed solution is to have
add
return a boolean where true is successfully adding and false is unsuccessful, meaning it was already in the system. Then this can be used as feedback for determining what to do next, either ignore or remove/add to reschedule.The text was updated successfully, but these errors were encountered: