add explicit feedback for illegal operations in GPU mode #295
Conversation
…, throw on set) instead of silent ignore.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates PhysX articulation root getters/setters to behave more explicitly when running with the Direct GPU API, switching from commented-out exceptions to runtime warnings (getters) and hard errors (setters).
Changes:
- Add GPU-simulation warnings for root pose and velocity getters that may return unsynchronized CPU data.
- Add GPU-simulation runtime errors for root pose and velocity setters to prevent illegal writes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| logger::warn("getting root pose may yields unsynchronized CPU data in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| throw std::runtime_error("setting root pose is illegal in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| throw std::runtime_error("setting root linear velocity is illegal in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| throw std::runtime_error("setting root angular velocity is illegal in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| logger::warn("getting root pose may yields unsynchronized CPU data in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| logger::warn("getting root linear velocity may yield unsynchronized CPU data in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| logger::warn("getting root angular velocity may yield unsynchronized CPU data in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| throw std::runtime_error("setting root pose is illegal in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| throw std::runtime_error("setting root linear velocity is illegal in GPU simulation."); | ||
| } |
| if (getRoot()->isUsingDirectGPUAPI()) { | ||
| throw std::runtime_error("setting root angular velocity is illegal in GPU simulation."); | ||
| } |
|
I think your fix is correct and reasonable, but to avoid breaking existing code, we probably should log an error for the set methods as well. Set could also be valid if the entity has not been added to scene or physics has not been initialized. I will look into a more comprehensive fix but for now logging is probably enough. |
|
Ah I see, but |
Hi @fbxiang, regarding the issue mentioned in #292, I've made a simple fix. Note that PhysX also disallows setting root properties in GPU mode, so I think throwing an error (instead of silent ignore) is the right approach here. I've tested it locally in both GPU and CPU modes, and everything runs fine. Please feel free to test it further or let me know any suggestions for improvement.