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

Added checks to init() in rapier3d-compat module to catch multiple calls #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grischaerbe
Copy link

As calling RAPIER.init() multiple times results in overlapping pointers, weird results and random errors but sometimes also stable situations, it's rather hard to debug and classify.
This PR makes multiple calls to RAPIER.init() result in a single Promise being resolved.

@alexandernanberg
Copy link
Contributor

IMO this doesn’t belong in the lib and is not worth the extra complexity, particularly since you easily can fix this on the consumer side

@grischaerbe
Copy link
Author

grischaerbe commented Jul 23, 2022

I agree it's rather easy to implement on the consumer side, however I still think it's a good addition:

  • It doesn't add complexity to the public facing API.
  • In some cases you can't have a shared application state.
  • It's a simple flag and doesn't add computational complexity.
  • The errors that result in calling init() multiple times are hard to classify. Sometimes it doesn't break rapier completely, it just behaves awkwardly.
  • It's something that is implemented in other well known libraries, such as three.js.

@sebcrozet
Copy link
Member

sebcrozet commented Jul 23, 2022

Thank you @grischaerbe for this change. I agree that it can easily be fixed on the user side, but the real issue here is the difficulty the user may have to understand that they are having crashes because of double-initialization. So this change can save them some debugging time.

@alexandernanberg
Copy link
Contributor

An alternative approach could be to throw an error (with a good description) if multiple calls to init occurs. The part I'm not a super big fan of is that a consumer can do the following without any issues (which could hide bugs in the consumer code).

RAPIER.init();
RAPIER.init();

But I don't have a super strong opinion on this and don't wanna block the PR 🙂

@trepidacious
Copy link

It seems like this could also be useful in case Rapier is used from multiple external libraries that might not be set up to work together - e.g. see the react-three-rapier issue I created. In this case, I'd like to use Rapier directly in my own code, and react-three-rapier uses it independently in a React context. As a result I can't see any way of making sure that init is called only once, without react-three-rapier being modified in some way so I can either tell whether it has initialised, or to make available the same functionality this PR allows. react-three-rapier does provide access to the initialised rapier, but this is only inside a React context which makes it awkward to use for a plain function.

As far as I can see, there are two main requirements:

  1. Make it possible to safely initialise rapier when it is not known whether it has already been initialised (e.g. my use case above).
  2. Allow users to easily detect code errors where init is called multiple times.

From the initial comment, neither requirement is currently met - as far as I can see 1. is not possible (I'd be very interested if there is a workaround), and 2. is not met because calling init() multiple times may either produce no errors, or ones that are hard to debug, so a new user seems unlikely to find out about their error this way.

Not to make it too complicated, but could there be two functions? The current init() would be left alone, except that it would throw an error as @alexandernanberg described. Any users with multiple inits would then become aware immediately (and from the sound of it might end up fixing some annoying intermittent bugs?), meeting requirement 2. Then add a new function that will permit multiple calls as in this PR. Since it's a new function, it would be "opt-in", and the user would presumably be aware of the trade-offs involved.

At a minimum, should the getting started docs have a clear warning never to call init() multiple times? I was doing this myself until I started to worry that it might be meant to be called once only, which led me to this issue.

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

Successfully merging this pull request may close these issues.

4 participants