mujoco icon indicating copy to clipboard operation
mujoco copied to clipboard

Changing indexFormat for the imported mesh from 16 bit to 32 bit.

Open vidurvij-apptronik opened this issue 1 year ago • 8 comments

I am changing the import format from 16 bit to 32 bit to support meshes with higher vertices count which is already supported in MuJoCo standalone.

https://github.com/google-deepmind/mujoco/issues/1244

vidurvij-apptronik avatar Dec 04 '23 21:12 vidurvij-apptronik

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Dec 04 '23 21:12 google-cla[bot]

@yuvaltassa We can discuss the issue here?

vidurvij-apptronik avatar Jan 16 '24 21:01 vidurvij-apptronik

CC @erez-tom, @Balint-H

yuvaltassa avatar Jan 16 '24 21:01 yuvaltassa

I think this is a reasonable addition, but I'd remove the commented out block that previously checked the vertex count. I believe a good follow-up PR would add the vertex count check back in, and use the 16 bit indexing on the mesh if it is sufficient.

Also I took a look at this PR last month (sorry for the delay!) and started to investigate mesh importing a bit closer, as meshes that I'd have expected to be imported easily (based on vert counts from blender) were rejected. I need to understand it a bit more, but it seems the if the imported meshes are reexported, they have more vertices (possibly duplicates) than the original file. However, that is a separate issue from the indexing, and will be addressed in the future (potentially with your help!), and shouldn't block this PR.

Don't forget to go through the CLA process, if you haven't yet. (Look the Details section of the failed check below)

Balint-H avatar Jan 16 '24 22:01 Balint-H

@Balint-H The issue is that I cant sign the CLA yet. Is it possible to for you to make the change?

vidurvij-apptronik avatar Jan 16 '24 22:01 vidurvij-apptronik

Hmm, what do you see when you visit this link (from the failed check's description)? https://cla.developers.google.com/clas

Or do you mean "can't" as in you have IP or other legal concerns that prevent you from signing it? If that's the case I can resubmit this PR.

Balint-H avatar Jan 16 '24 22:01 Balint-H

@Balint-H The latter one.

vidurvij-apptronik avatar Jan 16 '24 22:01 vidurvij-apptronik

Just an update: The PR from me will come later today or tomorrow. I managed to figure out a couple of related issues first. One of these used to make .stls generated by the plugin unopenable by Blender or other graphics software (making debugging problematic). More importantly, vertex indices will be reused now, significantly reducing the mesh asset size, and allowing the use of higher res meshes with the same amount of indices. (Previously an identical vertex was stored as a brand new entry. For closed meshes this used to result in ~6 times more indices than necessary for a mesh)

Balint-H avatar Jan 19 '24 15:01 Balint-H