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

refactor: extract duplicated velocity calculation logic in Contact class #306

Conversation

diogomene
Copy link

What was done?

To eliminate the code duplication identified in the Contact class during the calculation of the relative velocity between two contact points (bodies A and B), the repeated instructions were extracted and centralized into a method named computeRelativeVelocity within the Contact class:

computeRelativeVelocity(
  dv: Vec2Value,
  vA: Vec2Value,
  wA: number,
  rA: Vec2Value,
  vB: Vec2Value,
  wB: number,
  rB: Vec2Value
): void {
  matrix.zeroVec2(dv);
  matrix.plusVec2(dv, vB);
  matrix.plusVec2(dv, matrix.crossNumVec2(temp, wB, rB));
  matrix.minusVec2(dv, vA);
  matrix.minusVec2(dv, matrix.crossNumVec2(temp, wA, rA));
}

This function takes the required parameters and performs the necessary calculations in-place by reference.

With this solution, the duplication has been effectively addressed, improving both the readability and understanding of the code through modularization and reuse of instructions. Additionally, any future changes to the calculation logic can now be made in a single function, avoiding the need to update all three occurrences of the routine.

Resolves #305

@shakiba
Copy link
Collaborator

shakiba commented Dec 1, 2024

@diogomene thanks reviewing the code and finding potential improvements. The code is intentionally verbos to make it easier to read, debug, and profile for performance optimization. It's also easier for browser to optimize at runtime.

As you mentioned, verbos and repetitive code is more difficult when you update the logic, however we almost don't change the logic.

@diogomene
Copy link
Author

@shakiba. Understood. As long as the code is not overly repetitive and not used in other classes, keeping its locality while calling the procedure within the class may be better.

If the logic is unlikely to change, I see no risks in repeating the code within the class.

Thank you so much for the review; I truly appreciate your time and attention.

@diogomene diogomene closed this Jan 1, 2025
@diogomene diogomene deleted the refactor/removeDuplicatedCodeForRelativeVelocityInContact branch January 1, 2025 07:48
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.

Remove repetitive code at Contact, on relative velocity calculation
2 participants