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

Support generic MeshFileGeometry for mesh types we can't easily load #92

Merged
merged 5 commits into from
Apr 11, 2019

Conversation

rdeits
Copy link
Owner

@rdeits rdeits commented Feb 15, 2019

TODO:

@codecov-io
Copy link

codecov-io commented Feb 15, 2019

Codecov Report

Merging #92 into master will increase coverage by 0.2%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #92     +/-   ##
=========================================
+ Coverage    73.2%   73.41%   +0.2%     
=========================================
  Files          12       12             
  Lines         321      331     +10     
=========================================
+ Hits          235      243      +8     
- Misses         86       88      +2
Impacted Files Coverage Δ
src/objects.jl 100% <ø> (ø) ⬆️
src/MeshCat.jl 30.43% <ø> (ø) ⬆️
src/lowering.jl 82.6% <66.66%> (-0.73%) ⬇️
src/geometry.jl 93.33% <85.71%> (-2.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f6771c...84228cb. Read the comment docs.

@tkoolen
Copy link
Collaborator

tkoolen commented Feb 15, 2019

Just so I understand, what would be the drawbacks of using this always if the source of the mesh is a file on disk, e.g. in MechanismGeometries.URDF? (Of course it's great to retain support for programmatically generated meshes). I think you once mentioned the case where meshcat is running on one computer and the mesh file lives on another, but I guess that's not an issue now if we're just reading the file contents and sending that over the network, rather than just the file name.

@rdeits
Copy link
Owner Author

rdeits commented Feb 15, 2019

There's really not much downside at all, and you're right that it doesn't matter whose computer has the mesh since we send the entire contents.

The only real problem is that it's not totally obvious what a mesh file represents. Is it just a geometry? Many formats support materials and/or textures. But some formats (like DAE) can represent an entire scene with all sorts of behavior attached to it. So it's a little ambiguous to ask meshcat to render a file when that file contains more than just a single mesh geometry.

However, we already have that problem, as MeshIO assumes that a single mesh file produces a single GeometryTypes.HomoegenousMesh result. It's just a little harder to debug that kind of thing when the confusion is happening in javascript-land.

But, all told, this is as good as any solution we currently have available, so we should certainly use it if it's convenient.

@tkoolen
Copy link
Collaborator

tkoolen commented Feb 17, 2019

How do you think we should handle this in MechanismGeometries? MechanismGeometries is viewer-agnostic (as it should be), so it wouldn't have access to MeshCat.MeshFileGeometry.

How about this:

  • have a separate MechanismGeometries.MeshFile that also just stores the mesh file name
  • expand MechanismGeometries.GeometryLike to include MechanismGeometries.MeshFile
  • have MeshCatMechanisms support MechanismGeometries.MeshFile by overloading setelement!
  • have DrakeVisualizer support MechanismGeometries.MeshFile by handling .dae -> .obj file name conversion and mesh loading in its addgeometry! function (or deprecate the package)

@rdeits
Copy link
Owner Author

rdeits commented Mar 3, 2019

That all sounds good to me, although I think we should consider dropping DrakeVisualizer support entirely. I'm happy to keep things viewer-agnostic, but I have no interest in using DrakeVisualizer and don't really want to keep adding features to it.

@tkoolen
Copy link
Collaborator

tkoolen commented Mar 3, 2019

Yep, sounds good. I can put some PRs together. I'm actually hoping this will also help with latency.

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.

3 participants