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 to fcl 0.6.1 and ci build for linux/win/mac using github actions #29

Merged
merged 20 commits into from
Sep 9, 2021

Conversation

CyrilWaechter
Copy link

This pull request kind of combine #21 and #18:

@mikedh
Copy link
Collaborator

mikedh commented Aug 3, 2021

This is awesome! I checked with @mmatl a while back and he was cool with me merging one of the PR's updating to the latest version of FCL and the cross-platform stuff is also awesome. This looks like it did the work to combine those PR's and check via multiplatform builds. I'm happy to review/test/merge once builds/tests/conflicts are resolved, unless someone has strong objections.

@mmatl
Copy link
Collaborator

mmatl commented Aug 4, 2021

Thanks for the big PR! @mikedh @CyrilWaechter just let me know when this is fully ready and I can stage a deploy

DmitryNeverov and others added 17 commits August 4, 2021 11:12
Updated header file locations
Disabled functions not present in Eigen objects
Disabled distance function
Set Eigen scalar template parameter to `double` everywhere
Vector3, Matrix3, Quaternion and Transform
are now native Eigen objects in FCL,
so helper functions were changed accordingly.
distance function and its dependants.
Now the old test/* is passing.
Use fcl version as version to make targeted version more obvious.
* Fix conflicting folder name between python package and fcl dependency
* Move to `src/module` tree schema
* Fix `setup.cfg` options
* Merge clone and build bash scripts for clarity (future mac/win builds)
* Blender use a specific version of numpy.
* ci failed to compiled fcl while it was correctyl compiled locally with
the same build script. Use precompiled binaries until we found out why.
* upload wheels to release
cibuildwheel already install dependencies during the process.
@CyrilWaechter
Copy link
Author

CyrilWaechter commented Aug 4, 2021

@mikedh @mmatl I did not expect an answer. Happy to see that the repo is more alive than I thought 😃 .

  • I have rebased my branch on top of master to allow cleaner merge
  • I have udated readme, setup.cfg, dockerfile and travis (in case you keep travis)
  • I have split github actions workflows to upload wheels only on release (There is currently no auto-upload to pypi. A step can be added to perform this).

Let me know if you want me to fix/change something else.

@mmatl
Copy link
Collaborator

mmatl commented Aug 4, 2021

@CyrilWaechter Looks good to me! I'll go ahead and merge it now.

@mikedh
Copy link
Collaborator

mikedh commented Aug 4, 2021

It's not dead, it's just resting it's eyes haha. Getting those PR's across the line is a ton of work, thanks for taking it on!

I installed the wheels generated via this PR built for 3.8/manylinux, and with one minor issue my upstream tests worked. I did have to change data I was passing from float32 to float64 to get upstream tests to pass:

Traceback (most recent call last):
  File "/home/mikedh/trimesh/trimesh/collision.py", line 204, in add_object
    t = fcl.Transform(transform[:3, :3], transform[:3, 3])
ValueError: Buffer dtype mismatch, expected 'double' but got 'float'
ValueError: Buffer dtype mismatch, expected 'double' but got 'float'
Exception ignored in: 'fcl.fcl.numpy_to_vec3d'

If fcl.numpy_to_vec32 requires a float64/double, should it always convert in that function via a .astype?

Only other thought is I think we should probably drop Travis completely and just use Github actions.

@mmatl
Copy link
Collaborator

mmatl commented Aug 4, 2021

@mjd3 Could you merge this PR and do a deploy? I no longer have access rights to this repo, it appears.

@CyrilWaechter
Copy link
Author

I installed the wheels generated via this PR built for 3.8/manylinux, and with one minor issue my upstream tests worked. I did have to change data I was passing from float32 to float64 to get upstream tests to pass:

Traceback (most recent call last):
  File "/home/mikedh/trimesh/trimesh/collision.py", line 204, in add_object
    t = fcl.Transform(transform[:3, :3], transform[:3, 3])
ValueError: Buffer dtype mismatch, expected 'double' but got 'float'
ValueError: Buffer dtype mismatch, expected 'double' but got 'float'
Exception ignored in: 'fcl.fcl.numpy_to_vec3d'

If fcl.numpy_to_vec32 requires a float64/double, should it always convert in that function via a .astype?

Sorry. I have no idea. @DmitryNeverov did the core work. I just fixed bvh path and work on the packaging side. On my side I just checked that all test were passing and that I was able to use clash detection tool from BlenderBIM Add-on which currently use python-fcl.

src/fcl/fcl.pyx Outdated Show resolved Hide resolved
src/fcl/fcl.pyx Outdated Show resolved Hide resolved
@DmitryNeverov
Copy link

@CyrilWaechter
Glad to see this eventually got some traction, thanks for pushing it forward.

I've left suggestions above that should fix

ValueError: Buffer dtype mismatch, expected 'double' but got 'float'

that @mikedh was facing.

mikedh and others added 2 commits August 6, 2021 13:00
Co-authored-by: Dmitry Neverov <[email protected]>
Co-authored-by: Dmitry Neverov <[email protected]>
@mikedh
Copy link
Collaborator

mikedh commented Aug 7, 2021

Awesome thanks @DmitryNeverov @Pebaz @CyrilWaechter for the fixes and all the work on the upgrade! I just checked the regenerated wheels and they fixed the only minor issue I was seeing. It looks like I still have merge privileges on this repo and I'm happy to merge this (I think it's ready to go), although I think the CI gnomes won't release to PyPi correctly until we do the following:

  • @mmatl could you generate a PyPi repo access token for python-fcl?
  • Could someone with admin on this repo (@mjd3?) add that token as PYPI_TOKEN (or something) secret to Github Actions? It looks like I can merge stuff here but not add secrets.
  • I'll delete .travis.ci and add the PyPi release step in a followup PR.

That sound OK?

@mmatl
Copy link
Collaborator

mmatl commented Aug 10, 2021

@mikedh Sorry for the delay! Token has been generated, and @mjd3 will take care of getting it added to the repo's secrets. Feel free to perform the merge and add the github actions PR as soon as you have time.

@mjd3
Copy link

mjd3 commented Aug 11, 2021

@mikedh token is added as PYPI_API_TOKEN to the repo's secrets. Let me know if you need any assistance with the GH Actions PR (although you probably have more experience with that than me)!

@mikedh
Copy link
Collaborator

mikedh commented Aug 11, 2021

Awesome thanks @mjd3 @mmatl! I'll block off an afternoon and get this released, I'll let you know if I run in to trouble haha.

@szobov
Copy link

szobov commented Sep 7, 2021

Hi there,
Thanks a lot for this update,
Any chance it will be released soon?
Cheers

@mikedh mikedh merged commit ffea96f into BerkeleyAutomation:master Sep 9, 2021
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.

6 participants