search

dimforge / ncollide

2 and 3-dimensional collision detection library in Rust.
https://ncollide.org
Apache License 2.0
921 stars 105 forks source link

Serialization support for ncollide #297

Open syberant opened 5 years ago

syberant commented 5 years ago

This issue is related to rustsim/nphysics#203. It is about implementing (de)serialization capabilities to ncollide so they can be used when adding those capabilities to nphysics.

As said in the linked issue I'm willing to make a PR for this myself and I will use this issue to discuss the questions specific to ncollide.

The first thing I did in my temporary fork was adding #![deny(bare_trait_objects)] to src/lib.rs and using cargo fix to put dyn in front of every trait object, thereby making them easily recognisable. This was ~250 changes and originally meant only for development purposes but I think it is sufficiently handy and non-intrusive that it may be a good idea to keep it. If this is done it should most likely be in a different PR to keep changes manageable.

Then I started derive-ing the crap out of everything and doing a few manual implementations which only contained unimplemented! when I ran into problems. I'm still looking at the problem of trait objects but typetag, as suggested by @Ralith, seems to fix it without security concerns by basically making an enum out of things. I also looked at serde_traitobject but that serialized the vtable pointer and could allow arbitrary code execution (I think, about 99% sure and the crate also warns of its insecurity) and is specific to a binary. Question: Is a dependency like typetag acceptable? Otherwise I would end up probably implementing about the same thing but less mature.

I have also looked at the serialization support of some dependencies:

I'm currently in the middle of my test week and will resume work on this next weekend, sorry for being a bit slow right now.

Ralith commented 5 years ago

put dyn in front of every trait object, thereby making them easily recognisable. This was ~250 changes and originally meant only for development purposes but I think it is sufficiently handy and non-intrusive that it may be a good idea to keep it.

Explicit dyn is the preferred style for those very reasons, so I strongly encourage you to submit that PR.

sebcrozet commented 5 years ago

Thank you for taking this @syberant!

The first thing I did in my temporary fork was adding #![deny(bare_trait_objects)] to src/lib.rs and using cargo fix to put dyn in front of every trait object, thereby making them easily recognisable. [...] If this is done it should most likely be in a different PR to keep changes manageable.

I agree adding dyn is desirable and should be in a different PR. However, I'm doing some restructuring in ncollide and merging a PR like this will lead to a lot of merge conflict I'm not willing to face right now. So I suggest we postpone the execution of cargo fix for a month or so unless it is absolutely necessary for your current work on serialization.

Question: Is a dependency like typetag acceptable? Otherwise I would end up probably implementing about the same thing but less mature.

It is acceptable as long as it is compatible with WASM.

  • smallvec supports serialization but I'm not even sure if it's in use anymore (couldn't find it at least).

We used it at some point in the past. If it isn't used any more, feel free to remove the dependency.

I'm currently in the middle of my test week and will resume work on this next weekend, sorry for being a bit slow right now.

Don't worry about this, the is no rush!

sebcrozet commented 5 years ago

It looks like typetag relies on the ctor crate which I doubt is compatible with WASM. This means the typetag dependency (and thus everything relying on trait-object serialization) must be put behind a feature.

Ralith commented 5 years ago

The entirety of serialization support should be an optional feature, since serde is a large dep that is not universally required.

sebcrozet commented 5 years ago

@Ralith Yes, this is true. What I meant to say is that we should have two features. One feature, say, serde-serialize that enables serialization for everything but trait-objects (and that will work on WASM too). And to get serialization of trait-objects too, the user has to enable another feature, say, trait-object-serialize that will enable the typetag dependency (and thus breaks WASM compatibility).

syberant commented 5 years ago

Thanks for catching the issue so quickly, I hadn't thought about WASM-compatibility of serialization and was using the already existing feature serde-serialize but will now make another feature trait-object-serialize for the trait-object specific parts.

sebcrozet commented 5 years ago

Relevant issues regarding compatibility with WASM: