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

Update BulletSharp to latest version #289

Merged
merged 41 commits into from Jun 27, 2019
Merged

Conversation

@Eideren
Copy link
Collaborator

Eideren commented Dec 10, 2018

Content of this post where edited for future references, you can find previous records on the edit history located inside the header of this post.

Original Description:
Our version of BulletSharp is a modified, extremely old, pre-github version of https://github.com/AndresTraks/BulletSharpPInvoke , this PR should fix dependencies and the likes to allow us to pull from a clean and up to date nuget version of BulletSharpPInvoke instead.
Xenko's version contains a bunch of methods and fields which will have to be replaced with workarounds, some of which don't have any straightforward substitute.

Location of the sources, build scripts, tools and instructions used for this PR's libs located over here:
https://github.com/Eideren/BulletSharpPInvoke

@Kryptos-FR

This comment has been minimized.

Copy link
Collaborator

Kryptos-FR commented Dec 10, 2018

I think our version is based on https://github.com/sinkingsugar/BulletSharpPInvoke which was indeed a fork of AndresTraks code. We might also use the mobile version from https://github.com/sinkingsugar/bullet2-sharp-mobile.

Maybe kindly ping @sinkingsugar 😄

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator BulletSharp.Math.Matrix(Matrix value)
{
unsafe { return *(BulletSharp.Math.Matrix*)&value; }

This comment has been minimized.

@sinkingsugar

This comment has been minimized.

Copy link

sinkingsugar commented Dec 11, 2018

Hmm the API is slightly different, many things were done towards Xenko perf/optimization.
Can't remember all but like @xen2 said, Xenko matrices are different.
And SharpMotionState was a big one to optimize perf and collisions detection.
Lot's of changes were also in the C++ library itself!

This is just a wrapper, not sure you get any benefit by updating it unless the C++ part is also updated ( and has outstanding changes )

So unless the native library is also updated and properly tested on all platforms I would pay attention.

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Dec 11, 2018

Well, no idea how I ended up looking at xenko's matrix and miss the difference in their order.

Just to clarify, this PR wasn't made just to stay up to date with the wrapper, the main push behind this PR is the fact that the version used within xenko doesn't support multiple features of bullet, the main one being concave mesh collision.

From what I gather you guys don't have the sources for the C# and C++ side anymore, I can't do much about that without them so unless you do end up finding them I'll just continue on and we'll see where we end up.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 11, 2018

Sorry, didn't realize you were looking for the old source (esp. after @Kryptos-FR mentionned a repository which was almost the proper one, I thought you had it and were more focusing on how to rebuild from new sources!)

Here you go, it was there all along!
https://github.com/xenko3d/bullet2-sharp-mobile/tree/master/src
It contains both C++ and C#

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 11, 2018

Now I understand better why you did a compare with decompiled code (at first I thought you had the sources but wanted to see everything formatted the same way... should have understood that earlier!)
Sorry for the misunderstanding!

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 11, 2018

BTW, while I still have it on top of my head, some remarks about how those native libs are compiled so far:

We were usually building the sources for libbulletc and some other native libraries that needs target OS in manually in various computer/VM (hard for people to do it themselves from scratch since those VM had lot of specific software installed).

It would be great if we were switching to something more reproducible and easier to test.
For example, something that can either (1) run in CI, or (2) something that can setup a full docker instance with all dependencies (for Linux/Android, or similar system for Mac), or a combination of both.

That's probably something we should do for all those dependencies that can't be built using our C++ native layer directly in Xenko projects (which support only a very limited subset of the standard library) but instead need to be built on the target OS.

@tebjan

This comment has been minimized.

Copy link
Contributor

tebjan commented Dec 11, 2018

Xenko matrices are different.

that is indeed a bit problematic. since directx as well es opengl and i guess bullet have exactly the same memory layout. i wonder what was leading to that decision and which action was taken. my feeling is that there was a couple of wrong assumptions of how matrix transposition and coordinate system handedness works.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 11, 2018

@tebjan moved Matrix discussion and my comments in #291

@tebjan

This comment has been minimized.

Copy link
Contributor

tebjan commented Dec 11, 2018

thanks @xen2 , will need some time to get deeper into it. hope i will have time for this soon.

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Dec 11, 2018

Hey, thanks a bunch for the source @xen2 , 'BvhTriangleMeshShape' is already implemented on that repo, as well as a ton of other missing classes, could the dll included with xenko be out of date ?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 11, 2018

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Dec 14, 2018

So, I ended up choosing to work on optimizing the main repo of BulletSharpPInvoke instead of keeping our own up to date, I already have posted a PR over there, we'll see how it works out.

All contact data in xenko is in world space right ?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 17, 2018

@Eideren Took a quick look at our code and bullet code and yes, it looks like all ContactPoint data is in world coordinates.

@GutterLab

This comment has been minimized.

Copy link

GutterLab commented Jan 8, 2019

May want to consider using https://github.com/bepu/bepuphysics2 instead of bullet altogether.

@tebjan

This comment has been minimized.

Copy link
Contributor

tebjan commented Jan 21, 2019

@GutterLab bepuphysics doesn't look as mature as bullet, from their readme:

It notably does not yet include:

  • Convex hulls, cylinders, cones, or other complex shapes
  • Dedicated heightmap terrain meshes
  • Mesh-mesh collision detection
  • Bounciness, other than the frequency/damping ratio tuning
  • Continuous collision detection, other than the speculative margin
  • Many useful constraint types
@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jan 31, 2019

I'm still working on this but BulletSharpPInvoke's maintainer seems pretty busy right now, we might have to fork it instead, we'll see.

@Flavelius

This comment has been minimized.

Copy link

Flavelius commented Feb 1, 2019

To chime in on the different engine suggestions, PhysX could be a good consideration too, potentially providing some speedups though GPU support.

@Kryptos-FR

This comment has been minimized.

Copy link
Collaborator

Kryptos-FR commented Feb 1, 2019

potentially providing some speedups though GPU support

Isn't that only on NVidia graphic cards. If so, we'll have a kind of vendor-lock.

@Flavelius

This comment has been minimized.

Copy link

Flavelius commented Feb 1, 2019

Afaik, it is. But having that optional, according to
https://store.steampowered.com/hwsurvey/videocard/ or
https://www.statista.com/chart/15172/the-pc-graphics-card-market-share-among-gamers-on-steam/
it could atleast benefit a good share of users. Although not all, yes.

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 5, 2019

@xen2 I just tested out a legit game level made out of a single 1,800,000 triangle mesh.
Using it within the FPS sample did not seem to significantly affect load time; the entire game loads in less than 8 seconds and, yes, it does work correctly as I could not pass through the mesh.

I do not think that the load time is significant enough to warrant looking into backing the collision data even more so when we consider the issues that it would bring:
Increase our build output size (the vert+indices of the mesh itself is only about 40MB but the BVH data might significantly increase the required size, we also might not be able to re-use BVHs so multiply all of that by the amount of colliders using that mesh), increase the compile time for those builds and waste plenty of our users time as they would need to explicitly ask/be prompted for such an asset to be created.

Again, lets not make this more complex than it needs to.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 6, 2019

@Eideren Sure, in that case I agree with you.
I was not trying to add complexity but rather trying to avoid adding some complexity in the loading area (reading the data twice is not so trivial as the vertex data is deep in the object tree, and there's no common states when loading).

Anyway, it seems there's no avoiding it.

I will explain little bit the issues in details in another post, and then we could try to figure out a solution. Maybe there's a more clever way I couldn't think of yet.

Sorry it's taking some time to figure out, it's definitely not trivial as it sounds.
(also I have a lot going on at the same time -- almost settled in new place, moved in yesterday/today!)

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 17, 2019

Quick update: after 3 long weeks of delays due to customs, I should receive all my stuff tomorrow.
Crossing finger that I have that "get/setdata with no command list" branch lying somewhere.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 18, 2019

OK, got the computer today!
I could extract the branch, rebase it (git didn't play so nice since it was on old Xenko repo without common ancestor and before all the renaming) and fix a few compile errors.
It really had the GetData and SetData without CommandList, so at least that part I remembered well!
Here it is: https://github.com/xenko3d/xenko/tree/d3d_multithreading

I didn't investigate (yet) the details of the PR as well as if & how to merge it.
Please note that there is a few things that are probably broken still (i.e. I didn't see a grid in the Game Studio).

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 19, 2019

@xen2 You didn't get back to me on the issue with extracting vertex data from disk, I did prod around a bit to see if I could figure it out myself and from what I understood the data is uploaded to the GPU as soon as it is deserialized ? Without any proper way to hook ourselves into the system ?

I would love to help you out with the commandlist stuff but I don't have much experience with graphics api, so I won't be able to contribute significantly.

I think I should put the editor-side mesh collider code in a separate PR so that we can finally merge this one if you're ready to do so ?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 19, 2019

@Eideren Yes, the issue is that data is loaded to GPU memory right away.
Technically one could hook deserialization of buffer, but at the point it's called, there is not enough context info to know if the data needs to be set aside or not for something else.

Command list stuff would help in that you could get data from buffer without a command list, so you could just read back the GPU memory just like reading CPU memory.

Also, if everything is somewhat usable without editor-side mesh collider, we should definitely split this in another PR and merge the other part. Sorry that I didn't realize it was OK without!

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 21, 2019

@xen2 There you go, I'll create the mesh collider PR from the master branch as soon as this one is merged in.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 21, 2019

@Eideren thanks, code looks good to me!
I will run a quick test to see if everything works good in practice and then merge it.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 22, 2019

I noticed a bug when trying the ThirdPersonPlatformer.
It happens only in specific area of the map.
Probably due to the raycast.
Here's a video of it:
https://youtu.be/DOEGVDvNUkI

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 22, 2019

@xen2 I'll take a look at it

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 25, 2019

Solved, the issue laid within ShapeSweep, that's what the TPP template uses by default for its camera collision, works fine now.

@Eideren Eideren changed the title [WIP] Update BulletSharp to latest version Update BulletSharp to latest version Jun 25, 2019
@Eideren Eideren force-pushed the Eideren:bulletsharp_nuget branch from 8f6cb37 to ae1f4b4 Jun 26, 2019
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 26, 2019

Thanks.
I did some additional testing and everything looks good, LGTM for merge!

Maybe one last thing before doing so: could you add a short markdown file at deps\BulletPhysics\README.md that contains:

  • link to repo for BulletSharp.NetStandard.dll and libbulletc.dll (https://github.com/Eideren/BulletSharpPInvoke if I understood correctly)
  • which version is currently compiled (git commit or tag)
  • any extra info that might be useful to remember when trying to build it again (I suppose most of it is already in your BulletSharpPInvoke repo, in which case it's all good, but if you think of anything else...)

Maybe you could also either update or delete the batch files (esp. build.bat & checkout.bat -- sign.bat looks OK already) so that people are not confused trying to fetch different repo.

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 26, 2019

@xen2 Sure I meant to deal with those but kind of forgot, thanks for reminding me.
So, sign.bat is as up to date as I can do on my end, not sure how all of that works but the paths should all be mapped correctly ( paradox.snk was swapped to xenko.public.snk but I don't know if the functionality, if any, is retained ).

I think you should probably fork/copy my repo to xenko3d's account and then modify checkout.bat and the new README.md to point to that fork instead.

@xen2 xen2 merged commit 5cee86c into xenko3d:master Jun 27, 2019
2 checks passed
2 checks passed
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 27, 2019

Merged, thanks again and sorry it was delayed (esp. I didn't realize this could be split in two to take care of the more delicate part later)

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jun 27, 2019

@xen2 Nah, it's my fault for not mentioning it sooner :)

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jun 27, 2019

@Eideren no worries, the delicate part should have been taken care of sooner too!
Anyway, I have made a fork of BulletSharpPInvoke in xenko org: https://github.com/xenko3d/BulletSharpPInvoke
I will give you full permissions on it.

Eideren added a commit to Eideren/xenko that referenced this pull request Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.