rapier.js icon indicating copy to clipboard operation
rapier.js copied to clipboard

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

Open grischaerbe opened this issue 2 years ago • 5 comments

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.

grischaerbe avatar Jul 22 '22 21:07 grischaerbe

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

alexandernanberg avatar Jul 23 '22 10:07 alexandernanberg

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.

grischaerbe avatar Jul 23 '22 12:07 grischaerbe

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.

sebcrozet avatar Jul 23 '22 13:07 sebcrozet

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 🙂

alexandernanberg avatar Oct 11 '22 07:10 alexandernanberg

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.

trepidacious avatar Dec 17 '23 12:12 trepidacious