-
Notifications
You must be signed in to change notification settings - Fork 400
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
--Bug fixes: Navmesh building and reconfigure in viewer.py #1632
--Bug fixes: Navmesh building and reconfigure in viewer.py #1632
Conversation
src/esp/sim/Simulator.cpp
Outdated
CORRADE_ASSERT( | ||
joinedMesh->vbo.size() > 0, | ||
"::recomputeNavMesh: " | ||
"Unable to compute a navmesh upon a non-existent mesh - " | ||
"the underlying joined collision mesh has no vertices. This is " | ||
"probably due to the current scene being NONE.", | ||
false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have this assert if the joinedMesh is empty? Or just fail? This will probably not occur often - would primarily happen if someone attempts to build a navmesh on the NONE scene, so just failing without the assertion would probably be fine, but I am not sure if there are possible situations where an assertion would be best (i.e. training and somehow an expected collision mesh is empty).
ESP_CHECK(joinedMesh->vbo.size() > 0, | ||
"::recomputeNavMesh: " | ||
"Unable to compute a navmesh upon a non-existent mesh - " | ||
"the underlying joined collision mesh has no vertices. This is " | ||
"probably due to the current scene being NONE. Aborting"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...or is it better to have ESP_CHECK for this, which will throw in python and can be trapped in a try if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach, thanks for adding this check. I've seen the resulting issue a couple times and the current crash message response is non-obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation and Context
This small PR fixes 2 bugs :
How Has This Been Tested
Locally all python and c++ tests pass
Types of changes
Checklist