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

MeshShape::setScale does not accept negative values? #1839

Closed
costashatz opened this issue Aug 29, 2024 · 4 comments · Fixed by #1841
Closed

MeshShape::setScale does not accept negative values? #1839

costashatz opened this issue Aug 29, 2024 · 4 comments · Fixed by #1841
Labels
type: bug Indicates an unexpected problem or unintended behavior

Comments

@costashatz
Copy link
Contributor

I do not understand why MeshShape::setScale() does not accept negative values. This is a common practice to mirror the mesh (otherwise we need to have multiple "duplicate" meshes, especially when using the URDF format).

Also, this check was "recently" added (I did not check when this happened, but it wasn't there for many years that I am using the library). Is there a specific rationale/guideline for this? Imho, we should allow negative scaling.

@costashatz costashatz added the type: bug Indicates an unexpected problem or unintended behavior label Aug 29, 2024
@jslee02
Copy link
Member

jslee02 commented Aug 29, 2024

Accepting negative values makes sense to me. We may need to ensure that it works fine with the codebase (e.g., updating bounding box) and Assimp also allows it.

Regarding when and why it was changed, I don't recall quite frankly. The git log says that line was changed 6 years ago (from checking individual xyz components to using Eigen's all() function) and the previous commit of the 6 years ago also didn't accept negative values.

image image

@costashatz
Copy link
Contributor Author

Oh I see what happened. I was compiling with disabled assertions!

In any case, we should allow for negative scaling. Apart from the bounding box update, where else should we handle this?

@jslee02
Copy link
Member

jslee02 commented Aug 31, 2024

It seems only the updating bounding box. #1841 to continue this discussion!

@jslee02
Copy link
Member

jslee02 commented Sep 3, 2024

Closing per #1841. Please let me know if this works for you!

@jslee02 jslee02 closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants