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

Reconsider how useRenderLoop works. #633

Open
4 tasks done
alvarosabu opened this issue Apr 11, 2024 · 8 comments
Open
4 tasks done

Reconsider how useRenderLoop works. #633

alvarosabu opened this issue Apr 11, 2024 · 8 comments
Assignees
Labels

Comments

@alvarosabu
Copy link
Member

alvarosabu commented Apr 11, 2024

Description

useRenderLoop is used to provide the end user a callback method onLoop every frame per second to align with the one that the core uses for rendering.

It's based on useRafn composable from @vueuse/core https://vueuse.org/core/useRafFn/ but due to the feedback of the community, there are few issues related to its current behavior:

Suggested solution

Refactor useRenderLoop to move away from useRafn

Alternative

No response

Additional context

No response

Validations

@alvarosabu alvarosabu self-assigned this Apr 11, 2024
@dghez
Copy link

dghez commented Apr 11, 2024

as stated here #624 (comment) imho have the option to take control of the render-loop entirely and the option to specify the priority are a "must have", as you can do in useFrame in r3f.

If you do

useFrame(() => {}, 0)

you won't see anything because you are taking control over the render loop, very useful when you want to render manually into render-targets or have custom pipeline or render with layers :)

@andretchen0
Copy link
Contributor

Also Related

Moving forward

I wonder if we'd be best off simply leaving the current useRenderLoop implementation alone, for the most part.

useRenderLoop's good enough for a lot of circumstances – e.g., fire a function every ~1/n seconds. Even if we pull the renderer.render(...) out of that loop, it won't impact those usecases.

We could then more or less freely implement a new function that's tied to a particular TresCanvas' renderer, e.g., useFrame – to borrow R3F's name. Maybe that function also pauses/resumes with useRenderLoop to maintain most of the current functionality.

... Or maybe we just deprecate useRenderLoop and move on.

@alvarosabu
Copy link
Member Author

Hi, @andretchen0 I was doing the same mental exercise yesterday. Let's analyze the pros and cons of the several options

I managed to create a render loop per instance of TresCanvas. The problem is, opposite to the original useRenderLoop , the useLoop needs to be inside of a subcomponent of TresCanvas because it uses de ContextProvider... It's the only way to make it unique.

That would be a huge breaking change but, it's actually how R3F useFrame works. I just tested on an R3F app and you cannot use any of the hooks on the upper level as we do with useRendererLoop

image

Option 1 - Both composables coexist

useRenderLoop's good enough for a lot of circumstances – e.g., fire a function every ~1/n seconds. Even if we pull the renderer.render(...) out of that loop, it won't impact those use-cases.

With this approach, we avoid having a breaking change that would make the v4 not retro-compatible with v3

Pros

  • We avoid breaking all the demos and user apps that have a useRenderLoop on the same component that has a TresCanvas
  • Less maintenance burden. No real migration guide is required
  • Communication strategy would be easier.

Cons

  • useRenderLoop loses its complete meaning, is no longer the loop that is used for rendering, is just a useRafn, rename it would cause the same retro-compatibility issue on users.

Option 2 - Deprecate useRenderLoop in favor of useLoop

Pros

  • This would refine the conditional rendering and provide the highly requested solution to override the loop externally
  • We do take advantage of the major version to release this breaking change
  • We could use this to enforce the good practice of not using composables on the parent of the TresCanvas
  • In my opinion, this is how it should have been since the beginning.

Cons

  • While other v4 features will not really impact end users' apps, this one would break all the scenes that have useRenderLoop on the parent of the TresCanvas.
  • Delay of v4 release
  • Ecosystem (cientos, postprocessing) highly depends on the loop to work, this would trigger a major change on all.
  • All documentation and demos need to be updated, including my new course 🫠
  • Migration guide required
  • Maintenance effort increase for us since we would need to keep support for v3 for a little while until everyone migrates to v4

With that being said, after a night of sleep, even if the cons list is high, I'm personally inclined to Option 2 even if requires a little extra from the core team. The value of the benefits overcomes the cons.

  • Is a necessary change, useFrame equivalent should have been from the beginning.
  • We can continue exporting an empty version of the userRenderLoop with a deprecation error on console pointing to the migration guide to use useLoop instead to soften the migration out.
  • We communicate accordingly to the community
  • We open the door to true control of the rendering by users.

I'm pretty sure users will complain not being able to use the loop on the parent component, but we could even figure out some kind of context bridge like you propose on your PoC @andretchen0 https://github.com/orgs/Tresjs/discussions/578#discussioncomment-8697380

Sorry for the long text, wydt?

@andretchen0
Copy link
Contributor

andretchen0 commented Apr 12, 2024

wydt?

Ideally, I'd like to take some things off the list of cons.


I'd prefer to keep the current API intact, while tying it to a <TresCanvas /> and expanding it to include priority.

API-wise, useRenderLoop takes no arguments currently, so we could expand it like this:

  • useRenderLoop() – use the old API
  • useRenderLoop(fn, priority?: number) – use the new API

Behind the scenes, we'd reimplement the old API – onBeforeLoop, etc. – using the new priority-based mechanism. At first glance, this seems like a pretty straight-forward refactor – though maybe making the actual changes will turn up a problem I don't currently see.


I'm pretty sure users will complain not being able to use the loop on the parent component

Yeah, this is the real stumbling block for me, too.

It's just really helpful to useRenderLoop() in the <script setup> of a <TresCanvas />. It makes demos and reproductions shorter and more user-friendly – just one file!

but we could even figure out some kind of context bridge like you propose on your PoC

It looks to me like rather than throwing here ...

https://github.com/Tresjs/tres/blob/main/src/composables/useTresContextProvider/index.ts#L174

... we could create a new unbound context and bind it when the next TresCanvas is created.

That'll work for common use cases, I think. We can offer an explicit API for more control for users who want it.


I've been doing some smaller nice-to-haves that I'd been putting off. Once I'm done with that, I'll move back to the core to try to move this forward.

In the meantime, I'd prefer not to hold back v4.

@alvarosabu
Copy link
Member Author

Hi @andretchen0 thanks for your insightful feedback. In the following days I will push the PoC I got working taking in consideration both your feedback and @JaimeTorrealba.

Now that this pandora box was opened. Should we bind all composables to each instance or just the loop?

  • useTexture
  • useLoader

I personally only see benefits on the render loop one, but the rest can be global inmho. It's true that R3F has a warning on the docs that Hooks can only be use inside of the Canvas context
Screenshot 2024-04-13 at 15 12 08

@andretchen0
Copy link
Contributor

@alvarosabu

In the following days I will push the PoC I got working taking in consideration both your feedback and @JaimeTorrealba.

Ok. Just for reference, I think this discussion has most of the relevant issues related to useRenderLoop and binding context. Maybe that'll be helpful.

I personally only see benefits on the render loop one, but the rest can be global imho.

Off hand, I'm not sure.

I do know that if we're going to have plugins, we need extend/catalogue (or similar) to be bound to a <TresCanvas />.

@Tinoooo
Copy link
Contributor

Tinoooo commented May 3, 2024

@alvarosabu @JaimeTorrealba and I had a chat about the while topic. We propose the following structure for useLoop to allow users to register callbacks around a loop's lifecycle.

Types/Interfaces:

interface Options {
  delta: number
  elapsed: number
  clock: Clock
  context: TresContext
}

type EventHookOn = (
  fn: (opts: Options) => void,
  priority?: number
) => {
  off: () => void
}

interface UseLoop {
  onBeforeRender: EventHookOn
  render: (opts: Options) => void
  onAfterRender: EventHookOn
  ...
}

Pros:

  • It feels very Vue-ish. The users might be used to this structure since it is very similar to Vue's lifecycle hooks and vue-router's navigation guards
  • The numeric parameter priority can be set. In most cases this will not be used. But there are use cases where it is handy. It allows us to register callbacks that happen right before rendering. One example where this comes in handy is useFBO. We can make sure that things happen right before rendering by calling onBeforeRender(() => {...}, Number.POSITIVE_INFINITY).
  • The render call can be overwritten via render. To make this more obvious, we decided to omit the prefix "on".

@alvarosabu
Copy link
Member Author

To complement what Tino explained in the previous comment:

With this approach, we avoid using magic numbers to denote the different stages of the loop, indexes are now just for priority of execution for before and after render callbacks.
Diagram onLoop

@alvarosabu alvarosabu mentioned this issue May 4, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants