-
Notifications
You must be signed in to change notification settings - Fork 97
feat: Support nameTagVisibility option of teams #373
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.
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
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThe changes introduce explicit management of player name tag visibility based on team settings. A new method handles name tag visibility updates according to team rules, and event listeners ensure player entities are refreshed when team membership changes occur. The code now responds to team-related events to keep name tag displays accurate. Additionally, player state tracking and entity event emissions were extended to include team affiliation data. Changes
Sequence Diagram(s)sequenceDiagram
participant Bot
participant Entities
participant PlayerEntity
Bot->>Entities: teamUpdated / teamMemberAdded / teamMemberRemoved event
Entities->>Entities: updateAllEntitiesInTeams(team)
loop For each PlayerEntity in team
Entities->>Entities: emit entityUpdate(PlayerEntity)
Entities->>PlayerEntity: updateNameTagVisibility()
PlayerEntity->>PlayerEntity: Set name tag visibility based on team rules
end
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 (2)
renderer/viewer/three/entities.ts (2)
811-812
: Duplicatename = 'nametag'
assignment (minor)
addNametag
already setsnameTag.name = 'nametag'
(line 165).
The extra assignment here is harmless but redundant. Feel free to drop it.
1043-1052
: MissingupdateNameTagVisibility
call for non-player modelsThe visibility logic is invoked only when the entity is both a
player
ande.playerObject
exists.
If a remote player is for some reason shown with the generic fallback mesh (e.g. skin load failure), their tag will never be updated.
Consider callingupdateNameTagVisibility
unconditionally forentity.type === 'player'
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/viewer/three/entities.ts
(3 hunks)src/entities.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
By code everything seems fine, let me know when if can be merged |
Added some more improvements now to only update necessary entities and also fix that team-removal would not update the name tags. (Needs the new changes in mineflayer to work of course) |
There was a problem hiding this 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 (1)
src/entities.ts (1)
200-208
: Fix loose equality operators.The function logic is correct, but please use strict equality operators for consistency and type safety.
- if ((entityTeam?.nameTagVisibility === 'hideForOwnTeam' && entityTeam.name == team.name) - || (entityTeam?.nameTagVisibility === 'hideForOtherTeams' && entityTeam.name != team.name)) { + if ((entityTeam?.nameTagVisibility === 'hideForOwnTeam' && entityTeam.name === team.name) + || (entityTeam?.nameTagVisibility === 'hideForOtherTeams' && entityTeam.name !== team.name)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/entities.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.ts`: You may use the global variable `bot` directly in any file under ...
src/**/*.ts
: You may use the global variablebot
directly in any file under thesrc/
directory.
Insrc/
code, you may use the global variableappViewer
fromsrc/appViewer.ts
directly. Do not importappViewer
or usewindow.appViewer
; use the globalappViewer
variable as-is.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/vars-usage.mdc)
List of files the instruction was applied to:
src/entities.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:204-211
Timestamp: 2025-06-23T17:04:16.004Z
Learning: In mineflayer, the `teamMemberRemoved` event only provides the team object as a parameter, not the username of the removed player. Therefore, when handling this event, it's necessary to update all player entities because there's no way to identify which specific player was removed from the team.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:204-211
Timestamp: 2025-06-23T17:04:16.004Z
Learning: In mineflayer, the `teamMemberRemoved` event does not provide the removed username as a parameter, so when handling this event, you need to update all player entities because there's no way to identify which specific player was removed from the team.
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:192-199
Timestamp: 2025-06-23T17:04:53.409Z
Learning: The mineflayer `teamMemberAdded` event is emitted with only one argument: the team object, not with both team and username parameters.
src/entities.ts (7)
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, team information for entities should be inlined into entity update events from the world data emitter rather than accessing the global bot object from renderer code. This maintains better separation of concerns between the data layer and renderer.
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:204-211
Timestamp: 2025-06-23T17:04:16.004Z
Learning: In mineflayer, the `teamMemberRemoved` event only provides the team object as a parameter, not the username of the removed player. Therefore, when handling this event, it's necessary to update all player entities because there's no way to identify which specific player was removed from the team.
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:204-211
Timestamp: 2025-06-23T17:04:16.004Z
Learning: In mineflayer, the `teamMemberRemoved` event does not provide the removed username as a parameter, so when handling this event, you need to update all player entities because there's no way to identify which specific player was removed from the team.
Learnt from: Phoenix616
PR: zardoy/minecraft-web-client#373
File: src/entities.ts:192-199
Timestamp: 2025-06-23T17:04:53.409Z
Learning: The mineflayer `teamMemberAdded` event is emitted with only one argument: the team object, not with both team and username parameters.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-06-23T17:08:19.477Z
Learning: In the Mineflayer codebase, maintain a strict separation between game logic (in `src/`) and renderer/view logic (in `renderer/`). The renderer must never access the Mineflayer `bot` global directly; instead, it should interact with game state via explicit interfaces or state managers, such as the `playerState` property.
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-06-23T17:08:19.477Z
Learning: Global variables defined for use in `src/` (such as `appViewer` from `src/appViewer.ts`) should be accessed directly as globals, not imported or accessed via `window`, to ensure consistency and avoid ambiguity.
🧬 Code Graph Analysis (1)
src/entities.ts (2)
src/mineflayer/plugins/mouse.ts (1)
bot
(47-54)src/appViewer.ts (1)
appViewer
(277-277)
🪛 ESLint
src/entities.ts
[error] 203-203: Expected '===' and instead saw '=='.
(eqeqeq)
[error] 204-204: Expected '!==' and instead saw '!='.
(eqeqeq)
🔇 Additional comments (6)
src/entities.ts (6)
7-7
: LGTM!Clean import addition for the Team type needed for the new functionality.
192-198
: LGTM!The team update handler correctly identifies affected player entities and triggers updates to reflect name tag visibility changes.
210-214
: LGTM!The logic correctly determines when entity updates are needed based on name tag visibility rules and the player's team membership.
216-229
: LGTM!Excellent implementation that efficiently handles team member additions by utilizing the updated mineflayer event signature with the members parameter. The reactive state management and conditional updates are well-designed.
231-244
: LGTM!The team member removal handler correctly mirrors the addition logic and efficiently handles bot team state cleanup and selective entity updates.
246-252
: LGTM!The team removal handler correctly manages the bot's reactive state and triggers appropriate entity updates when teams are completely removed.
0c18ee0
to
f8f84ed
Compare
Is there anything still required to get this merged? |
@@ -99,6 +99,10 @@ export class PlayerStateControllerMain { | |||
}) | |||
this.reactive.gameMode = bot.game?.gameMode | |||
|
|||
customEvents.on('gameLoaded', () => { | |||
this.reactive.team = bot.teamMap[bot.username] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Phoenix616 I'm not sure, but shouldn't team
be updated whenever the team is actually updated in (teamUpdated
event above)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
teamUpdated
only updates the values of that team object, I might be wrong but I'm pretty sure Typescript/Javascript is pass by reference and doesn't copy the object on variable assignment (pass by value) so any change to the team object itself should be reflected in this variable no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will re-check.
This adds support for the
nameTagVisibility
option which teams have to hide the names of players in certain situations.Summary by CodeRabbit
New Features
Bug Fixes