10000 Support collision group and fix the position of characterController synchronization late issue by luzhuang · Pull Request #2614 · galacean/engine · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support collision group and fix the position of characterController synchronization late issue #2614

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
May 14, 2025
Merged

Support collision group and fix the position of characterController synchronization late issue #2614

merged 20 commits into from
May 14, 2025

Conversation

luzhuang
Copy link
Contributor
@luzhuang luzhuang commented Apr 11, 2025 •
edited by coderabbitai bot
Loading

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Other information:

Summary by CodeRabbit

  • New Features

    • Added support for collision groups in physics engines, enabling control over collision interactions based on group assignments.
    • Introduced methods to configure collision interactions between different collision groups.
    • Enabled setting collision groups on character controllers and colliders across LitePhysics and PhysX engines.
    • Added new end-to-end test cases showcasing collision group behavior in LitePhysics and PhysXPhysics.
    • Updated physics scene and collider classes to integrate collision layer management and filtering.

  • Bug Fixes

    • Enhanced collision detection logic to respect group-based collision filtering rules.

  • Documentation

    • Clarified default values and improved comments related to collision group and contact offset features.

  • Tests

    • Expanded test coverage with comprehensive tests verifying collision group settings and their effects across physics engines.

@luzhuang luzhuang added enhancement New feature or request physics Engine's physical system labels Apr 11, 2025
@luzhuang luzhuang added this to the 1.5 milestone Apr 11, 2025
@luzhuang luzhuang requested a review from GuoLei1990 April 11, 2025 12:20
@luzhuang luzhuang added this to Physic Apr 11, 2025
@github-project-automation github-project-automation bot moved this to In progress in Physic Apr 11, 2025
@coderabbitai coderabbitai
Copy link
coderabbitai bot commented Apr 11, 2025 •
edited
Loading

Walkthrough

This change set introduces collision group management to both LitePhysics and PhysX physics engines. New methods and properties are added to core physics classes and interfaces to support assigning colliders to groups and configuring inter-group collision rules. End-to-end tests, configuration entries, and unit tests are added or updated to validate and demonstrate the new collision group functionality.

Changes

File(s) Change Summary
e2e/case/litePhysics-collision-group.ts, e2e/case/physx-collision-group.ts Added new end-to-end test cases demonstrating collision group behavior for LitePhysics and PhysX, including custom scripts, scene setup, and group-based collision configuration.
e2e/config.ts Added two new test configuration entries for the new collision group tests under the "Physics" category.
packages/core/src/physics/Collider.ts Added _collisionLayerIndex property, getter/setter for collisionLayer, and _setCollisionLayer method to manage collider group assignment and syncing with native colliders during shape changes and synchronization.
packages/core/src/physics/PhysicsScene.ts Added getColliderLayerCollision and setColliderLayerCollision methods to query and configure collision rules between layers with validation of single-layer inputs.
packages/core/src/physics/CharacterController.ts, packages/core/src/physics/DynamicCollider.ts Minor code cleanup and ensured base class logic is called in _handleShapesChanged.
packages/core/src/physics/shape/ColliderShape.ts Updated documentation comment for contactOffset getter to mention the default value of 0.02 and pass change flags in shape change notifications.
packages/design/src/physics/ICollider.ts Added setCollisionLayer(layer: number): void to the collider interface for layer assignment.
packages/design/src/physics/IPhysics.ts Added getColliderLayerCollision and setColliderLayerCollision methods for layer collision management. Also corrected parameter descriptions in joint creation methods.
packages/physics-lite/src/LitePhysics.ts, packages/physics-lite/src/LiteCollider.ts, packages/physics-lite/src/LiteDynamicCollider.ts, packages/physics-lite/src/LiteStaticCollider.ts, packages/physics-lite/src/LitePhysicsScene.ts Refactored constructors to inject LitePhysics dependency, added layer management methods and properties, implemented layer-based collision filtering in the scene's collision detection loops.
packages/physics-physx/src/PhysXPhysics.ts, packages/physics-physx/src/PhysXCollider.ts, packages/physics-physx/src/PhysXCharacterController.ts Added layer management methods, updated imports, and ensured actors/controllers can be assigned to layers and have their collision layer set.
tests/src/core/physics/CharacterController.test.ts, tests/src/core/physics/Collider.test.ts, tests/src/core/physics/PhysicsManager.test.ts Added or updated unit tests to cover collision layer assignment, collision filtering, and edge cases for both LitePhysics and PhysX.
tests/vitest.config.ts Added explicit server port configuration for Vitest.
packages/physics-physx/src/shape/PhysXColliderShape.ts Reordered and grouped private/protected fields with added internal comments; no logic changes.
packages/core/src/physics/enums/ColliderShapeChangeFlag.ts Added new enum ColliderShapeChangeFlag to represent types of shape changes for internal handling.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Engine
    participant PhysicsScene
    participant ColliderA
    participant ColliderB

    User->>Engine: Assign ColliderA to Layer 1
    User->>Engine: Assign ColliderB to Layer 2
    User->>PhysicsScene: setColliderLayerCollision(1, 2, false)
    Engine->>PhysicsScene: Update collision matrix
    Engine->>PhysicsScene: Simulate physics
    PhysicsScene->>ColliderA: Check collision with ColliderB
    alt Layers 1 and 2 cannot collide
        PhysicsScene-->>ColliderA: Skip collision
    else Layers can collide
        PhysicsScene->>ColliderA: Process collision
    end
Loading

Suggested reviewers

  • GuoLei1990
  • cptbtptpbcptdtptp

Poem

🐇 Hopping through layers, collisions in sight,
Groups set with care, making physics just right.
Spheres bounce or pass, as rules now decide,
In code and in tests, no surprises to hide.
A rabbit’s delight, in this physics dance,
Collisions controlled, given every chance! 🎉🐰

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f899d28 and f04ed3b.

📒 Files selected for processing (4)
  • packages/core/src/physics/Collider.ts (8 hunks)
  • packages/core/src/physics/DynamicCollider.ts (2 hunks)
  • packages/core/src/physics/enums/ColliderShapeChangeFlag.ts (1 hunks)
  • packages/core/src/physics/shape/ColliderShape.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/core/src/physics/enums/ColliderShapeChangeFlag.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/core/src/physics/shape/ColliderShape.ts
  • packages/core/src/physics/DynamicCollider.ts
  • packages/core/src/physics/Collider.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: build (22.x, windows-latest)
  • GitHub Check: e2e (22.x)
  • GitHub Check: codecov
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov Codecov
Copy link
codecov bot commented Apr 11, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 51.06888% with 206 lines in your changes missing coverage. Please review.

Project coverage is 68.58%. Comparing base (d069479) to head (f04ed3b).
Report is 1 commits behind head on dev/1.5.

Files with missing lines Patch % Lines
e2e/case/physx-collision-group.ts 0.00% 88 Missing and 1 partial ⚠️
e2e/case/litePhysics-collision-group.ts 0.00% 84 Missing and 1 partial ⚠️
packages/physics-lite/src/LitePhysicsScene.ts 59.25% 11 Missing ⚠️
e2e/config.ts 0.00% 10 Missing ⚠️
packages/physics-lite/src/LitePhysics.ts 89.79% 5 Missing ⚠️
packages/core/src/physics/Collider.ts 89.47% 4 Missing ⚠️
packages/core/src/physics/PhysicsScene.ts 93.75% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           dev/1.5    #2614      +/-   ##
===========================================
- Coverage    68.68%   68.58%   -0.10%     
===========================================
  Files          971      974       +3     
  Lines       102088   102468     +380     
  Branches      8781     8811      +30     
===========================================
+ Hits         70119    70281     +162     
- Misses       31703    31919     +216     
- Partials       266      268       +2
Flag Coverage Δ
unittests 68.58% <51.06%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai[bot]
coderabbitai bot reviewed Apr 11, 2025
View reviewed changes
Copy link
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (13)
e2e/case/litePhysics-collision-group.ts (3)

92-93: Consider translating the Chinese comment to English

The comment "设置立方体的位置和大小" should be translated to English (e.g., "Set the position and size of the cube") for consistency with the rest of the codebase.

- // 设置立方体的位置和大小
+ // Set the position and size of the cube
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 93-93: e2e/case/litePhysics-collision-group.ts#L93
Added line #L93 was not covered by tests

94-94: Remove commented-out code

This commented-out line // groundEntity.isActive = false; appears to be leftover code that isn't needed. Consider removing it if it's not serving any purpose.

- // groundEntity.isActive = false;

132-132: Use named constants instead of magic numbers

The code uses magic numbers 2 and 3 for collision groups. Consider using the Layer enum constants instead, which would make the code more maintainable and readable.

- scene.physics.setColliderGroupCollision(2, 3, false);
+ scene.physics.setColliderGroupCollision(Layer.Layer2, Layer.Layer3, false);
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 132-135: e2e/case/litePhysics-collision-group.ts#L132-L135
Added lines #L132 - L135 were not covered by tests

packages/physics-lite/src/LiteDynamicCollider.ts (1)

16-17: Update JSDoc to reflect parameter changes

The JSDoc comment for the constructor should be updated to include documentation for the new litePhysics parameter.

/**
 * Initialize dynamic actor.
+ * @param litePhysics - The physics engine instance
 * @param position - The global position
 * @param rotation - The global rotation
 */
tests/src/core/physics/PhysicsManager.test.ts (1)

547-584: Duplicated test logic should be refactored.

The collision group tests for PhysXPhysics are identical to the LitePhysics tests. Consider refactoring to use a shared test function to avoid code duplication.

- it("Collision Group Tests", () => {
-   describe("should set and get collision group settings correctly", () => {
-     it("should set and get collision group settings correctly", () => {
-       physicsScene.setColliderGroupCollision(0, 1, true);
-       // ... duplicate test code
-     });
-   });
-   // ... more duplicate test blocks
- });

+ it("Collision Group Tests", () => {
+   testCollisionGroups(physicsScene);
+ });

// Add at the top of the file or in a separate test utility file:
+ function testCollisionGroups(physicsScene: PhysicsScene) {
+   describe("should set and get collision group settings correctly", () => {
+     it("should set and get collision group settings correctly", () => {
+       physicsScene.setColliderGroupCollision(0, 1, true);
+       // ... existing test code
+     });
+   });
+   // ... other test blocks
+ }
e2e/case/physx-collision-group.ts (1)

86-87: Translate Chinese comments to English for consistency.

The file contains Chinese comments that should be translated to English to maintain consistency with the rest of the codebase.

- // 调整相机位置以便更好地观察穿透效果
+ // Adjust camera position for better observation of penetration effect

- // 设置半透明以便能看到穿透的球体
+ // Set semi-transparency to see the penetrating sphere

Also applies to: 106-107

tests/src/core/physics/CharacterController.test.ts (1)

375-376: Consider using descriptive variables rather than magic numbers.

Throughout the tests, there are several comments indicating "restore default settings" but the code uses magic numbers for layer indices. Consider using named constants for better readability.

- // 恢复默认设置
- engine.sceneManager.activeScene.physics.setColliderGroupCollision(1, 2, true);
+ // Restore default settings
+ const layer1 = Layer.Layer1;
+ const layer2 = Layer.Layer2;
+ engine.sceneManager.activeScene.physics.setColliderGroupCollision(layer1, layer2, true);

Also applies to: 429-430, 487-488

packages/physics-physx/src/PhysXPhysics.ts (2)

95-95: Updated script source for WebAssembly mode.

Switching to the new URL looks fine, but be sure the file location is stable and won't break references later.

251-278: Collision group methods look correctly integrated.

  1. setColliderGroupCollision() & getColliderGroupCollision():
    • Provide straightforward group interaction toggles. Confirm that calling code handles potential missing group flags gracefully.
  2. setColliderGroup() & getColliderGroup():
    • Using Math.log2(layer) presumes a power-of-two layer. This logic is valid only if combined layers are forbidden. Confirm that no combined layers are passed, or handle them explicitly.

Otherwise, this addition aligns well with the new collision-group-based design.

tests/src/core/physics/Collider.test.ts (1)

16-16: Vitest imports.

No concerns. Just verify that the testing library version is stable in the project environment.

packages/core/src/physics/Collider.ts (1)

181-181: In _syncNative(), line 181 references this._setCollisionGroupByLayer(this.collisionGroup)?

Consider verifying that this.collisionGroup is already set if syncCollisionGroupByLayer is false. If the user sets collisionGroup manually, there’s no need to override it. Possibly a minor logic port.

packages/physics-lite/src/LitePhysics.ts (2)

159-161: Consider adding validation for the group parameter.

The method correctly sets the collision group, but consider adding validation to ensure the provided group is within valid bounds.

setColliderGroup(collider: LiteCollider, group: Layer): void {
+  if (group < 0 && group !== -1) {
+    console.warn("Invalid collision group value. Group must be non-negative or -1.");
+    return;
+  }
  collider._collisionGroup = group;
}

183-190: The comment doesn't match the implementation logic.

The comment states "If either layer is Layer.Nothing, they can collide", but the implementation doesn't specifically check for Layer.Nothing. Instead, it returns true for any invalid index (when -1 is returned from _getColliderGroupIndex).

getColliderGroupCollision(group1: number, group2: number): boolean {
  const index = this._getColliderGroupIndex(group1, group2);
  if (index > -1) {
    return this._groupCollisionMatrix[index] ?? true;
  }
-  // If either layer is Layer.Nothing, they can collide
+  // If either group is invalid (-1), they can collide by default
  return true;
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa6d0e1 and 0e1cc65.

⛔ Files ignored due to path filters (2)
  • e2e/fixtures/originImage/Physics_litePhysics-collision-group.jpg is excluded by !**/*.jpg
  • e2e/fixtures/originImage/Physics_physx-collision-group.jpg is excluded by !**/*.jpg
📒 Files selected for processing (24)
  • e2e/case/litePhysics-collision-group.ts (1 hunks)
  • e2e/case/physx-collision-group.ts (1 hunks)
  • e2e/config.ts (1 hunks)
  • packages/core/src/Entity.ts (2 hunks)
  • packages/core/src/enums/EntityModifyFlags.ts (1 hunks)
  • packages/core/src/physics/CharacterController.ts (1 hunks)
  • packages/core/src/physics/Collider.ts (6 hunks)
  • packages/core/src/physics/DynamicCollider.ts (2 hunks)
  • packages/core/src/physics/PhysicsScene.ts (1 hunks)
  • packages/core/src/physics/StaticCollider.ts (1 hunks)
  • packages/core/src/physics/shape/ColliderShape.ts (1 hunks)
  • packages/design/src/physics/ICollider.ts (1 hunks)
  • packages/design/src/physics/IPhysics.ts (1 hunks)
  • packages/physics-lite/src/LiteCollider.ts (2 hunks)
  • packages/physics-lite/src/LiteDynamicCollider.ts (2 hunks)
  • packages/physics-lite/src/LitePhysics.ts (5 hunks)
  • packages/physics-lite/src/LitePhysicsScene.ts (5 hunks)
  • packages/physics-lite/src/LiteStaticCollider.ts (2 hunks)
  • packages/physics-physx/src/PhysXCharacterController.ts (2 hunks)
  • packages/physics-physx/src/PhysXCollider.ts (1 hunks)
  • packages/physics-physx/src/PhysXPhysics.ts (5 hunks)
  • tests/src/core/physics/CharacterController.test.ts (2 hunks)
  • tests/src/core/physics/Collider.test.ts (3 hunks)
  • tests/src/core/physics/PhysicsManager.test.ts (5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (12)
packages/core/src/physics/CharacterController.ts (1)
packages/core/src/Component.ts (1)
  • entity (71-73)
packages/design/src/physics/ICollider.ts (1)
packages/core/src/Entity.ts (2)
  • layer (254-256)
  • layer (258-263)
packages/core/src/physics/StaticCollider.ts (1)
packages/core/src/Component.ts (1)
  • entity (71-73)
tests/src/core/physics/PhysicsManager.test.ts (2)
packages/physics-lite/src/LitePhysics.ts (1)
  • LitePhysics (28-204)
packages/core/src/physics/PhysicsScene.ts (1)
  • PhysicsScene (16-463)
e2e/case/litePhysics-collision-group.ts (4)
packages/core/src/mesh/PrimitiveMesh.ts (1)
  • PrimitiveMesh (23-1506)
packages/core/src/physics/shape/SphereColliderShape.ts (1)
  • SphereColliderShape (9-39)
packages/core/src/physics/DynamicCollider.ts (1)
  • DynamicCollider (11-493)
packages/core/src/physics/shape/BoxColliderShape.ts (1)
  • BoxColliderShape (10-48)
e2e/case/physx-collision-group.ts (6)
packages/core/src/mesh/MeshRenderer.ts (1)
  • MeshRenderer (13-190)
packages/core/src/physics/shape/SphereColliderShape.ts (1)
  • SphereColliderShape (9-39)
packages/core/src/physics/DynamicCollider.ts (1)
  • DynamicCollider (11-493)
packages/core/src/lighting/PointLight.ts (1)
  • PointLight (10-80)
packages/core/src/physics/StaticCollider.ts (1)
  • StaticCollider (9-22)
packages/core/src/physics/shape/BoxColliderShape.ts (1)
  • BoxColliderShape (10-48)
packages/design/src/physics/IPhysics.ts (1)
packages/design/src/physics/ICollider.ts (1)
  • ICollider (6-29)
packages/physics-lite/src/LitePhysicsScene.ts (3)
packages/physics-lite/src/LitePhysics.ts (1)
  • LitePhysics (28-204)
packages/design/src/physics/ICollision.ts (1) < 10000 ul dir="auto">
  • ICollision (4-13)
    • packages/design/src/physics/index.ts (1)
    • ICollision (9-9)
    packages/physics-lite/src/LitePhysics.ts (2)
    packages/physics-lite/src/LiteStaticCollider.ts (1)
    • LiteStaticCollider (9-22)
    packages/physics-lite/src/LiteDynamicCollider.ts (1)
    • LiteDynamicCollider (8-206)
    tests/src/core/physics/Collider.test.ts (4)
    packages/core/src/Entity.ts (1)
    • Entity (21-847)
    packages/physics-lite/src/LitePhysics.ts (1)
    • LitePhysics (28-204)
    packages/core/src/physics/DynamicCollider.ts (1)
    • DynamicCollider (11-493)
    packages/core/src/physics/shape/BoxColliderShape.ts (1)
    • BoxColliderShape (10-48)
    packages/core/src/physics/Collider.ts (2)
    packages/core/src/Entity.ts (3)
    • Entity (21-847)
    • layer (254-256)
    • layer (258-263)
    packages/core/src/index.ts (2)
    • Entity (11-11)
    • EntityModifyFlags (42-42)
    packages/core/src/Entity.ts (1)
    packages/core/src/index.ts (1)
    • EntityModifyFlags (42-42)
    🪛 GitHub Check: codecov/patch
    e2e/case/litePhysics-collision-group.ts

    [warning] 21-21: e2e/case/litePhysics-collision-group.ts#L21
    Added line #L21 was not covered by tests

    [warning] 23-28: e2e/case/litePhysics-collision-group.ts#L23-L28
    Added lines #L23 - L28 were not covered by tests

    [warning] 30-30: e2e/case/litePhysics-collision-group.ts#L30
    Added line #L30 was not covered by tests

    [warning] 32-34: e2e/case/litePhysics-collision-group.ts#L32-L34
    Added lines #L32 - L34 were not covered by tests

    [warning] 36-46: e2e/case/litePhysics-collision-group.ts#L36-L46
    Added lines #L36 - L46 were not covered by tests

    [warning] 49-55: e2e/case/litePhysics-collision-group.ts#L49-L55
    Added lines #L49 - L55 were not covered by tests

    [warning] 58-60: e2e/case/litePhysics-collision-group.ts#L58-L60
    Added lines #L58 - L60 were not covered by tests

    [warning] 62-62: e2e/case/litePhysics-collision-group.ts#L62
    Added line #L62 was not covered by tests

    [warning] 64-64: e2e/case/litePhysics-collision-group.ts#L64
    Added line #L64 was not covered by tests

    [warning] 66-67: e2e/case/litePhysics-collision-group.ts#L66-L67
    Added lines #L66 - L67 were not covered by tests

    [warning] 69-72: e2e/case/litePhysics-collision-group.ts#L69-L72
    Added lines #L69 - L72 were not covered by tests

    [warning] 75-76: e2e/case/litePhysics-collision-group.ts#L75-L76
    Added lines #L75 - L76 were not covered by tests

    [warning] 79-82: e2e/case/litePhysics-collision-group.ts#L79-L82
    Added lines #L79 - L82 were not covered by tests

    [warning] 85-88: e2e/case/litePhysics-collision-group.ts#L85-L88
    Added lines #L85 - L88 were not covered by tests

    [warning] 90-90: e2e/case/litePhysics-collision-group.ts#L90
    Added line #L90 was not covered by tests

    [warning] 93-93: e2e/case/litePhysics-collision-group.ts#L93
    Added line #L93 was not covered by tests

    [warning] 97-99: e2e/case/litePhysics-collision-group.ts#L97-L99
    Added lines #L97 - L99 were not covered by tests

    [warning] 101-104: e2e/case/litePhysics-collision-group.ts#L101-L104
    Added lines #L101 - L104 were not covered by tests

    [warning] 107-110: e2e/case/litePhysics-collision-group.ts#L107-L110
    Added lines #L107 - L110 were not covered by tests

    [warning] 112-112: e2e/case/litePhysics-collision-group.ts#L112
    Added line #L112 was not covered by tests

    [warning] 114-121: e2e/case/litePhysics-collision-group.ts#L114-L121
    Added lines #L114 - L121 were not covered by tests

    [warning] 123-130: e2e/case/litePhysics-collision-group.ts#L123-L130
    Added lines #L123 - L130 were not covered by tests

    [warning] 132-135: e2e/case/litePhysics-collision-group.ts#L132-L135
    Added lines #L132 - L135 were not covered by tests

    e2e/case/physx-collision-group.ts

    [warning] 21-21: e2e/case/physx-collision-group.ts#L21
    Added line #L21 was not covered by tests

    [warning] 23-24: e2e/case/physx-collision-group.ts#L23-L24
    Added lines #L23 - L24 were not covered by tests

    [warning] 26-28: e2e/case/physx-collision-group.ts#L26-L28
    Added lines #L26 - L28 were not covered by tests

    [warning] 30-31: e2e/case/physx-collision-group.ts#L30-L31
    Added lines #L30 - L31 were not covered by tests

    [warning] 33-34: e2e/case/physx-collision-group.ts#L33-L34
    Added lines #L33 - L34 were not covered by tests

    [warning] 36-38: e2e/case/physx-collision-group.ts#L36-L38
    Added lines #L36 - L38 were not covered by tests

    [warning] 41-50: e2e/case/physx-collision-group.ts#L41-L50
    Added lines #L41 - L50 were not covered by tests

    ⏰ Context from checks skipped due to timeout of 90000ms (1)
    • GitHub Check: e2e (22.x)
    🔇 Additional comments (63)
    packages/design/src/physics/ICollider.ts (1)

    19-23: Well-designed API extension for collision group support

    The new setCollisionGroup method is a clean addition to the interface that enables collision filtering by group. The method signature and documentation are clear, taking the entity's layer as a parameter to determine the collision group.

    packages/core/src/enums/EntityModifyFlags.ts (1)

    8-10: Appropriate addition to track layer changes

    The new Layer flag follows the established bit flag pattern (powers of 2) and is properly documented. This enum value will allow the system to track and respond to entity layer changes, which is essential for the collision group functionality.

    packages/core/src/physics/CharacterController.ts (1)

    84-84:

    ✅ Verification successful

    Implement collision group based on entity layer

    This addition correctly sets the collision group based on the entity's layer during initialization. This implements the behavior required by the new ICollider.setCollisionGroup interface method.

    Consider whether the collision group should be updated when the entity's layer changes after construction. Since you've added EntityModifyFlags.Layer, you might need a way to react to layer changes. Verify if this is handled elsewhere in the codebase:

    🏁 Script executed:

    #!/bin/bash
# Check if there's a listener for layer changes that updates collision groups
rg -A 5 -B 5 "_setCollisionGroupByLayer" --type ts
rg -A 10 -B 5 "EntityModifyFlags\.Layer" --type ts

    Length of output: 6932

    Collision group initialization and dynamic updates are handled correctly.

    • The CharacterController (as well as StaticCollider and DynamicCollider) correctly initializes the collision group via a call to _setCollisionGroupByLayer(entity.layer) in the constructor.
    • Runtime updates on the entity's layer are managed centrally in the base Collider class: when the entity’s layer is changed, the entity’s setter dispatches EntityModifyFlags.Layer, which triggers the _onEntityModified listener—calling _setCollisionGroupByLayer accordingly.
    • If dynamic updates for the collision group in CharacterController are desired, ensure that the inherited implementation (via the syncCollisionGroupByLayer flag) is enabled.

    Overall, the current implementation meets the new ICollider.setCollisionGroup requirements without additional modifications.

    packages/core/src/physics/StaticCollider.ts (1)

    20-20:

    ❓ Verification inconclusive

    Implement collision group for static colliders

    This addition correctly sets the collision group based on the entity's layer during initialization, consistent with the pattern used in other collider types.

    Similar to the CharacterController, verify if the collision group is updated when the entity's layer changes after construction:

    🏁 Script executed:

    #!/bin/bash
# Look for the implementation of _setCollisionGroupByLayer method
rg -A 10 ".*\s+_setCollisionGroupByLayer\s*\(.*\)\s*\{" --type ts

    Length of output: 67

    Action Required: Verify Dynamic Collision Group Updates in StaticCollider

    The code currently sets the collision group during initialization with:

    this._setCollisionGroupByLayer(entity.layer);

    However, similar to the CharacterController, we need to ensure that the collision group is updated if the entity’s layer changes after construction. Our initial search for the _setCollisionGroupByLayer implementation returned no results, so it’s unclear if dynamic updates are handled automatically or if additional logic is needed.

    Please manually verify that:

    • The _setCollisionGroupByLayer method (or its equivalent) is implemented and invoked when the entity’s layer changes post-construction.
    • Its behavior aligns with the update mechanism in CharacterController.
    packages/core/src/physics/shape/ColliderShape.ts (1)

    53-54: Documentation improvement looks good.

    Adding the default value in the documentation is a helpful improvement for users of the API, making it clearer what to expect from this property.

    e2e/config.ts (1)

    201-209: Test configuration additions look good.

    The new entries for testing collision groups with both LitePhysics and PhysXPhysics engines follow the existing pattern and will ensure proper coverage of the new functionality.

    packages/physics-physx/src/PhysXCollider.ts (1)

    64-76: Implementation of collision group methods is correct.

    Both methods properly implement the interface requirements for setting and getting collision groups, delegating to the underlying PhysX engine implementation.

    packages/physics-lite/src/LiteStaticCollider.ts (1)

    4-4: Constructor update for dependency injection.

    The change properly adds the LitePhysics dependency to the constructor and passes it to the parent class. This is aligned with the broader collision group implementation across the physics system.

    Also applies to: 17-18

    packages/physics-physx/src/PhysXCharacterController.ts (2)

    109-118: Implementation looks good

    The setCollisionGroup method implementation looks correct. It properly handles the case when the actor is null, and delegates to the physics engine's native method.

    150-150: Position initialization improvement

    The addition of setting the position after controller creation ensures consistent initialization. This is a good improvement.

    packages/core/src/physics/PhysicsScene.ts (1)

    234-252: Well-documented collision group methods

    The new methods for managing collision groups are well-documented and properly implemented. They delegate to the native physics implementation which is the correct approach.

    packages/physics-lite/src/LiteDynamicCollider.ts (2)

    4-4: Appropriate import addition

    Adding the import for LitePhysics is necessary for the constructor parameter change.

    16-17: Good dependency injection approach

    Changing the constructor to accept the LitePhysics instance and passing it to the parent constructor is a good practice for dependency injection. This improves testability and reduces global dependencies.

    tests/src/core/physics/PhysicsManager.test.ts (5)

    11-11: Added import for PhysicsScene is appropriate.

    The addition of PhysicsScene is needed for the new collision group test functionality.

    118-119: Good initialization of physics variables.

    Adding these variables at the class level provides better access throughout the test suite rather than using local variables.

    121-127: Physics engine initialization is properly structured.

    The code properly:

    1. Creates the LitePhysics instance
    2. Passes it to the engine creation
    3. Gets a reference to the PhysicsScene from the active scene

    This structure ensures tests have access to both the raw physics engine and the physics scene wrapper.

    349-386: Comprehensive test coverage for collision group functionality.

    The tests thoroughly validate the collision group API:

    • Basic collision group settings (enable/disable collisions between groups)
    • Symmetrical behavior (group1→group2 matches group2→group1)
    • Edge cases (maxGroup = 31)
    • Invalid inputs (-1)

    This test structure provides good coverage for the new collision group feature.

    396-404: Consistent setup for both physics engines.

    The variable setup and initialization for PhysXPhysics matches the LitePhysics implementation, ensuring consistent testing patterns across both physics implementations.

    packages/core/src/physics/DynamicCollider.ts (1)

    313-313: Good initialization of collision group based on entity layer.

    The call to _setCollisionGroupByLayer(entity.layer) ensures that the collision group is set immediately after the collider is created, based on the entity's layer value.

    packages/core/src/Entity.ts (2)

    145-145: Improved encapsulation by making _layer private.

    Converting the public layer property to a private field with accessor methods is a good practice for encapsulation, allowing for controlled access and modification.

    251-263: Well-implemented property accessors with change notification.

    The layer property accessors are well-implemented:

    1. The getter simply returns the private field value
    2. The setter checks for actual changes before:
      • Updating the value
      • Dispatching a modification event to notify dependent components

    This enables proper reactivity to layer changes throughout the system.

    packages/physics-lite/src/LiteCollider.ts (5)

    2-2: Simplified import with package consolidation.

    The import was updated to use a consolidated import from the main engine package rather than separate imports, improving code readability.

    7-7: Added necessary dependency on LitePhysics.

    The addition of LitePhysics import and property enables the collider to interact with the physics system for collision group management.

    Also applies to: 15-15

    24-24: Default collision group set appropriately.

    Setting the default collision group to Layer.Everything ensures that new colliders will interact with all other colliders by default, which is the expected behavior.

    26-29: Constructor properly updated for dependency injection.

    The constructor now:

    1. Accepts a LitePhysics parameter
    2. Properly initializes the _litePhysics field

    This follows good dependency injection practices.

    75-81: Complete implementation of collision group interface.

    The setCollisionGroup method:

    1. Has proper JSDoc documentation referencing the interface
    2. Correctly delegates to the LitePhysics instance's setColliderGroup method

    This completes the implementation of the ICollider interface for collision groups.

    e2e/case/physx-collision-group.ts (4)

    1-24: Well-structured file setup with appropriate imports.

    The file is properly organized with a descriptive title and category, and imports all necessary components from the Galacean engine. The imports are well-ordered from core components to specialized physics modules.

    🧰 Tools
    🪛 GitHub Check: codecov/patch

    [warning] 21-21: e2e/case/physx-collision-group.ts#L21
    Added line #L21 was not covered by tests

    [warning] 23-24: e2e/case/physx-collision-group.ts#L23-L24
    Added lines #L23 - L24 were not covered by tests

    26-38: The CheckScript implementation looks good.

    The script correctly implements both onTriggerEnter and onContactEnter handlers, providing visual feedback by changing the entity color to green when collisions occur. This approach helps visualize collision events effectively.

    🧰 Tools
    🪛 GitHub Check: codecov/patch

    [warning] 26-28: e2e/case/physx-collision-group.ts#L26-L28
    Added lines #L26 - L28 were not covered by tests

    [warning] 30-31: e2e/case/physx-collision-group.ts#L30-L31
    Added lines #L30 - L31 were not covered by tests

    [warning] 33-34: e2e/case/physx-collision-group.ts#L33-L34
    Added lines #L33 - L34 were not covered by tests

    [warning] 36-38: e2e/case/physx-collision-group.ts#L36-L38
    Added lines #L36 - L38 were not covered by tests

    40-72: Well-implemented helper function for creating physics spheres.

    The createPhysicsSphere function is well-structured, with clear separation of concerns for visual representation and physics properties. Good job setting the physics material properties and adding the necessary components.

    🧰 Tools
    🪛 GitHub Check: codecov/patch

    [warning] 41-50: e2e/case/physx-collision-group.ts#L41-L50
    Added lines #L41 - L50 were not covered by tests

    74-146: Good implementation of collision group demo.

    The setup demonstrates the collision group functionality well. The scene includes:

    1. A semi-transparent ground plane (Layer3)
    2. A red sphere that can collide (Layer1)
    3. A blue sphere that can pass through (Layer2)

    Line 142 correctly sets up the collision rules between groups 2 and 3, preventing the blue sphere from colliding with the ground. The test effectively showcases the new collision group functionality.

    packages/design/src/physics/IPhysics.ts (1)

    137-166: Well-documented collision group interface methods.

    The four new methods are well-defined with clear JSDoc documentation:

    • setColliderGroupCollision: Controls whether two collision groups can interact
    • getColliderGroupCollision: Retrieves the collision status between groups
    • setColliderGroup: Assigns a collision group to a collider
    • getColliderGroup: Retrieves a collider's assigned group

    These methods provide essential functionality for implementing physics collision groups and follow the existing interface pattern consistently.

    packages/physics-lite/src/LitePhysicsScene.ts (3)

    10-10: Good integration of LitePhysics dependency.

    The code correctly adds a dependency on LitePhysics by importing it, adding a private member variable, and accepting it in the constructor. This enables access to the collision group functionality from the physics implementation.

    Also applies to: 36-47

    223-232: Efficient collision group check implementation.

    The code correctly skips collision detection when layers can't collide, which should improve performance by avoiding unnecessary calculations. Good job implementing this optimization at the appropriate locations in the collision detection paths.

    Also applies to: 257-262

    366-375: Well-implemented collision group check method.

    The _checkColliderCollide method is implemented correctly:

    1. It first checks if both colliders are in the same group (allowing collision)
    2. Otherwise, it delegates to the physics engine to determine if the groups can collide

    This approach follows good design principles by delegating complex collision matrix decisions to the physics engine implementation.

    tests/src/core/physics/CharacterController.test.ts (4)

    12-13: Appropriate import additions.

    The addition of ControllerCollisionFlag and Layer imports is appropriate for the new test cases, enabling layer-specific testing.

    327-377: Thorough test for basic collision group functionality.

    This test effectively verifies that:

    1. Collision detection works as expected by default
    2. Collision can be disabled between specific layers
    3. The trigger detection honors the collision group settings

    The test setup is clear and the assertions validate the expected behavior correctly.

    379-431: Good test for layer change behavior.

    This test properly verifies that changing an entity's layer correctly updates its collision group, ensuring that:

    1. Collision is initially disabled between layers 1 and 2
    2. When the obstacle's layer changes to Layer3, collision is enabled
    3. The trigger detection responds appropriately to the layer change

    This is important to verify that the entity layer system integrates correctly with the collision group system.

    433-489: Comprehensive test for manual collision group settings.

    This test validates the manual override capability where developers can:

    1. Disable automatic synchronization between layers and collision groups
    2. Manually set collision groups to custom values
    3. Configure specific collision rules for those custom groups

    The test demonstrates how to use more advanced collision group configurations and verifies they work correctly.

    packages/physics-physx/src/PhysXPhysics.ts (2)

    1-1: Good addition of Layer import.

    This import enables the new collision group functionality that depends on layer-based collision settings.

    150-160: Renamed return value from manager to scene.

    This clarifies the object's purpose and avoids ambiguity. Ensure all references in the codebase use scene consistently.

    tests/src/core/physics/Collider.test.ts (7)

    3-3: Imported Collision enum or type.

    Confirm that the collision type is used purely for the new detection script. Looks correct for these test cases.

    6-6: Layer import for layer-based tests.

    This makes sense, given the new layer collision tests below.

    14-14: Introducing LitePhysics in tests.

    The tests incorporate both PhysX and LitePhysics to ensure consistent coverage. This is a positive approach to validating multiple backends.

    44-58: New CollisionDetectionScript class.

    1. Simple boolean flagging approach is user-friendly for verifying collisions in tests.
    2. The reset() method is clear and ensures test isolation.

    Well-structured test helper.

    384-384: New block “Collider Layer Collision Tests.”

    The overall structure is good. It logically groups tests for verifying collision layer interactions.

    385-622: “LitePhysics Layer Collision” describe block.

    • Multiple it tests verify different layering scenarios: enabling/disabling collisions, changing layers mid-test, manual group assignment, etc.
    • The approach thoroughly tests typical user flows for collisions.

    Ensure that all test cases accurately reflect real-world usage of layer-based collisions and that any corner cases (like no collisions or multiple combined layers) have coverage.

    623-723: “PhysXPhysics Layer Collision” describe block.

    Mirrors the LitePhysics test suite. This parity is valuable for cross-engine consistency.

    packages/core/src/physics/Collider.ts (6)

    10-11: EntityModifyFlags import and new Layer import.

    These imports align with the new collision group synchronization feature.

    39-53: syncCollisionGroupByLayer property logic.

    • Automatically registers/unregisters the entity modification listener.
    • This design is sound for enabling/disabling dynamic layer sync.

    No immediate concerns, just confirm that turning sync on/off mid-game doesn't cause unexpected reassignments.

    55-71: collisionGroup property logic.

    • Enforcing the 0–31 range is appropriate for typical 32-bit mask usage.
    • Emitting a warning if syncCollisionGroupByLayer is still enabled is a good safeguard.

    Implementation is consistent with the new design.

    172-175: _handleShapesChanged() reassigning the collision group.

    This helps ensure that any shape-level changes remain in sync with the collider’s group.

    210-228: _setCollisionGroupByLayer(layer: Layer) method.

    • Handle the “Nothing” and combined-layer checks.
    • Use Math.log2(layer) for power-of-two layers.

    This method is properly guarded with warnings. Just ensure that usage in the game code can’t inadvertently pass layer = 0 for a “no group” scenario.

    Would you like to confirm that no code path tries to set an entity’s layer to Layer.Nothing for colliders that rely on non-zero collision group? We can run a quick search for references to .layer = Layer.Nothing in the codebase if needed.

    230-235: _onEntityModified with the Layer flag.

    Effectively updates the collision group whenever the entity’s layer changes. This matches the layering sync approach.

    packages/physics-lite/src/LitePhysics.ts (10)

    1-1: Import addition looks good.

    Adding the Layer import from "@galacean/engine" is necessary to support the new collision group functionality.

    6-6: Import addition looks good.

    Adding ICollider to the imports from "@galacean/engine-design" is appropriate for the collision group implementation.

    29-30: Appropriate data structure for collision matrix.

    Using a boolean array for the collision matrix is a space-efficient approach for storing the collision states between different groups.

    58-58: Required change for collision group functionality.

    Passing this to the LitePhysicsScene constructor is necessary to provide access to the collision group management methods.

    72-72: Required change for collision group functionality.

    Passing this to the LiteStaticCollider constructor is necessary to provide access to the collision group management methods.

    79-79: Required change for collision group functionality.

    Passing this to the LiteDynamicCollider constructor is necessary to provide access to the collision group management methods.

    166-168: Implementation is correct.

    The method correctly retrieves the collision group from the collider.

    173-178: Implementation is correct.

    The method correctly updates the collision state between two groups in the collision matrix.

    192-203: Efficient index calculation for collision matrix.

    The method efficiently calculates a unique index for each pair of collision groups using the triangular number formula, which ensures that each unique pair of groups maps to a unique index in the collision matrix.

    156-203:

    ✅ Verification successful

    Consider adding integration tests for the collision group functionality.

    The implementation of collision groups is a significant feature addition. Consider adding comprehensive tests to verify that collision groups work correctly across different scenarios.

    🏁 Script executed:

    #!/bin/bash
# Check if tests for collision groups exist
echo "Searching for tests related to collision groups..."
rg -i "collision.*group|group.*collision" --type ts tests/

    Length of output: 10227

    Integration tests for collision groups functionality are already present.
    After verifying the test files in tests/src/core/physics/PhysicsManager.test.ts, tests/src/core/physics/CharacterController.test.ts, and tests/src/core/physics/Collider.test.ts, it appears that the collision group functionality—covering valid scenarios, edge cases, and invalid inputs—is well tested. No further integration tests are necessary at this time.

    coderabbitai[bot]
    coderabbitai bot reviewed Apr 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 2

    ♻️ Duplicate comments (2)
    packages/core/src/physics/Collider.ts (2)

    29-30: Uninitialized _collisionGroup.

    When _syncCollisionGroupByLayer is true and the entity uses Layer.Nothing (0), _collisionGroup remains undefined. This may cause _nativeCollider.setCollisionGroup(this.collisionGroup) to receive undefined.

    - protected _collisionGroup: number;
+ protected _collisionGroup: number = 0;

    This ensures a safe default to avoid undefined usage.

    237-239: 🛠️ Refactor suggestion

    Ensure _setCollisionGroup handles all cases.

    The _setCollisionGroup method should verify that this.collisionGroup is defined before setting it on the native collider. This reinforces the need for initializing _collisionGroup with a default value.

    protected _setCollisionGroup(): void {
- this._nativeCollider.setCollisionGroup(this.collisionGroup);
+ // Ensure we have a valid collision group value
+ const group = this.collisionGroup !== undefined ? this.collisionGroup : 0;
+ this._nativeCollider.setCollisionGroup(group);
}
    🧹 Nitpick comments (2)
    packages/physics-lite/src/LiteCollider.ts (1)

    23-24: Consider assigning a default value to _collisionGroup.

    Currently, _collisionGroup might remain undefined if setCollisionGroup() is never called. Setting a default value (e.g., 0 for “default layer”) can help avoid potential undefined usage.

    - /** @internal */
- _collisionGroup: number;
+ /** @internal */
+ _collisionGroup: number = 0;
    packages/physics-lite/src/LitePhysics.ts (1)

    173-183: Consider double-checking the default collision fallback.

    Currently, getColliderGroupCollision() returns true if the matrix value is undefined. Confirm that defaulting to true is desired. Some systems use a “fail safe” approach that treats undefined as false to avoid unintended collisions.

    - return this._groupCollisionMatrix[index] ?? true;
+ return this._groupCollisionMatrix[index] ?? false;
    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 0e1cc65 and f9df061.

    📒 Files selected for processing (5)
    • packages/core/src/physics/Collider.ts (6 hunks)
    • packages/design/src/physics/IPhysics.ts (1 hunks)
    • packages/physics-lite/src/LiteCollider.ts (2 hunks)
    • packages/physics-lite/src/LitePhysics.ts (5 hunks)
    • packages/physics-physx/src/PhysXPhysics.ts (5 hunks)
    🚧 Files skipped from review as they are similar to previous changes (1)
    • packages/physics-physx/src/PhysXPhysics.ts
    🧰 Additional context used
    🧬 Code Graph Analysis (3)
    packages/design/src/physics/IPhysics.ts (2)
    packages/design/src/physics/ICollider.ts (1)
    • ICollider (6-29)
    packages/design/src/physics/index.ts (1)
    • ICollider (2-2)
    packages/physics-lite/src/LitePhysics.ts (2)
    packages/physics-lite/src/LiteStaticCollider.ts (1)
    • LiteStaticCollider (9-22)
    packages/physics-lite/src/LiteDynamicCollider.ts (1)
    • LiteDynamicCollider (8-206)
    packages/core/src/physics/Collider.ts (4)
    packages/core/src/Component.ts (1)
    • entity (71-73)
    packages/core/src/Entity.ts (3)
    • Entity (21-847)
    • layer (254-256)
    • layer (258-263)
    packages/core/src/index.ts (2)
    • Entity (11-11)
    • EntityModifyFlags (42-42)
    packages/core/src/clone/CloneManager.ts (1)
    • ignoreClone (9-11)
    🔇 Additional comments (21)
    packages/core/src/physics/Collider.ts (7)

    10-11: Good addition of imports for new collision group functionality.

    The imports for EntityModifyFlags and Layer are necessary for the collision group management and layer-based synchronization features.

    39-53: Proper sync mechanism implementation.

    The getter and setter for syncCollisionGroupByLayer correctly handle registration and unregistration of entity modification listeners. When enabled, it automatically updates the collision group based on the entity's layer.

    55-71: Solid validation in the collisionGroup setter.

    Good job implementing validation to ensure the collision group value stays between 0 and 31, and warning users when they try to manually set a collision group while sync is enabled.

    79-80: Good event listener binding.

    Properly binding the _onEntityModified method and registering it as a listener ensures the collision group stays in sync with the entity's layer.

    172-174: Proper collision group update on shape changes.

    The modified _handleShapesChanged method now ensures the collision group is set when shapes change, which maintains consistency.

    196-196: Proper cleanup in destructor.

    Good practice to unregister the entity modification listener when the collider is destroyed, preventing memory leaks.

    230-235: Good entity layer change handling.

    The _onEntityModified method is correctly decorated with @ignoreClone and properly handles entity layer changes by updating the collision group.

    packages/design/src/physics/IPhysics.ts (2)

    137-145: Add boundary checks or clarifications on valid group IDs.

    The newly introduced setColliderGroupCollision() method looks logically sound. However, it might be helpful to clarify or enforce valid group ID ranges (e.g., ensure group IDs used downstream won’t cause array overflows or collisions outside the desired mask). Additionally, consider documenting the default collision state if a group pairing has never been explicitly set, for predictable behavior across implementations.

    146-151: Method signature is clear and consistent.

    The setColliderGroup() method and corresponding docs look good. The API naming is consistent, and the parameters are self-explanatory. No concerns here.

    packages/physics-lite/src/LiteCollider.ts (4)

    2-2: Imports are appropriate.

    The addition of Layer from @galacean/engine and importing LitePhysics aligns with the new collision group functionality. No issues found.

    Also applies to: 7-7

    15-15: Storing the physics reference internally is a good design choice.

    Having a private _litePhysics instance allows for centralized collision group management. This approach is suitable for referencing methods in LitePhysics.

    26-28: Constructor assignment appears correct.

    Passing LitePhysics in the constructor, then storing it in _litePhysics ensures colliders can invoke collision group logic. No issues found.

    75-81: Delegation to _litePhysics is straightforward.

    setCollisionGroup() successfully defers the group assignment to LitePhysics. Implementation is consistent with the new interface specification.

    packages/physics-lite/src/LitePhysics.ts (8)

    1-1: Revised imports align with new collision group functionality.

    Bringing in Layer plus reorganizing references to ICollider ensures usage of typed collisions and updated method signatures. No issues here.

    Also applies to: 6-6

    29-30: Validate collision matrix capacity.

    Introducing _groupCollisionMatrix is a pragmatic solution. Confirm that you won’t exceed practical memory usage if group IDs are large or used extensively. If valid group IDs are small and fairly limited, this approach is fine.

    Would you like me to generate a script to check typical usage of collision group IDs across the codebase?

    58-58: Passing current instance to scene for group checks.

    Supplying this to the LitePhysicsScene constructor ensures any scene-level collision queries can reference your collision matrix. Implementation is consistent.

    72-72: Static collider construction updated correctly.

    Forwarding this to LiteStaticCollider matches the new constructor signature. No issues.

    79-79: Dynamic collider construction updated correctly.

    Similar approach to static collider—no issues found here either.

    156-162: setColliderGroup() simply assigns _collisionGroup.

    The method’s straightforward implementation is consistent with the design file. Additional logic (e.g., toggling collisions or re-checking shapes) can be appended if required in the future.

    163-172: Index-based collision toggling is straightforward.

    setColliderGroupCollision() leverages _groupCollisionMatrix correctly. The use of _getColliderGroupIndex() ensures collisions for a pair of groups is stored in a single index. No immediate concerns.

    185-196:

    ✅ Verification successful

    Verify that 32 accurately represents Layer.Nothing.

    The _getColliderGroupIndex() method returns -1 when group1 or group2 is 32. Double-check that 32 does indeed correspond to “no collision” as expected. If this logic changes, collisions or ignoring collisions may be misapplied.

    🏁 Script executed:

    #!/bin/bash
# Searching for references to Layer.Nothing or numeric 32 in the codebase
rg 'Layer\.Nothing|[^A-Za-z0-9_]32[^A-Za-z0-9_]' -A 4

    Length of output: 72450

    Action: Confirm use of literal 32 for the "nothing" layer in collision logic

    The search results show that the _getColliderGroupIndex() method checks for a layer value of 32 and that the surrounding comment clearly indicates that a group value of 32 is used to represent Layer.Nothing (i.e. “no collision”). Based on this evidence, it appears that 32 is indeed intended to denote the "nothing" layer.

    A couple of points to consider:

    • If an enum or constant (e.g., Layer.Nothing) is defined elsewhere in the codebase, using it here instead of the literal 32 would make the intent clearer and help avoid future mismatches if the underlying value ever changes.
    • Please ensure that any future modifications to the layer enumeration are reflected consistently in this collision logic to prevent inadvertent collision errors.

    coderabbitai[bot]
    coderabbitai bot reviewed Apr 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 1

    🧹 Nitpick comments (3)
    packages/physics-physx/src/PhysXPhysics.ts (2)

    252-257: New method for configuring collision between groups.

    This method provides a way to specify whether two collision groups should interact, which is essential for implementing selective collision detection.

    However, consider adding additional documentation explaining:

    1. What valid values are for group1 and group2
    2. Whether there are any performance implications of enabling/disabling collisions between groups
    3. How this relates to the Layer system mentioned in the imports

    259-264: New method for setting a collider's group.

    This implementation correctly uses the PhysX API to set the collision group of a specific collider.

    Consider adding documentation about:

    1. The range of valid group values
    2. How these groups relate to any existing physics layers
    packages/physics-physx/src/PhysXCollider.ts (1)

    64-69: New method for setting collision group on a collider.

    This method provides a clean interface for PhysX colliders to set their collision group, delegating to the parent PhysXPhysics instance.

    However, there's a naming inconsistency - this method is called setCollisionGroup while the corresponding method in PhysXPhysics is called setColliderGroup. Consider standardizing the naming across the codebase for better maintainability.

    - setCollisionGroup(group: number): void {
+ setColliderGroup(group: number): void {
    this._physXPhysics.setColliderGroup(this, group);
}

    Alternatively, if both names need to remain for compatibility, ensure they're documented consistently.

    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between f9df061 and 2c53eef.

    📒 Files selected for processing (5)
    • packages/physics-lite/src/LiteDynamicCollider.ts (2 hunks)
    • packages/physics-lite/src/LitePhysicsScene.ts (5 hunks)
    • packages/physics-lite/src/LiteStaticCollider.ts (2 hunks)
    • packages/physics-physx/src/PhysXCollider.ts (1 hunks)
    • packages/physics-physx/src/PhysXPhysics.ts (5 hunks)
    🚧 Files skipped from review as they are similar to previous changes (2)
    • packages/physics-lite/src/LiteStaticCollider.ts
    • packages/physics-lite/src/LiteDynamicCollider.ts
    🧰 Additional context used
    🧬 Code Graph Analysis (2)
    packages/physics-physx/src/PhysXPhysics.ts (1)
    packages/physics-physx/src/PhysXPhysicsScene.ts (1)
    • PhysXPhysicsScene (11-287)
    packages/physics-lite/src/LitePhysicsScene.ts (3)
    packages/physics-lite/src/LitePhysics.ts (1)
    • LitePhysics (28-197)
    packages/design/src/physics/ICollision.ts (1)
    • ICollision (4-13)
    packages/design/src/physics/index.ts (1)
    • ICollision (9-9)
    🔇 Additional comments (11)
    packages/physics-physx/src/PhysXPhysics.ts (4)

    1-1: Update to imports reflects new functionality.

    The import change from version to Layer suggests this file will now work with the Layer system, which is commonly used for collision filtering.

    6-6: Good addition of ICollider interface import.

    This import is necessary for the new collision group functionality, as it's used in the parameter type for the new setColliderGroup method.

    151-161: Variable renamed from 'manager' to 'scene' for clarity.

    This change improves readability by using a name that better reflects the nature of the object (a PhysXPhysicsScene).

    96-96:

    ✅ Verification successful

    WebAssembly source URL has been updated.

    Make sure this new URL has been tested and confirmed to work correctly in all target environments.

    🏁 Script executed:

    #!/bin/bash
# Verify if the WebAssembly file is accessible
curl -I "https://mdn.alipayobjects.com/rms/afts/file/A*H4ElTYwBxwgAAAAAAAAAAAAAARQnAQ/physx.release.js"

    Length of output: 1830

    URL Verified: WebAssembly Source URL is Accessible

    • The WebAssembly script URL in packages/physics-physx/src/PhysXPhysics.ts (line 96) has been confirmed accessible with an HTTP/2 200 response and valid content headers.
    • This verifies that the new URL is correctly serving the file, so no further changes are needed here.
    packages/physics-physx/src/PhysXCollider.ts (1)

    1-87: Add test coverage for collision groups.

    While the implementation looks solid, the PR description notes that tests haven't been added. Please add tests to verify:

    1. Setting collision groups on different colliders
    2. Enabling/disabling collisions between specific groups
    3. Interaction with the Layer system

    This will ensure the collision group functionality works as expected and remains working through future updates.

    packages/physics-lite/src/LitePhysicsScene.ts (6)

    10-10: LGTM: Import of LitePhysics class

    Import is appropriate since the class now uses the LitePhysics class as a dependency.

    36-36: Good addition of the private physics property

    This private property is appropriately added to store the reference to the LitePhysics instance.

    223-230: Good collision optimization for box shapes

    Adding a collision group check before performing detailed collision detection is an appropriate performance optimization. This ensures that colliders that shouldn't interact based on their group settings skip the more expensive collision calculations.

    257-264: Good collision optimization for sphere shapes

    Similar to the box collision optimization, this correctly applies collision group filtering to sphere-based collisions.

    367-376: Well-designed collision group check method

    The _checkColliderCollide method effectively encapsulates the logic for determining if two colliders can interact based on their collision groups. The implementation correctly:

    1. First checks if colliders belong to the same group (collision allowed)
    2. If not, defers to the physics engine's collision matrix

    This implementation allows for detailed control over which groups can interact with each other.

    371-373:

    ❓ Verification inconclusive

    Consider validating the same-group collision assumption

    The current implementation assumes that colliders in the same group always collide with each other. Please verify this is the intended behavior, as some physics systems allow configuring whether objects in the same group can collide.

    🏁 Script executed:

    #!/bin/bash
# Check if there are any implementations that disable same-group collisions
rg --type ts "setColliderGroupCollision.*\s+\S+\s*,\s*\1\s*,\s*false"

    Length of output: 333

    Action Required: Verify Configurability of Same-Group Collisions

    The current implementation in packages/physics-lite/src/LitePhysicsScene.ts (lines 371-373) assumes that colliders in the same group always collide. However, before finalizing this logic, please verify that there isn’t any mechanism (e.g., via a call like setColliderGroupCollision) that disables same-group collisions. The initial automated search using a regex with a backreference produced an error due to missing PCRE2 support, so we couldn’t conclusively determine whether such configuration exists.

    Next Steps:

    • Re-run the search using the PCRE2 flag (or verify manually) with a command like: 
      rg --pcre2 --type ts "setColliderGroupCollision.*\s+(\S+)\s*,\s*\1\s*,\s*false"
    • Verify across the codebase (or by manual inspection) that no part allows configuring same-group collisions.
    • Confirm whether the assumption is acceptable for your physics system or if adjustments are needed.

    coderabbitai[bot]
    coderabbitai bot reviewed Apr 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 1

    🔭 Outside diff range comments (1)
    packages/core/src/physics/PhysicsScene.ts (1)

    365-365: ⚠️ Potential issue

    Fix undefined reference to componentsManager

    There's a reference to an undefined variable componentsManager which is causing a build failure.

    This needs to be corrected by either:

    1. Importing the componentsManager, or
    2. Using the fully qualified path to access it
    - componentsManager.callScriptOnPhysicsUpdate();
+ this._scene.componentsManager.callScriptOnPhysicsUpdate();

    Verify that componentsManager is accessible through the scene object or import it properly.

    🧰 Tools
    🪛 GitHub Actions: CI

    [error] 365-365: Cannot find name 'componentsManager'.

    🧹 Nitpick comments (1)
    packages/core/src/physics/PhysicsScene.ts (1)

    234-242: Implementation of collision group management looks good

    The new setColliderGroupCollision method provides a clean interface for controlling whether specific collision groups can interact with each other. The implementation correctly delegates to the native physics engine.

    Consider adding parameter validation and error handling to provide better feedback if invalid parameters are passed:

     setColliderGroupCollision(group1: number, group2: number, collide: boolean): void {
+  if (typeof group1 !== 'number' || typeof group2 !== 'number') {
+    throw new Error('Collision groups must be numbers');
+  }
   PhysicsScene._nativePhysics.setColliderGroupCollision(group1, group2, collide);
 }
    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 2c53eef and fc9e0ac.

    📒 Files selected for processing (1)
    • packages/core/src/physics/PhysicsScene.ts (1 hunks)
    🧰 Additional context used
    🪛 GitHub Actions: CI
    packages/core/src/physics/PhysicsScene.ts

    [error] 365-365: Cannot find name 'componentsManager'.

    @luzhuang luzhuang changed the title Support collision group Support collision group and fix the position of characterController synchronization late issue Apr 14, 2025
    coderabbitai[bot]
    coderabbitai bot reviewed Apr 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 1

    ♻️ Duplicate comments (2)
    packages/core/src/physics/Collider.ts (2)

    29-29: Uninitialized _collisionGroup property.

    When _syncCollisionGroupByLayer is true and the entity uses Layer.Nothing (0), _collisionGroup remains undefined. This may cause _nativeCollider.setCollisionGroup(this.collisionGroup) to receive undefined.

    - protected _collisionGroup: number;
+ protected _collisionGroup: number = 0;

    This ensures a safe default to avoid undefined usage.

    183-183: Incorrect parameter type in _syncNative method.

    The _setCollisionGroupByLayer method expects a Layer parameter, but you're passing this.collisionGroup which is a number.

    - this._setCollisionGroupByLayer(this.collisionGroup);
+ this._setCollisionGroupByLayer(this.entity.layer);
    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between fc9e0ac and 5291a5f.

    📒 Files selected for processing (2)
    • packages/core/src/physics/Collider.ts (6 hunks)
    • packages/core/src/physics/PhysicsScene.ts (1 hunks)
    🚧 Files skipped from review as they are similar to previous changes (1)
    • packages/core/src/physics/PhysicsScene.ts
    ⏰ Context from checks skipped due to timeout of 90000ms (1)
    • GitHub Check: e2e (22.x)
    🔇 Additional comments (6)
    packages/core/src/physics/Collider.ts (6)

    39-53: LGTM - Well-implemented synchronization mechanism.

    The getter and setter for syncCollisionGroupByLayer properly handle the registration and unregistration of entity modification listeners.

    55-74: LGTM - Well-implemented collision group setter with proper validation.

    The collision group setter correctly validates the range (0-31) and provides clear warning when attempting to manually set the collision group while synchronization is enabled.

    175-177: LGTM - Proper collision group update on shape changes.

    Resetting the collision group when shapes change ensures the native collider correctly reflects the current configuration.

    233-238: LGTM - Well-implemented entity modification listener.

    The method correctly uses the EntityModifyFlags.Layer flag to detect layer changes and updates the collision group accordingly.

    240-242: LGTM - Clean abstraction for setting the collision group.

    Extracting this logic into a separate method improves code organization and maintainability.

    212-231:

    ✅ Verification successful

    Collision Group Calculation Logic – Improve Documentation and Type Safety.

    The collision group calculation has several considerations:

    1. Using 32 as a special value for Layer.Nothing while the setter validates 0-31 creates a potential inconsistency
    2. The Math.log2 calculation assumes the layer is a perfect power of 2 (which is checked), but should ensure an integer result
    protected _setCollisionGroupByLayer(layer: Layer): void {
  if (layer === Layer.Nothing) {
-    // Only support 0-31, 32 will skip collision check
+    // Using special value 32 to indicate "no collision" for Layer.Nothing
+    // Regular collision groups are limited to 0-31
    this._collisionGroup = 32;
    return;
  }

  if ((layer & (layer - 1)) !== 0) {
    console.warn(
      "Combined layers are not supported for collision groups, you can set syncCollisionGroupByLayer to false, and set collisionGroup manually"
    );
    return;
  }

  let newGroup = Math.log2(layer);
+  // Ensure we have an integer value (should always be true for powers of 2)
+  newGroup = Math.floor(newGroup);
  if (newGroup !== this._collisionGroup) {
    this._collisionGroup = newGroup;
    this._setCollisionGroup();
  }
}

    Let's verify this is consistent with how collision groups are handled throughout the engine:

    🏁 Script executed:

    #!/bin/bash
# Find all uses of collision groups and Layer.Nothing handling
rg -A 3 -B 3 "setCollisionGroup|_collisionGroup|Layer\.Nothing" --type ts

    Length of output: 12542

    Collision Group Calculation: Enhanced Documentation & Type Safety Improvements

    The updated diff addresses two key points:

    • Special Value Clarification: It now explicitly documents that a collision group of 32 indicates "no collision" for entities on Layer.Nothing and that valid groups are 0–31.
    • Integer Assurance: It forces the result of Math.log2(layer) to an integer using Math.floor, ensuring extra type safety even though the power-of-2 check should guarantee an integer.

    The diff snippet reflecting these changes is:

    protected _setCollisionGroupByLayer(layer: Layer): void {
  if (layer === Layer.Nothing) {
-    // Only support 0-31, 32 will skip collision check
+    // Using special value 32 to indicate "no collision" for Layer.Nothing
+    // Regular collision groups are limited to 0-31
    this._collisionGroup = 32;
    return;
  }

  if ((layer & (layer - 1)) !== 0) {
    console.warn(
      "Combined layers are not supported for collision groups, you can set syncCollisionGroupByLayer to false, and set collisionGroup manually"
    );
    return;
  }

  let newGroup = Math.log2(layer);
+  // Ensure we have an integer value (should always be true for powers of 2)
+  newGroup = Math.floor(newGroup);
  if (newGroup !== this._collisionGroup) {
    this._collisionGroup = newGroup;
    this._setCollisionGroup();
  }
}

    Our grep verification confirms that this pattern is consistently used elsewhere in the engine, and these changes improve clarity and safety without impacting other collision handling mechanisms.

    coderabbitai[bot]
    coderabbitai bot reviewed Apr 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 2

    ♻️ Duplicate comments (2)
    packages/core/src/physics/Collider.ts (2)

    29-30: Initialize _collisionGroup with a default value.

    The _collisionGroup property is declared without an initial value. When _syncCollisionGroupByLayer is true but the entity's layer handling is delayed, _collisionGroup could remain undefined. This may cause unexpected behavior when accessing this.collisionGroup.

    - protected _collisionGroup: number;
+ protected _collisionGroup: number = 0;

    79-84: Initialize collision group in the constructor.

    The constructor registers the entity modification listener but doesn't initialize the collision group based on the entity's layer. This could lead to an undefined collision group until the entity is modified.

    constructor(entity: Entity) {
  super(entity);
  this._updateFlag = entity.registerWorldChangeFlag();
  this._onEntityModified = this._onEntityModified.bind(this);
  entity._registerModifyListener(this._onEntityModified);
+ this._setCollisionGroupByLayer(entity.layer);
}
    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 5291a5f and 092f26f.

    📒 Files selected for processing (1)
    • packages/core/src/physics/Collider.ts (6 hunks)
    🧰 Additional context used
    🧬 Code Graph Analysis (1)
    packages/core/src/physics/Collider.ts (2)
    packages/core/src/Entity.ts (3)
    • Entity (21-847)
    • layer (254-256)
    • layer (258-263)
    packages/core/src/index.ts (2)
    • Entity (11-11)
    • EntityModifyFlags (42-42)
    ⏰ Context from checks skipped due to timeout of 90000ms (1)
    • GitHub Check: e2e (22.x)
    🔇 Additional comments (4)
    packages/core/src/physics/Collider.ts (4)

    39-53: Well-implemented synchronization mechanism.

    The getter and setter for syncCollisionGroupByLayer are well implemented, properly managing the entity modification listener registration and collision group synchronization.

    55-74: Good validation and warning implementation.

    The collisionGroup setter includes appropriate validation for the value range and provides a helpful warning when attempting to set the value while synchronization is enabled.

    199-199: Good cleanup practice.

    Properly unregistering the entity modification listener in the _onDestroy method prevents potential memory leaks.

    233-238: Good use of entity modification flags.

    The _onEntityModified method correctly uses bit flags to check if the layer was modified, and appropriately updates the collision group in response.

    @luzhuang luzhuang changed the base branch from main to dev/1.5 May 13, 2025 03:42
    coderabbitai[bot]
    coderabbitai bot reviewed May 13, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 0

    🧹 Nitpick comments (4)
    tests/src/core/physics/CharacterController.test.ts (2)

    327-377: Well-structured test case for collision group functionality.

    This test appropriately validates that the CharacterController respects collision group settings by:

    1. Creating entities on different layers
    2. Verifying trigger detection works by default
    3. Disabling collision between groups
    4. Confirming trigger events no longer occur

    The test logic is clear and effectively validates both the positive and negative conditions.

    However, consider translating the Chinese comment on line 375 ("恢复默认设置" - "restore default settings") to English for consistency.

    379-433: Comprehensive test for manual collision group configuration.

    This test effectively validates that manually setting a custom collision group (10) on the CharacterController works correctly. The test follows good practices by:

    1. Testing the default behavior first
    2. Applying the custom collision group
    3. Verifying that disabling collision between groups prevents trigger events
    4. Resetting settings to avoid affecting other tests

    As with the previous test, consider translating the Chinese comment on line 432 to English for consistency.

    tests/src/core/physics/Collider.test.ts (2)

    385-486: Comprehensive test suite for LitePhysics collision groups.

    This test suite effectively validates collision group functionality in the LitePhysics engine, including:

    1. Default collision detection between different layers
    2. Disabling collision between groups and verifying no collision occurs
    3. Manual collision group assignment for static trigger colliders
    4. Proper cleanup of test entities after each test

    The implementation is thorough and tests both standard and edge cases.

    Consider translating the Chinese comment on line 476 ("恢复默认设置" - "restore default settings") to English for consistency.

    488-544: Effective validation of PhysXPhysics collision groups.

    This test suite verifies that the collision group functionality works consistently in the PhysXPhysics engine. The tests appropriately:

    1. Create entities on different layers
    2. Verify default collision behavior
    3. Test that disabling collisions between groups prevents detection
    4. Clean up test entities after each test

    Consider adding a test for manual collision group setting with PhysXPhysics similar to the one implemented for LitePhysics (lines 438-478) for more complete coverage.

    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between c0c6faf and 2d6e210.

    ⛔ Files ignored due to path filters (2)
    • e2e/fixtures/originImage/Physics_litePhysics-collision-group.jpg is excluded by !**/*.jpg
    • e2e/fixtures/originImage/Physics_physx-collision-group.jpg is excluded by !**/*.jpg
    📒 Files selected for processing (7)
    • e2e/config.ts (1 hunks)
    • packages/core/src/physics/CharacterController.ts (0 hunks)
    • packages/core/src/physics/Collider.ts (5 hunks)
    • packages/core/src/physics/DynamicCollider.ts (1 hunks)
    • packages/core/src/physics/PhysicsScene.ts (1 hunks)
    • tests/src/core/physics/CharacterController.test.ts (2 hunks)
    • tests/src/core/physics/Collider.test.ts (3 hunks)
    💤 Files with no reviewable changes (1)
    • packages/core/src/physics/CharacterController.ts
    10000 🚧 Files skipped from review as they are similar to previous changes (4)
    • packages/core/src/physics/PhysicsScene.ts
    • packages/core/src/physics/DynamicCollider.ts
    • e2e/config.ts
    • packages/core/src/physics/Collider.ts
    ⏰ Context from checks skipped due to timeout of 90000ms (3)
    • GitHub Check: codecov
    • GitHub Check: build (22.x, windows-latest)
    • GitHub Check: e2e (22.x)
    🔇 Additional comments (3)
    tests/src/core/physics/CharacterController.test.ts (1)

    12-13: Appropriate imports added for new functionality.

    The addition of ControllerCollisionFlag and Layer imports supports the new collision group tests.

    tests/src/core/physics/Collider.test.ts (2)

    3-16: Appropriate imports added for new tests.

    The imports of Collision, Layer, LitePhysics, and afterEach from vitest support the new collision group testing functionality.

    44-58: Well-designed helper class for collision detection.

    CollisionDetectionScript is a simple and effective utility class for tracking collision and trigger events. It follows a good pattern with:

    • Boolean flag to track collision state
    • Event handlers that set the flag
    • Reset method to clear state between tests

    This approach simplifies the test cases by encapsulating detection logic.

    GuoLei1990
    GuoLei1990 reviewed May 13, 2025
    View reviewed changes
    coderabbitai[bot]
    coderabbitai bot reviewed May 13, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 2

    🧹 Nitpick comments (4)
    e2e/case/physx-collision-group.ts (4)

    26-38: Consider extracting duplicate color-changing logic to a separate method.

    In the CheckScript class, both onTriggerEnter and onContactEnter methods have identical color-changing logic. Extract this to a separate method to improve maintainability and avoid code duplication.

    class CheckScript extends Script {
+  private changeColorToGreen(): void {
+    (this.entity.getComponent(MeshRenderer).getMaterial() as PBRMaterial).baseColor.set(0, 1, 0, 1);
+  }
+
  onTriggerEnter(other: ColliderShape) {
    console.log("onTriggerEnter", other);
    // Change color to green when collision occurs
-    (this.entity.getComponent(MeshRenderer).getMaterial() as PBRMaterial).baseColor.set(0, 1, 0, 1);
+    this.changeColorToGreen();
  }

  onContactEnter(other: ColliderShape) {
    console.log("onContactEnter", other);
    // Change color to green when collision occurs
-    (this.entity.getComponent(MeshRenderer).getMaterial() as PBRMaterial).baseColor.set(0, 1, 0, 1);
+    this.changeColorToGreen();
  }
}

    86-86: Translate Chinese comments for better code accessibility.

    Several comments are written in Chinese. For international codebases, translate these comments to English to ensure all developers can understand them.

    - // 调整相机位置以便更好地观察穿透效果
+ // Adjust camera position for better observation of penetration effect

- // 创建立方体作为地面
+ // Create a cube as the ground

- // 设置半透明以便能看到穿透的球体
+ // Set semi-transparency to see the penetrating sphere

- // 创建可以碰撞的红色球体
+ // Create a red sphere that can collide

- // 创建可以穿透的蓝色球体
+ // Create a blue sphere that can penetrate

    Also applies to: 96-96, 106-106, 122-122, 125-125

    67-67: Use setter method for consistency with API design.

    Instead of directly assigning to the collisionGroup property, consider using a setter method like setCollisionGroup() for better consistency with the method-based API design pattern used in other parts of the codebase.

    - sphereCollider.collisionGroup = collisionGroup;
+ sphereCollider.setCollisionGroup(collisionGroup);

- groundCollider.collisionGroup = 3;
+ groundCollider.setCollisionGroup(3);

    This would be more consistent with the setColliderGroupCollision method used on line 128.

    Also applies to: 120-120

    1-133: Consider adding cleanup and validation.

    This test demonstrates the functionality well but lacks proper cleanup and validation. Consider adding:

    1. Resources cleanup when the test completes
    2. Actual test assertions to validate collision behavior beyond visual confirmation
    3. Documentation explaining how to interpret the test results

    This would make the test more robust and easier to maintain.

    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 2d6e210 and 9156ff2.

    📒 Files selected for processing (2)
    • e2e/case/litePhysics-collision-group.ts (1 hunks)
    • e2e/case/physx-collision-group.ts (1 hunks)
    🚧 Files skipped from review as they are similar to previous changes (1)
    • e2e/case/litePhysics-collision-group.ts
    ⏰ Context from checks skipped due to timeout of 90000ms (3)
    • GitHub Check: build (22.x, windows-latest)
    • GitHub Check: e2e (22.x)
    • GitHub Check: codecov
    🔇 Additional comments (2)
    e2e/case/physx-collision-group.ts (2)

    62-65: Good physics material configuration.

    The sphere collider shape is well configured with appropriate bounciness, which will create realistic physical interactions.

    127-128: Good implementation of collision group functionality.

    The code properly demonstrates the collision group functionality by disabling collisions between groups 2 and 3, which will allow the blue sphere to pass through the ground while the red sphere collides with it.

    GuoLei1990
    GuoLei1990 reviewed May 13, 2025
    View reviewed changes
    GuoLei1990
    GuoLei1990 reviewed May 13, 2025
    View reviewed changes
    GuoLei1990
    GuoLei1990 reviewed May 13, 2025
    View reviewed changes
    packages/physics-physx/src/PhysXCharacterController.ts Outdated
    @@ -136,6 +147,7 @@ export class PhysXCharacterController implements ICharacterController {

    this._pxController = pxManager._getControllerManager().createController(desc);
    this._pxController.setUUID(shape._id);
    this._pxController.setPosition(this._worldPosition);
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Additional Bug？

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    yes

    Copy link
    Member

    Choose a reason for hiding this comment

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

    Why not consider shape scale?

    coderabbitai[bot]
    coderabbitai bot reviewed May 13, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 3

    🧹 Nitpick comments (3)
    packages/core/src/physics/Collider.ts (1)

    10-11: Potential unused import.

    The EntityModifyFlags import appears to be unused in this file. Consider removing it to maintain clean imports.

    -import { EntityModifyFlags } from "../enums/EntityModifyFlags";
    packages/physics-lite/src/LitePhysics.ts (2)

    176-183: Document default collision behavior.

    The method uses the nullish coalescing operator (??) to default to true if the matrix entry doesn't exist, meaning layers collide by default. This behavior should be documented.

    _getColliderLayerCollision(layer1: number, layer2: number): boolean {
  const index = this._getColliderLayerIndex(layer1, layer2);
  if (index > -1) {
-    return this._layerCollisionMatrix[index] ?? true;
+    // Layers collide by default if not explicitly disabled
+    return this._layerCollisionMatrix[index] ?? true;
  }
  // If either layer is Layer.Nothing, they cant collide
  return false;
}

    185-196: Use Layer.Nothing instead of hard-coded value.

    The method uses a hard-coded value of 32 which appears to represent Layer.Nothing. For clarity and maintainability, consider using the actual enum value.

    private _getColliderLayerIndex(layer1: number, layer2: number): number {
-  if (layer1 === 32 || layer2 === 32) {
+  if (layer1 === Layer.Nothing || layer2 === Layer.Nothing) {
    return -1;
  }

  const min = Math.min(layer1, layer2);
  const max = Math.max(layer1, layer2);

  // Calculate a unique index for the layer pair using the triangular number formula
  // This ensures that each layer combination maps to a unique index in the collision matrix
  return (max * (max + 1)) / 2 + min;
}
    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 9156ff2 and f190cb6.

    📒 Files selected for processing (15)
    • e2e/case/litePhysics-collision-group.ts (1 hunks)
    • e2e/case/physx-collision-group.ts (1 hunks)
    • packages/core/src/physics/Collider.ts (5 hunks)
    • packages/core/src/physics/PhysicsScene.ts (2 hunks)
    • packages/design/src/physics/ICollider.ts (1 hunks)
    • packages/design/src/physics/IPhysics.ts (1 hunks)
    • packages/physics-lite/src/LiteCollider.ts (2 hunks)
    • packages/physics-lite/src/LitePhysics.ts (5 hunks)
    • packages/physics-lite/src/LitePhysicsScene.ts (5 hunks)
    • packages/physics-physx/src/PhysXCharacterController.ts (2 hunks)
    • packages/physics-physx/src/PhysXCollider.ts (1 hunks)
    • packages/physics-physx/src/PhysXPhysics.ts (5 hunks)
    • tests/src/core/physics/CharacterController.test.ts (2 hunks)
    • tests/src/core/physics/Collider.test.ts (3 hunks)
    • tests/src/core/physics/PhysicsManager.test.ts (5 hunks)
    🚧 Files skipped from review as they are similar to previous changes (13)
    • packages/design/src/physics/ICollider.ts
    • packages/design/src/physics/IPhysics.ts
    • packages/physics-physx/src/PhysXCollider.ts
    • packages/physics-lite/src/LitePhysicsScene.ts
    • packages/physics-physx/src/PhysXCharacterController.ts
    • packages/physics-physx/src/PhysXPhysics.ts
    • tests/src/core/physics/CharacterController.test.ts
    • packages/core/src/physics/PhysicsScene.ts
    • tests/src/core/physics/PhysicsManager.test.ts
    • e2e/case/physx-collision-group.ts
    • packages/physics-lite/src/LiteCollider.ts
    • tests/src/core/physics/Collider.test.ts
    • e2e/case/litePhysics-collision-group.ts
    🧰 Additional context used
    🧬 Code Graph Analysis (1)
    packages/physics-lite/src/LitePhysics.ts (2)
    packages/physics-lite/src/LiteStaticCollider.ts (1)
    • LiteStaticCollider (10-23)
    packages/physics-lite/src/LiteDynamicCollider.ts (1)
    • LiteDynamicCollider (9-207)
    ⏰ Context from checks skipped due to timeout of 90000ms (2)
    • GitHub Check: e2e (22.x)
    • GitHub Check: codecov
    🔇 Additional comments (8)
    packages/core/src/physics/Collider.ts (3)

    38-43: LGTM! The collision layer getter implementation looks good.

    The getter for collision layer correctly returns the protected _collisionLayer property.

    152-154: Good addition of collision layer synchronization.

    The _handleShapesChanged method now correctly updates the collision layer when shapes change, which ensures collision settings remain consistent.

    160-160: Good addition of collision layer synchronization.

    The _syncNative method now correctly updates the collision layer during native synchronization, which ensures collision settings remain consistent.

    packages/physics-lite/src/LitePhysics.ts (5)

    29-29: Properly initialized collision matrix.

    Good addition of the collision matrix to track layer interactions.

    58-58: Passing physics instance to physics scene.

    Good update to pass the LitePhysics instance to the LitePhysicsScene constructor, which will enable the scene to access collision layer information.

    72-72: Passing physics instance to static collider.

    Good update to pass the LitePhysics instance to the LiteStaticCollider constructor, which will enable the collider to access collision layer information.

    79-79: Passing physics instance to dynamic collider.

    Good update to pass the LitePhysics instance to the LiteDynamicCollider constructor, which will enable the collider to access collision layer information.

    159-161: LGTM! Implementation of setColliderLayer.

    The method correctly sets the collider's collision layer.

    GuoLei1990
    GuoLei1990 reviewed May 13, 2025
    View reviewed changes
    packages/physics-physx/src/PhysXCharacterController.ts Outdated
    @@ -136,6 +147,7 @@ export class PhysXCharacterController implements ICharacterController {

    this._pxController = pxManager._getControllerManager().createController(desc);
    this._pxController.setUUID(shape._id);
    this._pxController.setPosition(this._worldPosition);
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Why not consider shape scale?

    tests/vitest.config.ts
    @@ -1,6 +1,9 @@
    import { defineProject } from "vitest/config";

    export default defineProject({
    server: {
    port: 51204
    },
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Why add?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    The single test conflicts with e2e

    coderabbitai[bot]
    coderabbitai bot reviewed May 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 1

    ♻️ Duplicate comments (1)
    packages/core/src/physics/PhysicsScene.ts (1)

    234-248: Add tests for collision group functionality

    The PR needs tests to verify the collision group functionality works as expected.

    Since you've added a new public API for collision group management, please add tests that verify:

    1. Collision groups can be configured correctly
    2. Collisions occur only between groups set to collide
    3. Collisions don't occur between groups set not to collide

    These tests will ensure the feature works as expected across both physics backends.

    🧹 Nitpick comments (1)
    packages/core/src/physics/PhysicsScene.ts (1)

    240-248: Consider edge case validation for layer parameters

    The current implementation assumes layers are powers of 2, but doesn't validate the actual values.

    Consider adding additional validation to ensure the layer values are within the expected range. For example:

    setColliderLayerCollision(layer1: Layer, layer2: Layer, isCollide: boolean): void {
  const index1 = Math.log2(layer1);
  const index2 = Math.log2(layer2);
  if (!Number.isInteger(index1) || !Number.isInteger(index2)) {
    throw new Error("Collision layer must be a single layer (Layer.Layer0 to Layer.Layer31)");
  }
+ if (index1 < 0 || index1 > 31 || index2 < 0 || index2 > 31) {
+   throw new Error("Layer index must be between 0 and 31");
+ }

  PhysicsScene._nativePhysics.setColliderLayerCollision(index1, index2, isCollide);
}
    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between c71e392 and 8fcfcff.

    📒 Files selected for processing (5)
    • packages/core/src/physics/Collider.ts (5 hunks)
    • packages/core/src/physics/PhysicsScene.ts (1 hunks)
    • packages/design/src/physics/IPhysics.ts (1 hunks)
    • packages/physics-physx/src/PhysXCharacterController.ts (3 hunks)
    • packages/physics-physx/src/PhysXPhysics.ts (5 hunks)
    🚧 Files skipped from review as they are similar to previous changes (4)
    • packages/design/src/physics/IPhysics.ts
    • packages/physics-physx/src/PhysXPhysics.ts
    • packages/physics-physx/src/PhysXCharacterController.ts
    • packages/core/src/physics/Collider.ts
    ⏰ Context from checks skipped due to timeout of 90000ms (2)
    • GitHub Check: e2e (22.x)
    • GitHub Check: codecov

    packages/core/src/physics/PhysicsScene.ts
    Comment on lines +234 to +248
    /**
    * Set whether two colliders can collide with each other.
    * @param layer1 - The first collision layer
    * @param layer2 - The second collision layer
    * @param isCollide - Whether the colliders should collide
    */
    setColliderLayerCollision(layer1: Layer, layer2: Layer, isCollide: boolean): void {
    const index1 = Math.log2(layer1);
    const index2 = Math.log2(layer2);
    if (!Number.isInteger(index1) || !Number.isInteger(index1)) {
    throw new Error("Collision layer must be a single layer (Layer.Layer0 to Layer.Layer31)");
    }

    PhysicsScene._nativePhysics.setColliderLayerCollision(index1, index2, isCollide);
    }
    Copy link

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue

    Fix the bug in the conditional check

    There's a bug in the validation condition where you check the same variable twice instead of checking both indices.

    - if (!Number.isInteger(index1) || !Number.isInteger(index1)) {
+ if (!Number.isInteger(index1) || !Number.isInteger(index2)) {
    throw new Error("Collision layer must be a single layer (Layer.Layer0 to Layer.Layer31)");
  }

    The condition should check both index1 and index2 to ensure they're both valid integer indices.

    📝 Committable suggestion

    ‼️ IMPORTANT
    Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    Suggested change
    /**
    * Set whether two colliders can collide with each other.
    * @param layer1 - The first collision layer
    * @param layer2 - The second collision layer
    * @param isCollide - Whether the colliders should collide
    */
    setColliderLayerCollision(layer1: Layer, layer2: Layer, isCollide: boolean): void {
    const index1 = Math.log2(layer1);
    const index2 = Math.log2(layer2);
    if (!Number.isInteger(index1) || !Number.isInteger(index1)) {
    throw new Error("Collision layer must be a single layer (Layer.Layer0 to Layer.Layer31)");
    }
    PhysicsScene._nativePhysics.setColliderLayerCollision(index1, index2, isCollide);
    }
    /**
    * Set whether two colliders can collide with each other.
    * @param layer1 - The first collision layer
    * @param layer2 - The second collision layer
    * @param isCollide - Whether the colliders should collide
    */
    setColliderLayerCollision(layer1: Layer, layer2: Layer, isCollide: boolean): void {
    const index1 = Math.log2(layer1);
    const index2 = Math.log2(layer2);
    - if (!Number.isInteger(index1) || !Number.isInteger(index1)) {
    + if (!Number.isInteger(index1) || !Number.isInteger(index2)) {
    throw new Error("Collision layer must be a single layer (Layer.Layer0 to Layer.Layer31)");
    }
    PhysicsScene._nativePhysics.setColliderLayerCollision(index1, index2, isCollide);
    }

    coderabbitai[bot]
    coderabbitai bot reviewed May 14, 2025
    View reviewed changes
    Copy link
    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 2

    🧹 Nitpick comments (3)
    tests/src/core/physics/CharacterController.test.ts (3)

    327-377: Good implementation of collision group test

    This test case effectively verifies that the collision group feature works correctly by:

    1. Setting up entities with different layers
    2. Testing that trigger events fire when layers can collide
    3. Disabling collision between layers and verifying no events occur
    4. Restoring default settings

    Consider translating the Chinese comment "恢复默认设置" (line 376) to English for consistency.

    379-433: Code duplication in collision group tests

    While this test correctly validates manual collision layer assignment for controllers, it duplicates much of the code from the previous test.

    Consider extracting common setup code (like the TriggerDetectionScript class and obstacle creation) into helper methods to reduce duplication between tests:

    + function createTriggerObstacle(parent: Entity, position: Vector3): {
+   entity: Entity,
+   script: TriggerDetectionScript
+ } {
+   const obstacleEntity = parent.createChild("obstacle");
+   obstacleEntity.transform.position = position;
+
+   const obstacleCollider = obstacleEntity.addComponent(StaticCollider);
+   const triggerShape = new BoxColliderShape();
+   triggerShape.size = new Vector3(1, 1, 1);
+   triggerShape.isTrigger = true;
+   obstacleCollider.addShape(triggerShape);
+
+   const triggerScript = obstacleEntity.addComponent(TriggerDetectionScript);
+   
+   return { entity: obstacleEntity, script: triggerScript };
+ }

    Also, consider translating the Chinese comment "恢复默认设置" (line 432) to English for consistency.

    339-349: Implement TriggerDetectionScript class once and reuse

    This script class is duplicated in both collision group tests.

    Define this script class once at the module level (outside the test cases) to avoid duplication.

    📜 Review details

    Configuration used: CodeRabbit UI
    Review profile: CHILL
    Plan: Pro

    📥 Commits

    Reviewing files that changed from the base of the PR and between 6347462 and f899d28.

    📒 Files selected for processing (8)
    • packages/core/src/physics/PhysicsScene.ts (1 hunks)
    • packages/core/src/physics/shape/ColliderShape.ts (1 hunks)
    • packages/design/src/physics/IPhysics.ts (1 hunks)
    • packages/physics-lite/src/LitePhysics.ts (5 hunks)
    • packages/physics-lite/src/LitePhysicsScene.ts (5 hunks)
    • packages/physics-physx/src/PhysXPhysics.ts (5 hunks)
    • tests/src/core/physics/CharacterController.test.ts (2 hunks)
    • tests/src/core/physics/PhysicsManager.test.ts (5 hunks)
    🚧 Files skipped from review as they are similar to previous changes (7)
    • packages/design/src/physics/IPhysics.ts
    • packages/physics-lite/src/LitePhysicsScene.ts
    • packages/physics-physx/src/PhysXPhysics.ts
    • tests/src/core/physics/PhysicsManager.test.ts
    • packages/core/src/physics/PhysicsScene.ts
    • packages/core/src/physics/shape/ColliderShape.ts
    • packages/physics-lite/src/LitePhysics.ts
    🧰 Additional context used
    🧬 Code Graph Analysis (1)
    tests/src/core/physics/CharacterController.test.ts (3)
    packages/core/src/physics/CharacterController.ts (1)
    • CharacterController (13-172)
    packages/core/src/physics/StaticCollider.ts (1)
    • StaticCollider (9-21)
    packages/core/src/physics/shape/BoxColliderShape.ts (1)
    • BoxColliderShape (10-48)
    ⏰ Context from checks skipped due to timeout of 90000ms (2)
    • GitHub Check: e2e (22.x)
    • GitHub Check: codecov
    🔇 Additional comments (1)
    tests/src/core/physics/CharacterController.test.ts (1)

    12-13: Adding the missing ControllerCollisionFlag and Layer imports

    Good addition of the necessary imports for the new test cases.

    tests/src/core/physics/CharacterController.test.ts
    Comment on lines +391 to +401
    class TriggerDetectionScript extends Script {
    triggerEntered = false;

    onTriggerEnter() {
    this.triggerEntered = true;
    }

    reset() {
    this.triggerEntered = false;
    }
    }
    Copy link

    Choose a reason for hiding this comment

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

    🛠️ Refactor suggestion

    Duplicated TriggerDetectionScript implementation

    This class is duplicated from the previous test case.

    Move this class definition outside the test cases to eliminate code duplication:

    -  class TriggerDetectionScript extends Script {
-    triggerEntered = false;
-
-    onTriggerEnter() {
-      this.triggerEntered = true;
-    }
-
-    reset() {
-      this.triggerEntered = false;
-    }
-  }

    Committable suggestion skipped: line range outside the PR's diff.

    tests/src/core/physics/CharacterController.test.ts
    Comment on lines +435 to +445
    it("keep entity position when disabled", () => {
    roleEntity.transform.position = new Vector3(0, 0, 3);
    // @ts-ignore
    engine.sceneManager.activeScene.physics._update(1);
    const controller = roleEntity.getComponent(CharacterController);
    controller.enabled = false;
    controller.enabled = true;
    // @ts-ignore
    controller._syncWorldPositionFromPhysicalSpace();
    expect(roleEntity.transform.position.z).eq(3);
    });
    Copy link

    Choose a reason for hiding this comment

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

    💡 Verification agent

    🧩 Analysis chain

    Using internal API with @ts-ignore in test

    This test relies on accessing a private method _syncWorldPositionFromPhysicalSpace with a @ts-ignore comment to bypass TypeScript visibility rules.

    While the test correctly verifies that the entity position is preserved when the controller is disabled and re-enabled, using private methods makes the test more fragile if internal implementation details change. Is there a public API that could be used instead to test this behavior?

    🏁 Script executed:

    #!/bin/bash
# Search for public methods in CharacterController that might handle position syncing
rg -t ts "position|sync" --iglob "**/*haracterController*.ts" -A 5 -B 5

    Length of output: 28885

    Replace private sync call with public engine update

    Rather than calling the private _syncWorldPositionFromPhysicalSpace() method (and needing a @ts-ignore), you can trigger the exact same transform update via the public engine pipeline. After re-enabling the controller, just run:

    controller.enabled = false;
controller.enabled = true;
engine.update();  // runs physics._update + _onLateUpdate(), which calls _syncWorldPositionFromPhysicalSpace()
expect(roleEntity.transform.position.z).toBe(3);

    This removes the need for any internal API in your test and keeps it stable against future refactorings.

    @GuoLei1990 GuoLei1990 merged commit ea51cf3 into galacean:dev/1.5 May 14, 2025
    7 of 9 checks passed
    @github-project-automation github-project-automation bot moved this from In progress to Done in Physic May 14, 2025
    @coderabbitai coderabbitai bot mentioned this pull request Jul 10, 2025
    Open
    3 tasks
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

    @GuoLei1990 GuoLei1990 GuoLei1990 approved these changes

    Assignees

    @GuoLei1990 GuoLei1990

    Labels
    enhancement New feature or request physics Engine's physical system
    Projects
    Physic
    Status: Done
    Milestone
    1.5
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @luzhuang @GuoLei1990
    0