From dcba1eef1c6bcb04d76c69e1fdae8f3ba957a641 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Wed, 5 Aug 2026 08:04:36 +0000 Subject: [PATCH] Address review on the roles plugin - Use the core role permissions rather than defining new ones. Assignments are only sent between the controller and an instance, so they are no longer addressed to control and need no permission at all. - Split the datastore reconciliation into ensureRoleMeta, sweepRoleMeta and applyAutoAssign, all run on init as well as when roles change. Properties left behind by a role deleted while the plugin was not running are now swept up, where before they would be inherited by the next role given that id. - Use a switch in onControllerConfigFieldChanged, matching instance.ts. - Resolve pending assignments on initialise. A pending role is either confirmed, and so now held by synced_players, or it never landed; either way it stops being held locally. Roles assigned with assign_player_local are untouched. - Drop the emit_updates juggling in reject_assignment, sync is already false. - Send each permission name once and reference it by index, see the benchmark in the pull request. Single role updates stay in the plain form. - Lay the role properties out in columns, name the apply button for the section it belongs to, and give every field a tooltip. Co-Authored-By: Claude Opus 5 (1M context) --- exp_roles/controller.ts | 52 ++++--- exp_roles/index.ts | 33 +---- exp_roles/instance.ts | 2 +- exp_roles/messages.ts | 45 ++++-- exp_roles/module/control.lua | 76 +++++++---- exp_roles/web/components/RoleProperties.tsx | 144 ++++++++++++-------- 6 files changed, 212 insertions(+), 140 deletions(-) diff --git a/exp_roles/controller.ts b/exp_roles/controller.ts index b1177471..b9bf3a8b 100644 --- a/exp_roles/controller.ts +++ b/exp_roles/controller.ts @@ -16,8 +16,12 @@ export class ControllerPlugin extends BaseControllerPlugin { ).bootstrap() ); - // Roles created before this plugin was installed have no properties yet + // The datastore can be out of step with the roles, either because the + // plugin was installed after they were created or because it was + // uninstalled while they were deleted this.ensureRoleMeta(); + this.sweepRoleMeta(); + this.applyAutoAssign(); this.controller.subscriptions.handle(messages.RoleUpdatedEvent, this.handleRoleSubscription.bind(this)); this.controller.subscriptions.handle( @@ -65,6 +69,28 @@ export class ControllerPlugin extends BaseControllerPlugin { return created; } + /** + * Delete properties left behind by roles which no longer exist. + * + * Without this they would be picked up by whichever role is next given the + * same id, which happens when a role is deleted while the plugin is not + * running. + */ + sweepRoleMeta() { + const orphaned = []; + for (const meta of this.roleMeta.valuesMutable()) { + if (!this.controller.roles.has(meta.id)) { + orphaned.push(meta); + } + } + + if (orphaned.length) { + this.roleMeta.deleteMany(orphaned); + } + + return orphaned; + } + /** Combine a clusterio role with its in game properties. */ buildRoleRecord(role: Readonly) { const meta = this.roleMeta.get(role.id) ?? new messages.RoleMetaRecord(role.id, role.id); @@ -80,9 +106,11 @@ export class ControllerPlugin extends BaseControllerPlugin { } async onControllerConfigFieldChanged(field: string, curr: unknown, prev: unknown) { - // Which role is the default is carried on the role records themselves - if (field === "controller.default_role_id") { - this.controller.subscriptions.broadcast(new messages.RoleUpdatedEvent(this.listRoleRecords())); + switch (field) { + // Which role is the default is carried on the role records themselves + case "controller.default_role_id": + this.controller.subscriptions.broadcast(new messages.RoleUpdatedEvent(this.listRoleRecords())); + break; } } @@ -107,21 +135,7 @@ export class ControllerPlugin extends BaseControllerPlugin { /** A clusterio role was created, changed or deleted. */ rolesUpdated(roles: lib.Role[]) { const created = this.ensureRoleMeta(); - - // Deleting a role leaves its properties behind, which would be reused by - // the next role to be given the same id - const orphaned = []; - for (const role of roles) { - if (role.isDeleted) { - const meta = this.roleMeta.getMutable(role.id); - if (meta) { - orphaned.push(meta); - } - } - } - if (orphaned.length) { - this.roleMeta.deleteMany(orphaned); - } + this.sweepRoleMeta(); // Newly created properties broadcast on their own through roleMetaUpdated const createdIds = new Set(created.map(meta => meta.id)); diff --git a/exp_roles/index.ts b/exp_roles/index.ts index 96d8cb0a..e3cb4093 100644 --- a/exp_roles/index.ts +++ b/exp_roles/index.ts @@ -7,36 +7,9 @@ declare module "@clusterio/lib" { } } -lib.definePermission({ - name: "exp_roles.role.list", - title: "List In Game Roles", - description: "List the in game properties of all roles.", - grantByDefault: true, -}); -lib.definePermission({ - name: "exp_roles.role.subscribe", - title: "Subscribe to In Game Role Updates", - description: "Receive updates when the in game properties of a role change.", - grantByDefault: true, -}); -lib.definePermission({ - name: "exp_roles.role.update", - title: "Update In Game Roles", - description: "Modify the in game properties of a role, such as its order and colour.", -}); - -lib.definePermission({ - name: "exp_roles.assignment.list", - title: "List Role Assignments", - description: "List the roles held by each player in game.", - grantByDefault: true, -}); -lib.definePermission({ - name: "exp_roles.assignment.subscribe", - title: "Subscribe to Role Assignment Updates", - description: "Receive updates when the roles held by a player change.", - grantByDefault: true, -}); +// The in game properties of a role are part of the role, so they are covered by +// the core role permissions rather than permissions of their own. Assignments +// are only ever sent between the controller and an instance, so they need none. export const plugin: lib.PluginDeclaration = { name: "exp_roles", diff --git a/exp_roles/instance.ts b/exp_roles/instance.ts index df7a03a4..615fe7c2 100644 --- a/exp_roles/instance.ts +++ b/exp_roles/instance.ts @@ -49,7 +49,7 @@ export class InstancePlugin extends BaseInstancePlugin { ]); await this.luaSend("initialise", { - roles: roles.map(role => role.toJSON()), + ...messages.encodeRolesForLua(roles), assignments: assignments.map(assignment => assignment.toJSON()), }); await this.luaSetEmitEvents(this.syncMode === "bidirectional"); diff --git a/exp_roles/messages.ts b/exp_roles/messages.ts index dda0c144..3886970b 100644 --- a/exp_roles/messages.ts +++ b/exp_roles/messages.ts @@ -240,6 +240,37 @@ export class AssignmentRecord { } } +/** + * Encode roles for the lua initialise call. + * + * Roles repeat the same permission names over and over, so each name is sent + * once and referenced by index. At 15 roles this is 17 KiB rather than 38 KiB + * and decodes in 0.37 ms rather than 0.50 ms; at 60 roles it is 30 KiB rather + * than 122 KiB and 0.87 ms rather than 1.52 ms. + * + * Single role updates stay in the plain form, they have nothing to share. + */ +export function encodeRolesForLua(roles: RoleRecord[]) { + const names: string[] = []; + const indexes = new Map(); + + const encoded = roles.map(role => { + const permissions = role.permissions.map(permission => { + let index = indexes.get(permission); + if (index === undefined) { + index = names.length; + names.push(permission); + indexes.set(permission, index); + } + return index; + }); + + return { ...role.toJSON(), permissions }; + }); + + return { permission_names: names, roles: encoded }; +} + /* Update events */ @@ -250,7 +281,7 @@ export class RoleUpdatedEvent { static type = "event" as const; static src = "controller" as const; static dst = ["control", "instance"] as const; - static permission = "exp_roles.role.subscribe" as const; + static permission = "core.role.subscribe" as const; constructor( public updates: RoleRecord[], @@ -274,8 +305,7 @@ export class AssignmentUpdatedEvent { static plugin = "exp_roles" as const; static type = "event" as const; static src = "controller" as const; - static dst = ["control", "instance"] as const; - static permission = "exp_roles.assignment.subscribe" as const; + static dst = "instance" as const; constructor( public updates: AssignmentRecord[], @@ -304,7 +334,7 @@ export class RoleListRequest { static type = "request" as const; static src = ["control", "instance"] as const; static dst = "controller" as const; - static permission = "exp_roles.role.list" as const; + static permission = "core.role.list" as const; static Response = lib.jsonArray(RoleRecord); constructor() {} @@ -316,7 +346,7 @@ export class RoleMetaUpdateRequest { static type = "request" as const; static src = "control" as const; static dst = "controller" as const; - static permission = "exp_roles.role.update" as const; + static permission = "core.role.update" as const; constructor( public meta: RoleMetaRecord, @@ -343,9 +373,8 @@ export class AssignmentListRequest { declare ["constructor"]: typeof AssignmentListRequest; static plugin = "exp_roles" as const; static type = "request" as const; - static src = ["control", "instance"] as const; + static src = "instance" as const; static dst = "controller" as const; - static permission = "exp_roles.assignment.list" as const; static Response = lib.jsonArray(AssignmentRecord); constructor() {} @@ -359,7 +388,7 @@ export class AssignmentUpdateRequest { declare ["constructor"]: typeof AssignmentUpdateRequest; static plugin = "exp_roles" as const; static type = "request" as const; - static src = ["control", "instance"] as const; + static src = "instance" as const; static dst = "controller" as const; static permission = "core.user.update_roles" as const; diff --git a/exp_roles/module/control.lua b/exp_roles/module/control.lua index 91139f60..59b5696e 100644 --- a/exp_roles/module/control.lua +++ b/exp_roles/module/control.lua @@ -141,12 +141,20 @@ end --- Build a role from the record sent by the controller --- @param record table +--- @param names string[]? When given, record.permissions holds indexes into it --- @return ExpRoles.Role -local function decode_role(record) +local function decode_role(record, names) local meta = record.meta local allowed_actions = {} - for _, permission in pairs(record.permissions) do - allowed_actions[permission] = true + if names then + -- Indexes are zero based, having come from javascript + for _, index in pairs(record.permissions) do + allowed_actions[names[index + 1]] = true + end + else + for _, permission in pairs(record.permissions) do + allowed_actions[permission] = true + end end local flags = {} @@ -732,6 +740,26 @@ end Sync entry points ]] +--- Stop holding a pending role locally once the controller has confirmed it +--- Roles assigned with assign_player_local are never pending so are untouched +--- @param player_name string +local function drop_confirmed_local(player_name) + local pending = script_data.pending[player_name] + if pending then + for _, role_id in pairs(script_data.synced_players[player_name] or {}) do + if remove_role_id(pending, role_id) then + remove_role_id(script_data.local_players[player_name], role_id) + end + end + if not next(pending) then script_data.pending[player_name] = nil end + end + + local local_roles = script_data.local_players[player_name] + if local_roles and not next(local_roles) then + script_data.local_players[player_name] = nil + end +end + --- Restore local references to persistent script data after load function ExpRoles.on_load() script_data = compat.script_data["exp_roles"] @@ -748,16 +776,18 @@ function ExpRoles.set_emit_events(enabled) end --- Replace all local state with the state held by the controller ---- @param payload { roles: table[], assignments: table[] } +--- Permission names are sent once and referenced by index, see encodeRolesForLua +--- @param payload { permission_names: string[], roles: table[], assignments: table[] } function ExpRoles.initialise(payload) local emit_updates = script_data.emit_updates script_data.emit_updates = false + local names = payload.permission_names script_data.roles = {} script_data.default_role_id = nil for _, record in pairs(payload.roles) do if not record.is_deleted then - script_data.roles[record.id] = decode_role(record) + script_data.roles[record.id] = decode_role(record, names) if record.is_default then script_data.default_role_id = record.id end @@ -771,8 +801,19 @@ function ExpRoles.initialise(payload) end end - -- Anything still pending was sent before the connection was lost, the - -- controller state received here is authoritative + -- The state received here is authoritative, so a pending role is either + -- confirmed and now held by synced_players, or it never landed and has to + -- be given up; either way it stops being held locally + for player_name, pending in pairs(script_data.pending) do + drop_confirmed_local(player_name) + for _, role_id in pairs(pending) do + remove_role_id(script_data.local_players[player_name], role_id) + end + local local_roles = script_data.local_players[player_name] + if local_roles and not next(local_roles) then + script_data.local_players[player_name] = nil + end + end script_data.pending = {} rebuild_role_views() @@ -823,22 +864,7 @@ function ExpRoles.receive_assignment_updates(records) script_data.synced_players[player_name] = record.role_ids or {} end - -- Anything confirmed by the controller no longer needs to be held locally - local pending = script_data.pending[player_name] - if pending then - for _, role_id in pairs(script_data.synced_players[player_name] or {}) do - if remove_role_id(pending, role_id) then - remove_role_id(script_data.local_players[player_name], role_id) - end - end - if not next(pending) then script_data.pending[player_name] = nil end - end - - local local_roles = script_data.local_players[player_name] - if local_roles and not next(local_roles) then - script_data.local_players[player_name] = nil - end - + drop_confirmed_local(player_name) rebuild_player_view(player_name) local player = game.get_player(player_name) @@ -858,10 +884,8 @@ function ExpRoles.reject_assignment(payload) if #roles == 0 then return end - local emit_updates = script_data.emit_updates - script_data.emit_updates = false + -- Sync is false, so this is never sent back to the controller change_player_roles(player_name, roles, "unassign", "", true, false) - script_data.emit_updates = emit_updates end --- Get the current script data for debugging purposes diff --git a/exp_roles/web/components/RoleProperties.tsx b/exp_roles/web/components/RoleProperties.tsx index 8e904f2b..a3729ec9 100644 --- a/exp_roles/web/components/RoleProperties.tsx +++ b/exp_roles/web/components/RoleProperties.tsx @@ -1,5 +1,5 @@ import React, { useContext, useEffect, useState } from "react"; -import { Button, ColorPicker, Form, Input, InputNumber, Space, Switch, Tooltip } from "antd"; +import { Button, Col, ColorPicker, Form, Input, InputNumber, Row, Switch, Tooltip } from "antd"; import * as lib from "@clusterio/lib"; import { ControlContext, SectionHeader, useAccount, notifyErrorHandler } from "@clusterio/web_ui"; @@ -34,7 +34,7 @@ export default function RoleProperties(props: { plugin: WebPlugin, role?: lib.Ro const [roles] = props.plugin.useRoles(); const record = props.role ? roles.get(props.role.id) : undefined; const meta = record?.meta; - const canUpdate = account.hasPermission("exp_roles.role.update"); + const canUpdate = account.hasPermission("core.role.update"); useEffect(() => { if (!meta) { @@ -85,61 +85,93 @@ export default function RoleProperties(props: { plugin: WebPlugin, role?: lib.Ro ))); } + const labelled = (text: string, tip: string) => {text}; + return <> - -
- Order} - > - - - - Priority - - } - > - - - - - - - - - - - - - Auto assign after (hours) - - } - > - - - - - - {canUpdate && - - - - } + { + setApplying(true); + apply() + .catch(notifyErrorHandler("Error updating role properties")) + .finally(() => setApplying(false)); + }} + >Apply in game properties : undefined} + /> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ; }