feat: add getClosestPlayer utility for TS/JS #74
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a getClosestPlayer utility function for both client-side and server-side TypeScript/JavaScript code, providing a convenient way to find the nearest player to given coordinates within a specified distance. The implementation follows the pattern of similar utilities in the codebase like getNearbyVehicles.
Key changes:
- Adds server-side
getClosestPlayerfunction with player exclusion capability - Adds client-side
getClosestPlayerfunction with option to include/exclude the local player - Exports the new utilities from their respective index files
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| package/server/resource/index.ts | Adds export for the new server-side getClosestPlayer module |
| package/server/resource/getClosestPlayer/index.ts | Implements server-side function to find closest player with optional player ID exclusion |
| package/client/resource/index.ts | Adds export for the new client-side getClosestPlayer module |
| package/client/resource/getClosestPlayer/index.ts | Implements client-side function to find closest player with optional self-inclusion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getClosestPlayer( | ||
| coords: Vector3, | ||
| maxDistance: number = 2.0, | ||
| includePlayer?: boolean |
There was a problem hiding this comment.
The parameter name "includePlayer" is ambiguous - it's unclear whether it means "include the current player" or "include players in general". Consider renaming to "includeSelf" or "includeLocalPlayer" to make it clear that this controls whether the local player is included in the search.
| export function getClosestPlayer( | ||
| coords: Vector3, | ||
| maxDistance: number = 2.0, | ||
| ignorePlayerId?: number | false | ||
| ): [number, number, Vector3] | undefined { |
There was a problem hiding this comment.
Inconsistent API design between client and server versions. The server version uses "ignorePlayerId?: number | false" while the client version uses "includePlayer?: boolean". For better consistency and maintainability, both versions should use similar parameter naming and types. Consider standardizing on a parameter like "excludePlayerId?: number" for server and "includeSelf?: boolean" for client.
| export * from './points'; | ||
| export * from './dui'; | ||
| export * from './addKeybind'; | ||
| export * from './getClosestPlayer' |
There was a problem hiding this comment.
Missing semicolon at the end of the export statement. All other export statements in this file end with semicolons for consistency.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const playerPed = GetPlayerPed(playerId); | ||
| const [x, y, z] = GetEntityCoords(playerPed, false); | ||
|
|
||
| const distance = coords.distance(new Vector3(x, y, z)); |
There was a problem hiding this comment.
Creating a new Vector3 object in each iteration of the loop is inefficient. Consider reusing a single Vector3 object or using Vector3.fromArray() method which appears to be available based on similar code in the codebase (see getNearbyVehicles function). This would reduce memory allocations and improve performance in loops with many players.
| const playerPed = GetPlayerPed(playerId); | ||
| const [x, y, z] = GetEntityCoords(playerPed, false); | ||
|
|
||
| const distance = coords.distance(new Vector3(x, y, z)); |
There was a problem hiding this comment.
Creating a new Vector3 object in each iteration of the loop is inefficient. Consider reusing a single Vector3 object or using Vector3.fromArray() method which appears to be available based on similar code in the codebase (see getNearbyVehicles function). This would reduce memory allocations and improve performance in loops with many players.
| for (let i = 0; i < players.length; i++) { | ||
| const playerId = players[i]; | ||
|
|
||
| if (!ignorePlayerId || playerId !== ignorePlayerId) { |
There was a problem hiding this comment.
The logic for ignorePlayerId is incorrect. When ignorePlayerId is false (a valid value according to the type), the condition "!ignorePlayerId" evaluates to true, causing the player to be included when they should be ignored. Additionally, when ignorePlayerId is undefined, the condition "playerId !== ignorePlayerId" will always be true, but the outer "!ignorePlayerId" will be false, excluding all players. The logic should be: "if (ignorePlayerId === undefined || ignorePlayerId === false || playerId !== ignorePlayerId)"
| export function getClosestPlayer( | ||
| coords: Vector3, | ||
| maxDistance: number = 2.0, | ||
| ignorePlayerId?: number | false |
There was a problem hiding this comment.
The type "number | false" for ignorePlayerId is confusing and non-idiomatic. Using false to mean "don't ignore any player" is unclear. Consider using "number | undefined" or "number | null" instead, which is more conventional in TypeScript. Alternatively, rename the parameter to something like "excludePlayerId" and make it optional without false as a valid value.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
| } | ||
|
|
||
| return closestCoords ? [closestId as number, closestPed as number, closestCoords] : undefined; |
There was a problem hiding this comment.
The return type uses type assertions "as number" which indicates a code smell. Since closestId and closestPed are only set when closestCoords is also set, they should be properly typed or the function logic should ensure they are never undefined when closestCoords exists. Consider initializing maxDistance to a separate variable to track the closest distance, or restructure the logic to avoid type assertions.
| } | ||
| } | ||
|
|
||
| return closestCoords ? [closestId as number, closestPed as number, closestCoords] : undefined; |
There was a problem hiding this comment.
The return type uses type assertions "as number" which indicates a code smell. Since closestId and closestPed are only set when closestCoords is also set, they should be properly typed or the function logic should ensure they are never undefined when closestCoords exists. Consider initializing maxDistance to a separate variable to track the closest distance, or restructure the logic to avoid type assertions.
1 participant
This PR adds the getClosestPlayer utility to the TypeScript/JavaScript package in order to hopefully make some lives easier.