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

[WIP] Ray Refactor #1108

Closed
wants to merge 18 commits into from
Closed

[WIP] Ray Refactor #1108

wants to merge 18 commits into from

Conversation

mikedh
Copy link
Owner

@mikedh mikedh commented Jan 4, 2021

Light refactor for trimesh.ray, also adds support for embree3.

  • rename the kwargs ray_origins/ray_directions to origins/directions
  • Switch the default embree binding to one supporting embree3
  • Add a parent ABC for RayMeshIntersector objects

@mikedh mikedh marked this pull request as draft January 4, 2021 02:27
@mikedh mikedh closed this Mar 14, 2021
@aluo-x
Copy link

aluo-x commented Mar 15, 2021

I've been meaning to submit a patch for trimesh on using embree3, there are a few subtleties (in using robust mode, multiple hits, resource release etc.). Will take a closer look once less busy (in around a week).

@mikedh
Copy link
Owner Author

mikedh commented Mar 15, 2021

That would be awesome! I think this branch has some nice stuff (parent class for intersector objects, kwarg rename with deprecation warning) but I didn't have time to get it over the finish line. If you want to branch off of this and take it further it would be great!

@sampotter
Copy link

Hi @mikedh and @aluo-x. Didn't realize you were considering using my Embree3 wrapper in trimesh. Pinging myself here so you can @ me in case you'd like any help or need info about my wrapper. (Please also feel free to create issues over on my repo.)

FYI, my development style for that repo has been to add what I need as I go. So just let me know if it's missing anything important...

@aluo-x
Copy link

aluo-x commented Mar 27, 2021

So I took a look at this pull request, and I do like the shared intersector class, should make a future new backend (CUDA) easier to adapt if ever necessary.

I'm curious your thoughts on potentially changing the return for multiple hits (possibly hidden behind separate_return=True argument, defaulting to false). Current API design returns all hits for a ray (in the multiple intersection case) as one giant array, but I think many usages may benefit from a return like this:
[[trig_ids for first hit ....], [trig_ids for second hit ....], ....]
[[ray_ids for first hit ....], etc, etc.]
[[xyz for first hit ....], etc, etc.]

Also on the topic of managing embree state (see this bug and this bug). I think it may be too much to ask a user to manually release this low level type manually, so the sane default would be to construct the device/scene/geometry and destroy them immediately after a ray_intersect call, but there is likely a performance penalty in cases where you can reuse the geometry (e.g. use the same geometry, but shoot different sets of rays in different calls). I propose an argument to auto_release=True, but add a custom release() method to the embree specific class.

Edit: to clarify, in each case we add flags which preserve the expected sane behavior, but allow them to be flipped to expose more information or more performance.

@aluo-x
Copy link

aluo-x commented Mar 27, 2021

We would likely need to distribute embree ourselves, there was a UV bug in robust mode that got fixed very recently in embree. Of course if we check for the intersection manually in numpy like trimesh does currently, this is not an issue.

Edit: possibly via anaconda? Not quite sure how the licensing works out. But I can develop assuming a valid installation of python-embree and embree3, then figure out the packaging stuff later.

@aluo-x
Copy link

aluo-x commented Mar 30, 2021

Any thoughts on this? @mikedh @sampotter

@mikedh
Copy link
Owner Author

mikedh commented Apr 1, 2021

Sorry for the delay, thanks @aluo-x and @sampotter for the help! Yes that sounds terrific!

We would likely need to distribute embree ourselves, there was a UV bug in robust mode that got fixed very recently in embree. Of course if we check for the intersection manually in numpy like trimesh does currently, this is not an issue.

It might be nice to build wheels eventually, I'm a big fan of cibuildwheel. In the meantime I've been installing embree3 using the ci bash script which seems ok-for-now. It looks like embree is Apache licensed, which I believe is fine to embed in wheels. One thing I really like about the wheel approach is it unit tests the Python binding using the same version of the library it's going to use on people's systems, whereas if it uses system-wide library it doesn't catch binding-library version issues.

I'm curious your thoughts on potentially changing the return for multiple hits (possibly hidden behind separate_return=True argument, defaulting to false). Current API design returns all hits for a ray (in the multiple intersection case) as one giant array, but I think many usages may benefit from a return like this:
[[trig_ids for first hit ....], [trig_ids for second hit ....], ....]
[[ray_ids for first hit ....], etc, etc.]
[[xyz for first hit ....], etc, etc.]

Definitely open to API adjustments! Would this make the hit indexes a "ragged sequence" rather than a uniform (n, d) array or am I misunderstanding? If I'm understanding correctly, that seems like it definitely easier to deal with under iteration, but precludes numpy operations for the most part?

Also on the topic of managing embree state (see this bug and this bug). I think it may be too much to ask a user to manually release this low level type manually, so the sane default would be to construct the device/scene/geometry and destroy them immediately after a ray_intersect call, but there is likely a performance penalty in cases where you can reuse the geometry (e.g. use the same geometry, but shoot different sets of rays in different calls). I propose an argument to auto_release=True, but add a custom release() method to the embree specific class.

Yeah makes total sense. Another option is to add __enter__/__exit__ to RayMeshParent class so you could do multiple queries using with, maybe something like:

with mesh.ray as ray:
    hit_1 = ray.intersects_id(...)
    hit_2 = ray.intersects_id(...)

One other thing that occurred to me in a ray refactor, should we expand the Intersectors to scenes? Or is that not worth the trouble/bandwidth right now.

@sampotter
Copy link

Would it be better to distribute a version of Embree with python-embree or with trimesh?

I have no problem with distributing a version of Embree so long as it doesn't make it difficult for the user to provide their own version.

@aluo-x
Copy link

aluo-x commented Apr 1, 2021

It looks like embree is Apache licensed, which I believe is fine to embed in wheels.

Very cool, I have no experience on the packaging/distribution side. Will leave that to someone else to deal with.

If I'm understanding correctly, that seems like it definitely easier to deal with under iteration, but precludes numpy operations for the most part?

Yes it would be "ragged". My current code base returns three lists. Each is a list of numpy arrays, either float or int. Float for hit locations, and int for ray/tri indicies. For the most part they will still work with numpy functions, but unless we hide it behind a flag it will be a breaking change.

Yeah makes total sense. Another option is to add __enter__/__exit__ to RayMeshParent class so you could do multiple queries

This could be an option, although it is not super obvious to me how we would want a user to be aware of this. I'm also not super sure it would necessarily map to all use cases, but I'm open to suggestions!

One other thing that occurred to me in a ray refactor, should we expand the Intersectors to scenes? Or is that not worth the trouble/bandwidth right now.

I guess an easy way would be to dump the scene object to a mesh, and that would be a straight forward way. But perhaps there is more to it?

Would it be better to distribute a version of Embree with python-embree or with trimesh?

Currently I believe most people install embree using anaconda. I forgot to check previously, but there appears to be an embree3 package. A cursory look seems to indicate that it is maintained by the author of the frensel path tracer.

Frensel actually has an python Optix wrapper (for CUDA), as well as a embree3 wrapper, I remember looking into it a couple of months ago. But my conclusion at the time was that it was too tied into the renderer itself, and it would be difficult for me to disentangle it.

Give me a couple of days, and I will work on the ray refactor.

@sampotter
Copy link

I came across fresnel when I was looking for an OptiX wrapper, recently, too. I came to the same conclusion (everything too tightly coupled).

I'd like to try out OptiX at some point in the future and could potentially be sold on the idea of writing a wrapper for it along the lines of python-embree. Won't have time for this for a while, but it seems like a fun project.

@aluo-x
Copy link

aluo-x commented May 3, 2021

I have a working copy of the new Trimesh based ray caster. Got side tracked due to academic stuff, I can push a version once the academic year wraps up in two weeks.

Also on the python Optix wrapper, there is this:
https://github.com/owl-project/NVISII

Looking at the examples - the API is super simple. It is more aimed towards rendering than "just" a ray casting engine though. It may be way overkill for our purposes.

The OWL project is maintained by Nvidia's "director of ray tracing", although as far as I can tell it is not an official Nvidia project.

@mikedh
Copy link
Owner Author

mikedh commented May 3, 2021

Awesome! No hurry. Oh nice yeah those Optix bindings look like they might be a little less heavyweight than some other GPU stuff.

@mikedh
Copy link
Owner Author

mikedh commented Oct 4, 2021

I totally "lost the dot" on this, need any help getting this over the line @aluo-x? Happy to help with intermediate results haha.

@aluo-x
Copy link

aluo-x commented Oct 4, 2021

Ahhh sorry, I intend to open source a differentiable embree3 version here (ICCV 2021):
https://github.com/aluo-x/NeuralRaycaster

@mikedh
Copy link
Owner Author

mikedh commented Oct 5, 2021

No hurry just curious! There's a few open ray issues that seem like they might be resolved by embree3, specifically the precision and "missed hit" issues.

@mmohrhard
Copy link

I have rebased and extended this PR and got the embree3 code working based on this PR and some of the suggestions from this PR. At least with our test geometries the results look much better. If anyone wants to have a look the whole updated commits are at mmohrhard#1

If there is interested I can also add my two commits to this PR.

@sampotter
Copy link

Is this using my library, python-embree?

I've had a few issues with it, and want to make sure I capture any fixes from you guys.

@mmohrhard
Copy link

Yes, this PR is using python-embree.

@mikedh mikedh mentioned this pull request Apr 7, 2022
@noamgat
Copy link
Contributor

noamgat commented May 10, 2022

This PR is super interesting! Where is it currently standing? (I see it as closed but don't understand exactly what it means)

@mikedh mikedh reopened this Jul 2, 2022
@mikedh mikedh changed the base branch from master to main July 2, 2022 19:39
@PaulDebus
Copy link
Contributor

This pull request seems like an interesting and important project. I have been recently working on speeding up my raycaster using the NVIDIA warp libraray, with outstanding results. Even in CPU (which should work without an NVIDIA GPU) mode, I got a speed up of around 4 orders of magnitude.

Maybe adding warp as an additional backend for the raycasting could be an extension of this project? If there is interest, I will offer to look into it.

@mikedh
Copy link
Owner Author

mikedh commented Jul 6, 2022

This pull request seems like an interesting and important project. I have been recently working on speeding up my raycaster using the NVIDIA warp libraray, with outstanding results. Even in CPU (which should work without an NVIDIA GPU) mode, I got a speed up of around 4 orders of magnitude.

Yeah adding another ray backend would be awesome! Especially one that appears to have nice packaging like warp :).

If anyone's watching this thread who has worked on a branch feel free to PR even partial stuff to this branch, I'm happy to do a merge-everything-then-clean-up workflow 😄.

@mikedh
Copy link
Owner Author

mikedh commented Jul 8, 2022

TODO:

  • fix embree3 binding
  • re-factor parent to be more scene-like, RayScene and then RayMesh just wraps the single-mesh case. This was originally written a while back as queries on single meshes but most backends support scenes.
  • benchmark embree2, embree3, and basic
  • see if it's possible to do a better broad-phase for basic-numpy ray checks to make it a little less pathetic. Last time I checked it's currently like 12K ray-per-sec for basic and 600K ray-per-sec for embree2.
  • re-re-name directions to vectors and update the kwarg deprecation decorator.

@PaulDebus
Copy link
Contributor

Hey,

I started implementing a warp-based backend (will open a WIP PR later) and had a couple of thoughts/ideas:

As I see it, the tests only check, if the code produces results of the correct shape/kind, but not if it produces the correct results. This would make implementing new backends easier, as implementation errors would be spotted. If there is interest in this, I can look into creating some tests but would appreciate your input.

My code currently only implements intersects_id and all other functions are variations of its results. These implementations could move into the base class, so only one method needs to be implemented and the rest is optional. As above, I can contribute here if there is interest.

Finally, I am unsure of a good way to select which backend is used - and how to pass additional parameters to the backend. I think using the fastest available method based on installed packages is a good default (after we figure out what is fastest) but we should expose the selection of backends. This would also be useful for implementing a test suite for all backends. Also, the warp backend could use an optional parameter (whether to use CUDA), which has to be passed in somehow. Currently, I do not see how this is implemented.

Thanks again for this important initiative!

@yil8
Copy link

yil8 commented May 8, 2023

@mikedh Hey, glad to find this PR and I'm curious what's the current status of adding embree3 support for trimesh? We at ambi robotics are using trimesh.ray.intersects_location and were also running into numerical issues of different results and missing intersection points at faces edges/corners.

@aluo-x
Copy link

aluo-x commented May 9, 2023

@yil8 You need to set scene.set_flags((1 << 2)), where scene is a embree3 python Scene object.

This activates RTC_SCENE_FLAG_ROBUST

Edit: see #242 (comment)

@mikedh mikedh closed this Mar 25, 2024
@tarheels100
Copy link

@mikedh I noticed this issue was closed recently. Has this been completed/merged? Is embree3 supported now? Thanks!

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.

8 participants