fix: prevent nan in collisions and enable detection#8
Conversation
Add epsilon check to avoid division by very small distance in handle_collisions, preventing potential NaN velocities and data corruption.
Summary of ChangesHello @bniladridas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the stability of the physics engine by addressing a critical edge case in collision handling. By introducing a minimum distance threshold, it effectively prevents numerical instability that could arise from division by near-zero values, ensuring more robust and predictable simulation behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a potential NaN issue in collision handling by adding an epsilon check. My review has two suggestions: first, to replace the magic number 1e-6 with a named constant for better readability. Second, and more importantly, the handle_collisions function is currently commented out, so the fix is inactive. This needs to be addressed.
| let diff = pos1 - pos2; | ||
| let distance = diff.norm(); | ||
| if distance < rad1 + rad2 && distance > 0.0 { | ||
| if distance < rad1 + rad2 && distance > 1e-6 { |
There was a problem hiding this comment.
This fix is correct in principle, but the handle_collisions function is currently not being called within the simulate method (it's commented out on line 126). For this fix to be effective, the function call needs to be uncommented.
| let diff = pos1 - pos2; | ||
| let distance = diff.norm(); | ||
| if distance < rad1 + rad2 && distance > 0.0 { | ||
| if distance < rad1 + rad2 && distance > 1e-6 { |
There was a problem hiding this comment.
The value 1e-6 is a magic number. To improve readability and maintainability, it's best to define it as a constant with a descriptive name. For example:
const MIN_DISTANCE_EPSILON: f32 = 1e-6;This constant could be defined at the module level and then used here:
if distance < rad1 + rad2 && distance > MIN_DISTANCE_EPSILON {Uncomment handle_collisions call in simulate. Define MIN_DISTANCE_EPSILON constant.
bniladridas
changed the title
1 participant
Summary
simulateMIN_DISTANCE_EPSILONfor maintainabilityWhy this change?
When two objects occupy the same or nearly the same position, the distance used in collision resolution can approach zero.
This can lead to:
Introducing an epsilon threshold ensures numerical stability while preserving correct collision behavior.
Enabling collisions makes the physics simulation more realistic and complete.