Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for static entities and portal-based teleportation, updates DTO definitions, and integrates a new StaticEntityManager to handle scene additions.
- Defined
StaticEntityDtoandPortalDto, and updatedSpaceMapData,PlayerDto, andAlienDtotypes. - Extended
KeyboardServiceto track player position, portals, and trigger teleport requests. - Added
loadStaticEntities, enhancedupdateSpacemap, and integratedStaticEntityManagerin the game component.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/game/types/SpaceMapData.ts | Added DTOs for static entities and portals, refined map types |
| src/app/game/services/keyboard.service.ts | Implemented J key teleport handling and portal tracking |
| src/app/game/services/hub.service.ts | Added requestTeleport to server request types |
| src/app/game/game.utils.ts | Imported static entities, added loadStaticEntities, updated map logic |
| src/app/game/game.component.ts | Initialized StaticEntityManager, wired teleport events, and portal updates |
| src/app/game/StaticEntityManager.ts | Created manager for static entities and portal meshes |
Comments suppressed due to low confidence (1)
src/app/game/StaticEntityManager.ts:81
- [nitpick] The method name suggests it returns only IDs, but it returns the full Map of meshes. Consider renaming to
getStaticEntitiesor returning an iterable of IDs.
public getStaticEntityIds(): Map<string, THREE.Mesh> {
| // Load players, aliens, and static entities for the new map | ||
| await loadPlayers(spaceMapData.mapObject.players, component); | ||
| await loadAliens(spaceMapData.mapObject.aliens, component); | ||
| await loadStaticEntities(spaceMapData.mapObject.staticEntities, component); |
There was a problem hiding this comment.
The loadStaticEntities function is declared to accept StaticEntityDto[] but is being passed an array that may include PortalDto items. Consider updating the signature to accept the union type (StaticEntityDto | PortalDto)[] or filter out portal entries before calling.
| await loadStaticEntities(spaceMapData.mapObject.staticEntities, component); | |
| const filteredStaticEntities = spaceMapData.mapObject.staticEntities.filter( | |
| (entity): entity is StaticEntityDto => 'staticEntitySpecificProperty' in entity | |
| ); | |
| await loadStaticEntities(filteredStaticEntities, component); |
| await loadMapEnvironment(component, spaceMapData); | ||
|
|
||
| // Load players and aliens for the new map | ||
| // Load players, aliens, and static entities for the new map |
There was a problem hiding this comment.
Initial map loading uses loadStaticEntities only and does not create portal meshes, so portals won’t be available until the map is updated. Consider invoking portal creation logic on the first load or reusing updateSpacemap flow.
| this.scene.add(mesh); | ||
| } | ||
|
|
||
| public createPortal(position: THREE.Vector3, destinationMap: string): THREE.Mesh { |
There was a problem hiding this comment.
Portals created here are not tracked in staticEntities, so removeAllStaticEntities or removeStaticEntity won't remove them. You may want to store portal meshes in the manager or provide a separate cleanup method.
| } | ||
| } | ||
|
|
||
| public async removeAllStaticEntities(): Promise<void> { |
There was a problem hiding this comment.
This method is marked async but contains no await expressions. You can remove async and return void for clarity.
| public async removeAllStaticEntities(): Promise<void> { | |
| public removeAllStaticEntities(): void { |
| private readonly TELEPORT_DISTANCE = 5; // Distance within which player can teleport | ||
|
|
||
| constructor(private hubService: HubService) { |
There was a problem hiding this comment.
[nitpick] The teleport distance is hardcoded as a magic number. Consider making this configurable (e.g., via constructor parameter or app settings) for greater flexibility.
| private readonly TELEPORT_DISTANCE = 5; // Distance within which player can teleport | |
| constructor(private hubService: HubService) { | |
| private readonly TELEPORT_DISTANCE: number; // Distance within which player can teleport | |
| constructor(private hubService: HubService, teleportDistance: number = 5) { | |
| this.TELEPORT_DISTANCE = teleportDistance; |
| console.log('Teleportation successful:', message); | ||
| }); | ||
|
|
||
| hubService.on('teleportFailed', (error: string) => { |
There was a problem hiding this comment.
You're registering teleport event listeners in ngOnInit but not removing them in ngOnDestroy. Add corresponding off calls to prevent potential memory leaks.
No description provided.