Skip to content

Conversation

@bnszky
Copy link
Member

@bnszky bnszky commented Dec 21, 2025

this is frontend implementations of crates, doors and buttons

@bnszky bnszky requested a review from 24bartixx December 21, 2025 10:39
@bnszky bnszky self-assigned this Dec 21, 2025
Copilot AI review requested due to automatic review settings December 21, 2025 10:39
@bnszky bnszky added the enhancement New feature or request label Dec 21, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements frontend support for interactive game mechanics: crates (movable objects), doors (open/close based on button state), and buttons (pressure plates). The implementation adds type definitions, Phaser entities/mechanics classes, and integrates them into the main game scene with real-time server synchronization.

Key Changes:

  • Added type definitions for Crate, Door, and Button with corresponding message interfaces for server updates
  • Created a base Mechanic class for stationary game objects (doors, buttons) and integrated Crate as a movable Entity
  • Integrated these mechanics into the main Phaser scene with message handlers for state synchronization

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/types/messages.tsx Extended MessageMapInfo with crates, doors, and buttons arrays; added MessageCratesUpdate and MessageDoorsAndButtonsUpdate interfaces for real-time updates
src/types/door.tsx New interface defining door properties including ID, color, position, and open state
src/types/crate.tsx New interface defining crate properties including position and numeric ID
src/types/button.tsx New interface defining button properties including ID, color, position, and pressed state
src/phaser/mechanics/mechanic.ts Base class for stationary mechanics (doors, buttons) extending Phaser.GameObjects.Container with texture management
src/phaser/mechanics/door.ts Door implementation with open/closed state management and texture switching
src/phaser/mechanics/button.ts Button implementation with pressed/released state management and texture switching
src/phaser/entities/crate.ts Crate implementation extending Entity for movable box mechanics
src/phaser/scenes/main.ts Integrated crates, doors, and buttons into the scene: added entity maps, preloaded textures, registered message handlers, and created initialization methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


export class Button extends Mechanic {
public readonly buttonId: string;
public readonly color: string;
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The color field in Button is stored but never used. The code always loads hardcoded green/red button textures regardless of the color value passed from the backend. Either implement color-based texture loading (e.g., 'button-{color}.png') or remove the unused color field from the interface and constructor.

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +31
public get id(): string {
return this.buttonId;
}

Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The id getter is defined but never used in the codebase. The Button instances are already accessed via their buttonId property. Consider removing it or documenting why it's needed for future use.

Suggested change
public get id(): string {
return this.buttonId;
}

Copilot uses AI. Check for mistakes.
Comment on lines 147 to 154
room.onMessage("cratesUpdate", (message: MessageCratesUpdate) => {
for (const crateUpdate of message.crates) {
const crate = this.crates.get(crateUpdate.crateId.toString());
if (crate !== undefined) {
crate.move(crateUpdate.direction);
}
}
});
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Inconsistent ID handling: the MessageCratesUpdate uses crateId as a number, but it's converted to a string when looking up in the crates Map (line 149). This creates a mismatch between the storage key (string from crateInfo.crateId.toString() in line 212) and potential lookup patterns. Consider standardizing on either number or string for crate IDs throughout the codebase.

Copilot uses AI. Check for mistakes.
Copy link
Member

@24bartixx 24bartixx Dec 21, 2025

Choose a reason for hiding this comment

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

Same here with ids (copilot comment)

}

export interface MessageDoorsAndButtonsUpdate {
doorsAndButtons: { doorId: string; buttonId: string; open: boolean }[];
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The message interface names an array field doorsAndButtons, but each element contains individual doorId and buttonId properties, suggesting a one-to-one pairing. This naming could be clearer. Consider renaming to doorButtonPairs or connections to better convey that each entry represents a door-button relationship, not separate collections.

Suggested change
doorsAndButtons: { doorId: string; buttonId: string; open: boolean }[];
doorButtonPairs: { doorId: string; buttonId: string; open: boolean }[];

Copilot uses AI. Check for mistakes.
Comment on lines 6 to 17
public readonly crateId: string;

constructor(
scene: Phaser.Scene,
x: number,
y: number,
crateId: number,
textureKey = "crate",
animator: SpriteAnimator | null = null,
) {
super(scene, x, y, textureKey, animator);
this.crateId = crateId.toString();
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Type inconsistency: the Crate constructor accepts crateId as a number parameter but immediately converts it to a string. The internal crateId field is typed as string, and the type definition in src/types/crate.tsx defines it as number. Consider accepting number and keeping it as number throughout, or accept string directly to avoid the unnecessary conversion.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

Here the copilot is actually right, try to keep it consistant

Comment on lines 23 to 26

public get id(): string {
return this.crateId;
}
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The id getter is defined but never used in the codebase. Since the crates are already accessed via their crateId property directly in main.ts, this getter adds no value. Consider removing it or ensure it's needed for future use.

Suggested change
public get id(): string {
return this.crateId;
}

Copilot uses AI. Check for mistakes.
Comment on lines 20 to 23
move(direction: Direction, ease = "Circular") {
super.move(direction, ease);
}

Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The move method override doesn't add any new functionality beyond calling super.move() with the same parameters. This wrapper method is unnecessary and can be removed. The parent class's move method can be called directly.

Suggested change
move(direction: Direction, ease = "Circular") {
super.move(direction, ease);
}

Copilot uses AI. Check for mistakes.
Copy link
Member

@24bartixx 24bartixx Dec 21, 2025

Choose a reason for hiding this comment

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

Copilot right

Comment on lines +28 to +30
public get id(): string {
return this.doorId;
}
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The id getter is defined but never used in the codebase. The Door instances are already accessed via their doorId property. Consider removing it or documenting why it's needed for future use.

Copilot uses AI. Check for mistakes.
Comment on lines +164 to +168
door.isOpen = element.open;
}

if (button !== undefined) {
button.isPressed = element.open;
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The message handler processes door and button updates together, but there's no validation that ensures both the doorId and buttonId actually exist before attempting to update them. If the backend sends an update with only a doorId or only a buttonId that exists, the undefined checks will silently skip the missing entity without logging. Consider adding a warning log when an expected door or button is not found to aid debugging.

Suggested change
door.isOpen = element.open;
}
if (button !== undefined) {
button.isPressed = element.open;
door.isOpen = element.open;
} else {
console.warn(
`doorsAndButtonsUpdate: door with id "${element.doorId}" not found when setting open=${element.open}`,
);
}
if (button !== undefined) {
button.isPressed = element.open;
} else {
console.warn(
`doorsAndButtonsUpdate: button with id "${element.buttonId}" not found when setting pressed=${element.open}`,
);

Copilot uses AI. Check for mistakes.

export class Door extends Mechanic {
public readonly doorId: string;
public readonly color: string;
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

The color field in Door and Button types is stored but never used. The code always loads hardcoded green textures ('door-green-open.png', 'button-green.png') regardless of the color value passed from the backend. Either implement color-based texture loading (e.g., 'door-{color}-open.png') or remove the unused color field from the interfaces and constructors.

Copilot uses AI. Check for mistakes.
Copy link
Member

@24bartixx 24bartixx left a comment

Choose a reason for hiding this comment

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

Grate job!
It works good
Just 3 small changes before mergin

As I think about we will have to do smoother player movement animation when the user holds an arror as it jumps unnaturally, but it's for another task

Comment on lines 6 to 17
public readonly crateId: string;

constructor(
scene: Phaser.Scene,
x: number,
y: number,
crateId: number,
textureKey = "crate",
animator: SpriteAnimator | null = null,
) {
super(scene, x, y, textureKey, animator);
this.crateId = crateId.toString();
Copy link
Member

Choose a reason for hiding this comment

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

Here the copilot is actually right, try to keep it consistant

Comment on lines 20 to 23
move(direction: Direction, ease = "Circular") {
super.move(direction, ease);
}

Copy link
Member

@24bartixx 24bartixx Dec 21, 2025

Choose a reason for hiding this comment

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

Copilot right

Comment on lines 147 to 154
room.onMessage("cratesUpdate", (message: MessageCratesUpdate) => {
for (const crateUpdate of message.crates) {
const crate = this.crates.get(crateUpdate.crateId.toString());
if (crate !== undefined) {
crate.move(crateUpdate.direction);
}
}
});
Copy link
Member

@24bartixx 24bartixx Dec 21, 2025

Choose a reason for hiding this comment

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

Same here with ids (copilot comment)

@24bartixx 24bartixx merged commit 639dff3 into main Dec 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants