Nextgen physics #257
Nextgen physics #257
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
/deploy |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-qozl2zdzp-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-460fn7r2l-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-piak1jfqs-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-2ppno71v6-zaro.vercel.app |
|
/deploy |
1 similar comment
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-qi4vytrgh-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-qs0cehyun-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-4vv9z846z-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-ope8e1sdz-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-mbai3hxol-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-k2iszdxn4-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-pf8vk89o6-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-1a7r7sci8-zaro.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(3 hunks)src/mineflayer/cameraShake.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Definitions (1)
src/mineflayer/cameraShake.ts (1)
src/customChannels.ts (1)
customEvents(6-18)
🪛 GitHub Check: build-and-deploy
src/mineflayer/cameraShake.ts
[failure] 4-4:
Cannot find name 'cameraShake'.
🪛 GitHub Actions: CI
src/mineflayer/cameraShake.ts
[error] 4-4: error TS2304: Cannot find name 'cameraShake'.
🔇 Additional comments (1)
src/mineflayer/cameraShake.ts (1)
3-10: Potential duplicate event listeners for 'hurtAnimation'.There are now two event listeners for the 'hurtAnimation' event - one at the top level (lines 3-5) and another within the 'mineflayerBotCreated' event (lines 7-10). This might cause the camera shake effect to be triggered twice for a single damage event.
If this is intentional, please add a comment explaining why both listeners are needed. Otherwise, consider unifying the implementations to avoid duplicated behavior.
🧰 Tools
🪛 GitHub Check: build-and-deploy
[failure] 4-4:
Cannot find name 'cameraShake'.🪛 GitHub Actions: CI
[error] 4-4: error TS2304: Cannot find name 'cameraShake'.
src/mineflayer/cameraShake.ts
Outdated
| customEvents.on('hurtAnimation', () => { | ||
| cameraShake.shakeFromDamage() | ||
| }) |
There was a problem hiding this comment.
Missing cameraShake definition causes build failure.
The static analysis and pipeline logs indicate a critical error: 'cameraShake' is not defined. This variable needs to be imported or defined before use.
To fix this issue, you need to either:
- Import cameraShake from its module:
import { getThreeJsRendererMethods } from 'renderer/viewer/three/threeJsMethods'
+import { cameraShake } from './path/to/cameraShake'
customEvents.on('hurtAnimation', () => {- Or if cameraShake is meant to be the same as what's used in the nested listener, use the ThreeJS renderer methods instead:
customEvents.on('hurtAnimation', () => {
- cameraShake.shakeFromDamage()
+ getThreeJsRendererMethods()?.shakeFromDamage()
})📝 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.
| customEvents.on('hurtAnimation', () => { | |
| cameraShake.shakeFromDamage() | |
| }) | |
| import { getThreeJsRendererMethods } from 'renderer/viewer/three/threeJsMethods' | |
| import { cameraShake } from './path/to/cameraShake' | |
| customEvents.on('hurtAnimation', () => { | |
| cameraShake.shakeFromDamage() | |
| }) |
| customEvents.on('hurtAnimation', () => { | |
| cameraShake.shakeFromDamage() | |
| }) | |
| customEvents.on('hurtAnimation', () => { | |
| getThreeJsRendererMethods()?.shakeFromDamage() | |
| }) |
🧰 Tools
🪛 GitHub Check: build-and-deploy
[failure] 4-4:
Cannot find name 'cameraShake'.
🪛 GitHub Actions: CI
[error] 4-4: error TS2304: Cannot find name 'cameraShake'.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-ne9lxoidi-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-llejtikrc-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-ja8t2kkga-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-9s87g88kq-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-l5fp3qujx-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/react/components/LibraryVersions.tsx (6)
2-6: Consider handling potential import failures.The component directly imports package.json files from dependencies. If any of these packages are missing or have a different structure than expected, it could lead to runtime errors. Consider adding error handling or fallback values.
10-14: Inconsistent version sourcing strategy.While
@nxg-org/mineflayer-physics-utilandminecraft-protocolversions are pulled directly from their package.json files,mineflayerversion is pulled from the project's package.json devDependencies. This inconsistency might cause confusion or maintenance issues.- 'mineflayer': packageJson.devDependencies['mineflayer'], + 'mineflayer': mineflayerPkg.version,Or if the current approach is intentional (based on the PR objective of referencing local files), consider adding a comment explaining the difference.
16-18: Remove or uncomment conditional rendering code.There's commented out code related to conditional rendering based on
gameLoadedstate. Either remove unused code or uncomment it if it's necessary for the component's functionality.
5-7: Remove unused imports.The
useSnapshothook is imported but not used since the related code is commented out. Remove unused imports to avoid unnecessary bundle size.-import { useSnapshot } from 'valtio' -import { miscUiState } from '../../globalState'
22-33: Improve accessibility of the component.The component has some potential accessibility issues:
pointerEvents: 'none'makes it non-interactivefontSize: '8px'is extremely small and may be difficult to read- The fixed positioning might overlap with other UI elements on smaller screens
Consider making these styles configurable or increasing the font size for better readability.
36-40: Add fallback for potentially undefined versions.The component doesn't handle the case where a version value might be undefined. Add a fallback to ensure the UI doesn't break in unexpected scenarios.
{Object.entries(versions).map(([lib, version]) => ( <div key={lib} style={{ marginTop: '5px' }}> - {lib}: {version} + {lib}: {version || 'unknown'} </div> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(4 hunks)src/react/components/LibraryVersions.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
|
Deployed to Vercel Preview: https://prismarine-hk01mk9js-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/controls.ts (1)
724-724: Direct property assignment for flying state.Setting the
flyingproperty directly simplifies the code by removing complex flying vector management that was previously implemented.Consider adding a type definition for the bot entity that includes the
flyingproperty instead of usingas anyto improve type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(4 hunks)src/controls.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Definitions (1)
src/controls.ts (3)
renderer/viewer/lib/basePlayerState.ts (1)
isFlying(80-82)src/mineflayer/playerState.ts (1)
isFlying(132-134)src/mineflayer/plugins/mouse.ts (1)
bot(31-38)
🔇 Additional comments (4)
src/controls.ts (4)
716-716: Improved flying check for simplicity.The function now directly checks the
flyingproperty of the bot's entity instead of using a more complex gravity-based check, making the code more straightforward.
728-728: Good defensive programming check.Adding this guard clause prevents unnecessary calls to end flying when the entity is not currently flying.
734-734: Consistent with the start flying implementation.This matches the approach used in
startFlying, maintaining consistency in how flying state is managed.
741-741:✅ Verification successful
Simplified fly toggle condition.
The condition now directly checks the
canFlyproperty instead of complex game mode checks, which aligns with the overall simplification of the flying mechanics.
🏁 Script executed:
#!/bin/bash # Check if canFly property is consistently used across the codebase rg -A 2 -B 2 "canFly" --type typescriptLength of output: 74
Below is a new verification script to search for all occurrences of “canFly” in the repository’s TypeScript files. This will allow you to manually verify that the simplified condition at
src/controls.ts:741is consistently used across the codebase:Simplified Fly Toggle Verification
- The condition at
src/controls.ts:741now directly checksbot.entity.canFly, reflecting the simplified logic without extra game mode checks.- Please run the above script and manually review its output to verify that similar simplifications using
canFlyare consistently applied elsewhere in the codebase.Once you’ve confirmed the output is as expected, you can approve this simplified change.
🏁 Script executed:
#!/bin/bash # Re-run search for occurrences of "canFly" in TypeScript files. rg -A 2 -B 2 "canFly" -g "*.ts"Length of output: 359
Simplified fly toggle condition verified
The condition in
src/controls.ts:741now directly checksbot.entity.canFly, and the search confirms that this pattern is consistently applied. No further changes are necessary; please proceed with the simplified implementation.
PR Type
enhancement, configuration changes
Description
Updated
mineflayerdependency to use a local file reference.Updated
@nxg-org/mineflayer-physics-utildependency to use a local file reference.Adjusted
package.jsonto reflect dependency changes for local development.Changes walkthrough 📝
package.json
Updated dependencies to local file referencespackage.json
mineflayerdependency to a local file reference.@nxg-org/mineflayer-physics-utildependency to a local filereference.
Summary by CodeRabbit