Skip to content

[AO migration] minor physics changes#1254

Merged
aclegg3 merged 2 commits intomasterfrom
ao-migration-physics-minor
May 21, 2021
Merged

[AO migration] minor physics changes#1254
aclegg3 merged 2 commits intomasterfrom
ao-migration-physics-minor

Conversation

@aclegg3
Copy link
Copy Markdown
Contributor

@aclegg3 aclegg3 commented May 20, 2021

Motivation and Context

ArticulatedObject migration PR split from #1226.
Contains all minor, straightforward changes from #1247:

  • refactoring object activation API
  • refactoring collision compound construction from mesh hierarchy
  • doc changes

How Has This Been Tested

Current tests should cover most of this.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@aclegg3 aclegg3 requested review from eundersander and jturner65 May 20, 2021 17:48
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 20, 2021
Comment thread src/esp/physics/PhysicsManager.h
Copy link
Copy Markdown
Contributor

@jturner65 jturner65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one comment regarding the enumerators for PhysicsSimulationLibrary.

@ref RigidObject implementing the specific interface to a simulation library.
*/
enum PhysicsSimulationLibrary {
enum class PhysicsSimulationLibrary {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would now be a good time to refactor these enumerators to be Pascal case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun migration. Need to work around the None keyword in python. Refactoring to NoPhysics instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I'm leaving the config and metadata tag "none" for now.

@aclegg3 aclegg3 merged commit 878638f into master May 21, 2021
@aclegg3 aclegg3 deleted the ao-migration-physics-minor branch May 21, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants