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

Replace three Timer with postprocessing Timer #218

Open
krispya opened this issue Dec 31, 2022 · 4 comments
Open

Replace three Timer with postprocessing Timer #218

krispya opened this issue Dec 31, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@krispya
Copy link
Member

krispya commented Dec 31, 2022

We recently added the three Timer class by Mugen. After this @vanruesc made us aware that he has a modified version inside of pmndrs/postprocessing that can be found here. I'm thinking we should use the same Timer class as pmndrs/postprocessing so that a shared timer is possible between pmndrs libraries.

Notable differences are using getters instead of get functions, reset properly resets the whole timer, autoReset manages the timer with page visibility without constructor side effects and the rAF timestamp can be passed into update (though this optimization was added to the three Timer recently).

The only downside I see is when the three Timer gets merged its API will likely be different from ours. However, I think if we do extend what looks like a better Timer design it can influence the final API and allow us to experiment with new ideas.

@krispya krispya added the enhancement New feature or request label Dec 31, 2022
@vanruesc
Copy link
Member

vanruesc commented Jan 1, 2023

FYI: I forgot to include the above-mentioned Timer changes in the latest pp release. Will update it first thing tomorrow.

@vanruesc
Copy link
Member

vanruesc commented Jan 1, 2023

The Timer class is now up-to-date in [email protected].

@krispya
Copy link
Member Author

krispya commented Jan 13, 2023

@vanruesc How do you feel about including your Timer class in three-stdlib and centrally maintaining it here? We can use the same class in R3F and possibly sync clocks with postprocessing.

@vanruesc
Copy link
Member

Feel free to include it in three-stdlib 👍

Adding three-stdlib as a dependency in pmndrs/postprocessing for a single class that will eventually be replaced seems overkill to me. I hope the Timer gets included in three soon, even if the API ends up being different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants