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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vidurvij-apptronik
Copy link

@vidurvij-apptronik vidurvij-apptronik commented Dec 4, 2023

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.

#1244

Copy link

google-cla bot commented Dec 4, 2023

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.

@vidurvij-apptronik
Copy link
Author

@yuvaltassa We can discuss the issue here?

@yuvaltassa
Copy link
Collaborator

CC @erez-tom, @Balint-H

@Balint-H
Copy link
Contributor

Balint-H commented Jan 16, 2024

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)

@vidurvij-apptronik
Copy link
Author

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

@Balint-H
Copy link
Contributor

Balint-H commented Jan 16, 2024

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.

@vidurvij-apptronik
Copy link
Author

@Balint-H The latter one.

@Balint-H
Copy link
Contributor

Balint-H commented Jan 19, 2024

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)

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.

None yet

4 participants