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

Utilize BodySet as a trait object instead of param #237

Merged
merged 3 commits into from Oct 26, 2019

Conversation

@distransient
Copy link
Contributor

distransient commented Oct 22, 2019

This commit prevents BodySet from polluting as a type parameter into all the core types required for physics stepping. Thanks to this, problems involved in dealing with these types in Specs usage are alleviated. However, this commit also removes the associated type of Body and always assumes dynamic dispatch on the Body type. This is an evolution of #226 that loosens future restrictions mentioned in that thread.

This commit prevents BodySet from polluting as a type parameter into all
the core types required for physics stepping. Thanks to this, problems
involved in dealing with these types in Specs usage are alleviated.
However, this commit also removes the associated type of Body and always
assumes dynamic dispatch on the Body type.
@distransient distransient mentioned this pull request Oct 22, 2019
2 of 5 tasks complete
@sebcrozet

This comment has been minimized.

Copy link
Member

sebcrozet commented Oct 22, 2019

Thank you for this PR! Since this is a significant change, I won't have time to review this in depth before November.

After a quick look, my main concern is the potential performance impact since this adds virtual function calls. We should add some benchmarks (in a separate PR that would be merged before this one) to check that.

@distransient

This comment has been minimized.

Copy link
Contributor Author

distransient commented Oct 22, 2019

👍 no problem. I do want to point out that assuming dyn Body for BodySet is not necessary for this PR, but heavily simplifies it, since all the places that would've previously been polluted with BodySet parameters would've then also been polluted with Body<N> parameters, and the trickiness of ?Sized.

sebcrozet added 2 commits Oct 26, 2019
@sebcrozet

This comment has been minimized.

Copy link
Member

sebcrozet commented Oct 26, 2019

I did not notice any performance regression with this change (by comparing the step times on our examples, before and after this change).

I took the liberty of pushing a couple of minor changes (fixing the DefaultMechanicalWorld alias and running cargo fmt).
Waiting for the CI to succeed in order to merge this.

@sebcrozet sebcrozet merged commit 18bcb84 into rustsim:master Oct 26, 2019
3 checks passed
3 checks passed
ci/circleci: build-native Your tests passed on CircleCI!
Details
ci/circleci: build-wasm Your tests passed on CircleCI!
Details
ci/circleci: check-fmt Your tests passed on CircleCI!
Details
@sebcrozet

This comment has been minimized.

Copy link
Member

sebcrozet commented Oct 26, 2019

All green. Thanks @distransient!

@eldyer

This comment has been minimized.

Copy link

eldyer commented Oct 26, 2019

@distransient Thanks a lot for this change, this works great and made my integration with specs much easier!

@distransient

This comment has been minimized.

Copy link
Contributor Author

distransient commented Oct 27, 2019

I've been caught up with school last two days, but glad my PR could help! ^^

@distransient distransient deleted the distransient:bodyset_trait_object branch Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.