Fix empty group ordering

src/schedule/core/systembuilder.js

@@ -219,7 +219,7 @@ export class SchedulerBuilder {
    * @returns {SystemRegistration[]}
    */
   sortScheduleSystems(context) {
-    const graph = this.expandScheduleGraph(context)
+    const { graph, systemsByGraphId } = this.expandScheduleGraph(context)
     const sorted = kahnTopologySort(graph)
 
     if (!sorted) {
@@ -230,9 +230,10 @@ export class SchedulerBuilder {
     const ordered = []
 
     for (let i = 0; i < sorted.length; i++) {
-      const system = graph.getNodeWeight(sorted[i])
+      const system = systemsByGraphId.get(sorted[i])
+
+      if (!system) continue
 
-      assert(system, `Internal error: Could not resolve system node ${sorted[i]} on schedule "${context.label}".`)
       ordered.push(system)
     }
 
@@ -242,25 +243,41 @@ export class SchedulerBuilder {
   /**
    * @private
    * @param {ScheduleContext} context
-   * @returns {Graph<SystemRegistration, undefined>}
+   * @returns {{ graph: Graph<SystemRegistration | SystemGroupRegistration, undefined>, systemsByGraphId: Map<number, SystemRegistration> }}
    */
   expandScheduleGraph(context) {
-    const graph = /** @type {Graph<SystemRegistration, undefined>} */ (new Graph(true))
+    const graph = /** @type {Graph<SystemRegistration | SystemGroupRegistration, undefined>} */ (new Graph(true))
 
     /** @type {Map<number, number>} */
     const graphIdsBySystemId = new Map()
 
+    /** @type {Map<number, number>} */
+    const graphIdsByGroupId = new Map()
+
+    context.graphIdsByGroupId = graphIdsByGroupId
+
+    /** @type {Map<number, SystemRegistration>} */
+    const systemsByGraphId = new Map()
+
     /** @type {Map<number, number[]>} */
     const groupSystemsCache = new Map()
 
     /** @type {Set<string>} */
     const edges = new Set()
 
+    for (let i = 0; i < context.groups.length; i++) {
+      const group = context.groups[i]
+      const graphId = graph.addNode(group)
+
+      graphIdsByGroupId.set(group.id, graphId)
+    }
+
     for (let i = 0; i < context.systems.length; i++) {
       const system = context.systems[i]
       const graphId = graph.addNode(system)
 
       graphIdsBySystemId.set(system.id, graphId)
+      systemsByGraphId.set(graphId, system)
     }
 
     for (let i = 0; i < context.systems.length; i++) {
@@ -281,13 +298,16 @@ export class SchedulerBuilder {
       }, group.config.before, group.config.after, groupSystemsCache)
     }
 
-    return graph
+    return {
+      graph,
+      systemsByGraphId
+    }
   }
 
   /**
    * @private
    * @param {ScheduleContext} context
-   * @param {Graph<SystemRegistration, undefined>} graph
+   * @param {Graph<SystemRegistration | SystemGroupRegistration, undefined>} graph
    * @param {Map<number, number>} graphIdsBySystemId
    * @param {Set<string>} edges
    * @param {ScheduleNodeRef} source
@@ -336,7 +356,7 @@ export class SchedulerBuilder {
   /**
    * @private
    * @param {ScheduleContext} context
-   * @param {Graph<SystemRegistration, undefined>} graph
+   * @param {Graph<SystemRegistration | SystemGroupRegistration, undefined>} graph
    * @param {Map<number, number>} graphIdsBySystemId
    * @param {Set<string>} edges
    * @param {ScheduleNodeRef} from
@@ -345,34 +365,81 @@ export class SchedulerBuilder {
    * @param {Map<number, number[]>} groupSystemsCache
    */
   addExpandedEdge(context, graph, graphIdsBySystemId, edges, from, to, targetLabel, groupSystemsCache) {
-    const fromSystems = this.expandNodeToSystems(context, from, groupSystemsCache)
-    const toSystems = this.expandNodeToSystems(context, to, groupSystemsCache)
+    const fromNodes = this.expandNodeToOrderingNodes(context, from, graphIdsBySystemId, groupSystemsCache)
+    const toNodes = this.expandNodeToOrderingNodes(context, to, graphIdsBySystemId, groupSystemsCache)
 
-    for (let i = 0; i < fromSystems.length; i++) {
-      for (let j = 0; j < toSystems.length; j++) {
-        const fromSystemId = fromSystems[i]
-        const toSystemId = toSystems[j]
+    for (let i = 0; i < fromNodes.length; i++) {
+      for (let j = 0; j < toNodes.length; j++) {
+        const fromNodeId = fromNodes[i]
+        const toNodeId = toNodes[j]
 
-        if (fromSystemId === toSystemId) {
+        if (fromNodeId === toNodeId) {
           throws(`The reference "${describeReference(targetLabel)}" creates a self-referential system ordering on schedule "${context.label}".`)
         }
 
-        const graphFrom = graphIdsBySystemId.get(fromSystemId)
-        const graphTo = graphIdsBySystemId.get(toSystemId)
-
-        assert(graphFrom !== undefined, `Internal error: Could not resolve graph node for system ${fromSystemId} on schedule "${context.label}".`)
-        assert(graphTo !== undefined, `Internal error: Could not resolve graph node for system ${toSystemId} on schedule "${context.label}".`)
-
-        const key = `${graphFrom}:${graphTo}`
+        const key = `${fromNodeId}:${toNodeId}`
 
         if (edges.has(key)) continue
 
         edges.add(key)
-        graph.addEdge(graphFrom, graphTo, undefined)
+        graph.addEdge(fromNodeId, toNodeId, undefined)
       }
     }
   }
 
+  /**
+   * @private
+   * @param {ScheduleContext} context
+   * @param {ScheduleNodeRef} node
+   * @param {Map<number, number>} graphIdsBySystemId
+   * @param {Map<number, number[]>} groupSystemsCache
+   * @returns {number[]}
+   */
+  expandNodeToOrderingNodes(context, node, graphIdsBySystemId, groupSystemsCache) {
+    if (node.kind === ScheduleNodeKind.System) {
+      const graphId = graphIdsBySystemId.get(node.id)
+
+      assert(graphId !== undefined, `Internal error: Could not resolve graph node for system ${node.id} on schedule "${context.label}".`)
+
+      return [graphId]
+    }
+
+    const systems = this.expandGroupToSystems(context, node.id, groupSystemsCache, new Set())
+
+    if (systems.length > 0) {
+
+      /** @type {number[]} */
+      const graphIds = []
+
+      for (let i = 0; i < systems.length; i++) {
+        const graphId = graphIdsBySystemId.get(systems[i])
+
+        assert(graphId !== undefined, `Internal error: Could not resolve graph node for system ${systems[i]} on schedule "${context.label}".`)
+        graphIds.push(graphId)
+      }
+
+      return graphIds
+    }
+
+    const graphId = this.expandGroupToGraphNode(context, node.id)
+
+    return [graphId]
+  }
+
+  /**
+   * @private
+   * @param {ScheduleContext} context
+   * @param {number} groupId
+   * @returns {number}
+   */
+  expandGroupToGraphNode(context, groupId) {
+    const graphId = context.graphIdsByGroupId?.get(groupId)
+
+    assert(graphId !== undefined, `Internal error: Could not resolve graph node for system group ${groupId} on schedule "${context.label}".`)
+
+    return graphId
+  }
+
   /**
    * @private
    * @param {ScheduleContext} context
@@ -445,6 +512,7 @@ function getOrCreateScheduleContext(schedules, label) {
     groups: [],
     nodesByLabel: new Map(),
     groupIdsByTypeId: new Map(),
+    graphIdsByGroupId: undefined,
     defaultSystemGroup: undefined
   })
 
@@ -475,6 +543,7 @@ const ScheduleNodeKind = Object.freeze({
  * @property {SystemGroupRegistration[]} groups
  * @property {Map<string, ScheduleNodeRef>} nodesByLabel
  * @property {Map<TypeId, number>} groupIdsByTypeId
+ * @property {Map<number, number> | undefined} graphIdsByGroupId
  * @property {Constructor | undefined} defaultSystemGroup
  */
 

src/schedule/tests/scheduleBuilder.test.js

@@ -334,6 +334,53 @@ describe('Testing `SchedulerBuilder`', () => {
     deepStrictEqual(order, ['earlyOne', 'earlyTwo', 'lateOne', 'lateTwo'])
   })
 
+  test('treats empty groups as ordering barriers', () => {
+    const builder = new SchedulerBuilder()
+    const scheduler = new Scheduler()
+    const world = new World()
+    /** @type {string[]} */
+    const order = []
+
+    class StartPhase { }
+    class MiddlePhase { }
+    class EndPhase { }
+
+    function startSystem() { order.push('start') }
+    function endSystem() { order.push('end') }
+
+    scheduler.set(new Executable({ label: 'update' }))
+
+    builder.addGroup({
+      label: StartPhase,
+      schedule: 'update',
+      before: [MiddlePhase]
+    })
+    builder.addGroup({
+      label: MiddlePhase,
+      schedule: 'update',
+      before: [EndPhase]
+    })
+    builder.addGroup({
+      label: EndPhase,
+      schedule: 'update'
+    })
+    builder.add({
+      schedule: 'update',
+      systemGroup: EndPhase,
+      system: endSystem
+    })
+    builder.add({
+      schedule: 'update',
+      systemGroup: StartPhase,
+      system: startSystem
+    })
+
+    builder.pushToScheduler(scheduler)
+    scheduler.get('update')?.run(world)
+
+    deepStrictEqual(order, ['start', 'end'])
+  })
+
   test('inherits parent ordering constraints across descendants', () => {
     const builder = new SchedulerBuilder()
     const scheduler = new Scheduler()