diff --git a/AI_GAME_STUDIO_ASSESSMENT.md b/AI_GAME_STUDIO_ASSESSMENT.md index a4d69712a..31b7c5926 100644 --- a/AI_GAME_STUDIO_ASSESSMENT.md +++ b/AI_GAME_STUDIO_ASSESSMENT.md @@ -4,7 +4,8 @@ Answers three questions the fork asked, then records what this plan builds on th Every claim below carries a `path:line` (or a `path`) that was read while writing this document, in this checkout. Where a figure is quoted from another project, the file that -contains it is named. +contains it is named. The gap claims were measured at the parent of `ae815f489`, before +the phases of section 5 landed; those that section 5 closes are stated in the past tense. --- @@ -37,9 +38,9 @@ common repeated-prop case, not for an arbitrary scene. `GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts:263`. - `RuntimeObject3D.getRendererObject()` returns `null` (`Extensions/3D/A_RuntimeObject3D.ts:97-99`), so the 2D AABB culling path in - `CustomRuntimeObjectInstanceContainer._updateObjectsPreRender` skips every 3D object - entirely: `if (rendererObject) { ... } else { object.updatePreRender(this); }` - (`GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts:266-283`). + `GDJS/Runtime/runtimescene.ts` `_updateObjectsPreRender` (pre-change `:529-573`) + skips every 3D object entirely: + `if (rendererObject) { ... } else { object.updatePreRender(this); }`. So every 3D object in the scene is submitted to the GPU every frame regardless of where the camera is. Combined with 1.1, a large city is not merely slow — it is bounded by total @@ -206,7 +207,8 @@ create_or_update_plan, run_gameplay_test, change_gameplay_tests `spawn_agent` is added to this list by Phase 4. The four roles and their exact tool names are defined in `newIDE/app/src/AiGeneration/Studio/Roles.js` (Phase 2); the read-only invariant -(tester mutates nothing; designer's only mutating tool is `add_or_edit_variable`) is asserted +(tester mutates no game content, but writes test definitions (`change_gameplay_tests`, +`run_gameplay_test`); designer's only mutating tool is `add_or_edit_variable`) is asserted by `Roles.spec.js`. --- @@ -218,7 +220,7 @@ by `Roles.spec.js`. | No instancing: every Model3D instance clones its own scene graph | `Extensions/3D/Model3DRuntimeObject3DRenderer.ts:349` (`SkeletonUtils.clone`); no `THREE.InstancedMesh` outside vendored Three.js | Draw calls rise 1:1 with repeated props; framerate collapses on any repeated-model set | **7** | | 3D objects never culled in a scene | `GDJS/Runtime/runtimescene.ts:524` (`TODO (3D) culling`) | Off-camera 3D objects still submitted every frame | **6** | | 3D objects never culled inside a custom object | `GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts:263` (`TODO (3D) culling`) | A custom object holding many 3D children is never culled as a whole or in part | **6** | -| `RuntimeObject3D` reports no renderer object, so the 2D AABB path skips it | `Extensions/3D/A_RuntimeObject3D.ts:97-99` (`getRendererObject() { return null; }`); branch at `GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts:266-283` | Reinforces the two rows above: even the existing 2D culling cannot see 3D objects | **6** | +| `RuntimeObject3D` reports no renderer object, so the 2D AABB path skips it | `Extensions/3D/A_RuntimeObject3D.ts:97-99` (`getRendererObject() { return null; }`) | Reinforces the two rows above: even the existing 2D culling cannot see 3D objects | **6** | | Base container has no camera to cull against | `GDJS/Runtime/RuntimeInstanceContainer.ts:496` (`TODO (3D) culling`) | Accepted — see below | accepted, not fixed | | TileMap viewport culling only for SimpleTileMap, only as a direct scene child, only with an unrotated 3D camera | `Extensions/TileMap/tilemapruntimeobject-pixi-renderer.ts:152-159` (`dimX + dimY > 100 && isSimpleTileMap(object) && instanceContainer === scene && (!gdjs.scene3d || rotations are 0)`) | A large `TileMap` (not Simple) or a tilemap nested in a custom object re-renders the whole map every frame | **8** | | Any tilemap opacity change forces a full re-render | `Extensions/TileMap/tilemapruntimeobject-pixi-renderer.ts:90-95` (`updateTileMap(true)`) | Fading a district stutters | accepted, not fixed | diff --git a/Extensions/3D/A_RuntimeObject3D.ts b/Extensions/3D/A_RuntimeObject3D.ts index b8c329de0..517a8ba3c 100644 --- a/Extensions/3D/A_RuntimeObject3D.ts +++ b/Extensions/3D/A_RuntimeObject3D.ts @@ -110,6 +110,10 @@ namespace gdjs { return gdjs.Object3DCulling.isInFrustum(this, frustum); } + setCullingVisible(culled: boolean): void { + this.getRenderer().setCullingVisible(culled); + } + getRendererObject() { return null; } @@ -612,7 +616,7 @@ namespace gdjs { hide(enable: boolean): void { super.hide(enable); - this.getRenderer().updateVisibility(); + this.getRenderer().updateVisibility(!this.isHidden()); } hasEstimatedVelocity(): boolean { diff --git a/Extensions/3D/A_RuntimeObject3DRenderer.ts b/Extensions/3D/A_RuntimeObject3DRenderer.ts index 7016acdfb..847cb5a77 100644 --- a/Extensions/3D/A_RuntimeObject3DRenderer.ts +++ b/Extensions/3D/A_RuntimeObject3DRenderer.ts @@ -54,8 +54,30 @@ namespace gdjs { this.updatePosition(); } - updateVisibility() { - this._threeObject3D.visible = !this._object.isHidden(); + /** + * The object's own hidden state (`!isHidden()`) and its frustum-cull state, + * combined by `applyRenderVisibility`. Kept separate so hiding and culling + * stay independent and can both be routed through the renderer (a detached + * three.js node is not enough: an instanced model's real draw is an + * `InstancedMesh` slot, which `Model3DRuntimeObject3DRenderer` collapses). + */ + protected _isVisible: boolean = true; + protected _isCulled: boolean = false; + + updateVisibility(visible: boolean) { + this._isVisible = !!visible; + this.applyRenderVisibility(); + } + + setCullingVisible(culled: boolean): void { + this._isCulled = !!culled; + this.applyRenderVisibility(); + } + + /** Combine the object's hidden state with its frustum-cull state. */ + protected applyRenderVisibility(): void { + const threeObject = this.get3DRendererObject(); + if (threeObject) threeObject.visible = this._isVisible && !this._isCulled; } invalidateRotation(): void { diff --git a/Extensions/3D/CustomRuntimeObject3D.ts b/Extensions/3D/CustomRuntimeObject3D.ts index 16ab0bf2f..e1f3b634b 100644 --- a/Extensions/3D/CustomRuntimeObject3D.ts +++ b/Extensions/3D/CustomRuntimeObject3D.ts @@ -41,6 +41,7 @@ namespace gdjs { private _rotationY: float = 0; private _customCenterZ: float = 0; private static _temporaryVector = new THREE.Vector3(); + private static _cullingScratchBox: THREE.Box3 | null = null; private _hasEstimatedVelocity = false; private _estimatedVelocityX: float = 0; @@ -81,9 +82,28 @@ namespace gdjs { /** * Writes the object's conservative axis-aligned box into `box` and returns * it. Allocates nothing. See `gdjs.Object3DCulling.computeAABB`. + * + * The box is widened to cover each 3D child's own box so that content which + * pokes outside the parent's declared size is never wrongly culled. This is + * conservative in the safe direction: it can only draw more, never hide. */ getAABB3D(box: THREE.Box3): THREE.Box3 { - return gdjs.Object3DCulling.computeAABB(this, box); + gdjs.Object3DCulling.computeAABB(this, box); + const scratch = + CustomRuntimeObject3D._cullingScratchBox || + (CustomRuntimeObject3D._cullingScratchBox = new THREE.Box3()); + for (const childInstance of this._instanceContainer.getAdhocListOfAllInstances()) { + if (!childInstance.isIncludedInParentCollisionMask()) { + continue; + } + if (!gdjs.CullableRuntimeObject3D.isCullable3D(childInstance)) { + continue; + } + childInstance.getAABB3D(scratch); + box.min.min(scratch.min); + box.max.max(scratch.max); + } + return box; } /** @@ -94,6 +114,10 @@ namespace gdjs { return gdjs.Object3DCulling.isInFrustum(this, frustum); } + setCullingVisible(culled: boolean): void { + this.getRenderer().setCullingVisible(culled); + } + override extraInitializationFromInitialInstance( initialInstanceData: InstanceData ) { @@ -396,6 +420,17 @@ namespace gdjs { if (!childInstance.isIncludedInParentCollisionMask()) { continue; } + if (gdjs.CullableRuntimeObject3D.isCullable3D(childInstance)) { + // Conservative z-extent including the child's rotation (see the + // rotation-aware box in `gdjs.Object3DCulling.computeAABB`). + const scratch = + CustomRuntimeObject3D._cullingScratchBox || + (CustomRuntimeObject3D._cullingScratchBox = new THREE.Box3()); + childInstance.getAABB3D(scratch); + minZ = Math.min(minZ, scratch.min.z); + maxZ = Math.max(maxZ, scratch.max.z); + continue; + } if (!gdjs.Base3DHandler.is3D(childInstance)) { continue; } diff --git a/Extensions/3D/CustomRuntimeObject3DRenderer.ts b/Extensions/3D/CustomRuntimeObject3DRenderer.ts index aa411d7ac..711d0a92f 100644 --- a/Extensions/3D/CustomRuntimeObject3DRenderer.ts +++ b/Extensions/3D/CustomRuntimeObject3DRenderer.ts @@ -84,7 +84,7 @@ namespace gdjs { this._object.isFlippedZ() ? -scaleZ : scaleZ ); - threeObject3D.visible = !this._object.hidden; + this.updateVisibility(); this._isContainerDirty = false; } @@ -129,8 +129,22 @@ namespace gdjs { this._isContainerDirty = true; } + private _isVisible: boolean = true; + private _isCulled: boolean = false; + updateVisibility(): void { - this._threeGroup.visible = !this._object.hidden; + this._isVisible = !this._object.hidden; + this.applyRenderVisibility(); + } + + setCullingVisible(culled: boolean): void { + this._isCulled = !!culled; + this.applyRenderVisibility(); + } + + /** Combine the object's hidden state with its frustum-cull state. */ + applyRenderVisibility(): void { + this._threeGroup.visible = this._isVisible && !this._isCulled; } updateOpacity(): void { diff --git a/Extensions/3D/Model3DInstancePool.ts b/Extensions/3D/Model3DInstancePool.ts index 42e6b1c98..3acb6f3d5 100644 --- a/Extensions/3D/Model3DInstancePool.ts +++ b/Extensions/3D/Model3DInstancePool.ts @@ -29,12 +29,31 @@ namespace gdjs { usedSlotCount: integer; /** Whether the instance matrices changed since the last `flush`. */ isMatrixDirty: boolean; + /** + * Slots that cast/receive shadows. Drives the `InstancedMesh`'s mesh-level + * shadow flags (Three.js shadows are per-mesh, not per-instance). + */ + castShadowSlots: Set; + receiveShadowSlots: Set; }; export class Model3DInstancePool { private _entries: Map = new Map(); - /** The scratch matrix, reused by `setMatrix`; never returned. */ - private _temporaryMatrix: THREE.Matrix4 = new THREE.Matrix4(); + /** + * Reused scratch (a `getMatrixAt` target and the collapsed matrix); never + * returned to callers. Allocated lazily so an unused pool allocates nothing. + */ + private _scratchMatrix: THREE.Matrix4 | null = null; + /** Preallocated `flush` handler so `flush` allocates no closure per frame. */ + private _flushEntry: (entry: InstanceEntry) => void; + + constructor() { + this._flushEntry = (entry: InstanceEntry): void => { + if (!entry.isMatrixDirty) return; + entry.instancedMesh.instanceMatrix.needsUpdate = true; + entry.isMatrixDirty = false; + }; + } /** * Take a free slot for `key`, creating the `InstancedMesh` when the key is @@ -52,23 +71,31 @@ namespace gdjs { instancedMesh, growMesh: (newCapacity: integer) => { const grown = createMesh(newCapacity); - // Move the matrices already written onto the bigger mesh. + // Move the matrices already written onto the bigger mesh. Read from + // the *current* mesh, not the one this closure first saw - a second + // grow must copy from the previous grown mesh, not a disposed one. + const current = newEntry.instancedMesh; + const scratch = + this._scratchMatrix || (this._scratchMatrix = new THREE.Matrix4()); for (let slot = 0; slot < newEntry.usedSlotCount; slot++) { - instancedMesh.getMatrixAt(slot, this._temporaryMatrix); - grown.setMatrixAt(slot, this._temporaryMatrix); + current.getMatrixAt(slot, scratch); + grown.setMatrixAt(slot, scratch); } grown.instanceMatrix.needsUpdate = true; - if (instancedMesh.parent) { - instancedMesh.parent.remove(instancedMesh); - instancedMesh.parent.add(grown); - } - instancedMesh.dispose(); + // `createMesh` already adds the grown mesh to the layer group; only + // the old one must be dropped (capture the parent first: `remove` + // nulls `.parent`). + const parent = current.parent; + if (parent) parent.remove(current); + current.dispose(); newEntry.instancedMesh = grown; return grown; }, freeSlots: [], usedSlotCount: 0, isMatrixDirty: true, + castShadowSlots: new Set(), + receiveShadowSlots: new Set(), }; this._entries.set(key, newEntry); return this._takeSlot(newEntry); @@ -95,19 +122,67 @@ namespace gdjs { if (!entry) return; if (slot < 0 || slot >= entry.usedSlotCount) return; if (entry.freeSlots.indexOf(slot) !== -1) return; + // Collapse first: a deleted object must stop being drawn even before the + // slot is reused. + this._collapseSlot(entry, slot); entry.freeSlots.push(slot); } /** - * Write the instance matrix of `slot`. Copies into a scratch matrix and marks - * the entry dirty; the caller flushes once per frame. + * Collapse a slot to zero scale (a zero instance matrix turns the triangle + * into a point, so it stops being drawn). Marks the entry dirty and clears + * the slot from the shadow sets so it stops driving the mesh's shadow flags. + */ + private _collapseSlot(entry: InstanceEntry, slot: integer): void { + const scratch = + this._scratchMatrix || (this._scratchMatrix = new THREE.Matrix4()); + scratch.makeScale(0, 0, 0); + entry.instancedMesh.setMatrixAt(slot, scratch); + entry.isMatrixDirty = true; + entry.castShadowSlots.delete(slot); + entry.receiveShadowSlots.delete(slot); + entry.instancedMesh.castShadow = entry.castShadowSlots.size > 0; + entry.instancedMesh.receiveShadow = entry.receiveShadowSlots.size > 0; + } + + /** Collapse a live slot without releasing it (used by hide/cull). */ + collapseSlot(key: string, slot: integer): void { + const entry = this._entries.get(key); + if (!entry) return; + if (slot < 0 || slot >= entry.usedSlotCount) return; + this._collapseSlot(entry, slot); + } + + /** + * Record which slots cast/receive shadows and update the `InstancedMesh`'s + * mesh-level shadow flags (the mesh casts if any live slot does). + */ + setSlotShadows( + key: string, + slot: integer, + castShadow: boolean, + receiveShadow: boolean + ): void { + const entry = this._entries.get(key); + if (!entry) return; + if (slot < 0 || slot >= entry.usedSlotCount) return; + if (castShadow) entry.castShadowSlots.add(slot); + else entry.castShadowSlots.delete(slot); + if (receiveShadow) entry.receiveShadowSlots.add(slot); + else entry.receiveShadowSlots.delete(slot); + entry.instancedMesh.castShadow = entry.castShadowSlots.size > 0; + entry.instancedMesh.receiveShadow = entry.receiveShadowSlots.size > 0; + } + + /** + * Write the instance matrix of `slot` (passed straight through, no copy); + * the caller flushes once per frame. */ setMatrix(key: string, slot: integer, matrix: THREE.Matrix4): void { const entry = this._entries.get(key); if (!entry) return; if (slot < 0 || slot >= entry.instancedMesh.count) return; - this._temporaryMatrix.copy(matrix); - entry.instancedMesh.setMatrixAt(slot, this._temporaryMatrix); + entry.instancedMesh.setMatrixAt(slot, matrix); entry.isMatrixDirty = true; } @@ -116,11 +191,7 @@ namespace gdjs { * once per frame, after all the objects of the frame wrote their matrices. */ flush(): void { - this._entries.forEach((entry) => { - if (!entry.isMatrixDirty) return; - entry.instancedMesh.instanceMatrix.needsUpdate = true; - entry.isMatrixDirty = false; - }); + this._entries.forEach(this._flushEntry); } /** The `InstancedMesh` of a key, for a caller that needs to check or scene it. */ @@ -131,9 +202,7 @@ namespace gdjs { /** The keys currently held, in insertion order. */ getKeys(): Array { - const keys: Array = []; - this._entries.forEach((entry, key) => keys.push(key)); - return keys; + return Array.from(this._entries.keys()); } /** Release every mesh this pool created and forget every key. */ @@ -149,13 +218,19 @@ namespace gdjs { } /** - * The pool key of one Model3D instance: the model resource, the material type and - * the layer it is rendered on. Two objects with the same key can share one + * The pool key of one Model3D instance: the model resource, the material type + * and the layer it is rendered on. Two objects with the same key can share one * `InstancedMesh` because their meshes and material are interchangeable. + * + * Each part is `JSON.stringify`-escaped so the comma-joined key is injective + * (a name containing `,` or `|` cannot collide with another triple). */ export const getModelInstanceKey = ( modelResourceName: string, materialType: string, layerName: string - ): string => `${modelResourceName}|${materialType}|${layerName}`; + ): string => + `${JSON.stringify(modelResourceName)},${JSON.stringify( + materialType + )},${JSON.stringify(layerName)}`; } diff --git a/Extensions/3D/Model3DObjectConfiguration.cpp b/Extensions/3D/Model3DObjectConfiguration.cpp index 117936da8..74282350c 100644 --- a/Extensions/3D/Model3DObjectConfiguration.cpp +++ b/Extensions/3D/Model3DObjectConfiguration.cpp @@ -283,6 +283,7 @@ void Model3DObjectConfiguration::DoUnserializeFrom( crossfadeDuration = content.GetDoubleAttribute("crossfadeDuration"); isCastingShadow = content.GetBoolAttribute("isCastingShadow"); isReceivingShadow = content.GetBoolAttribute("isReceivingShadow"); + useInstancing = content.GetBoolAttribute("useInstancing", false); RemoveAllAnimations(); auto &animationsElement = content.GetChild("animations"); @@ -314,6 +315,7 @@ void Model3DObjectConfiguration::DoSerializeTo( content.SetAttribute("crossfadeDuration", crossfadeDuration); content.SetAttribute("isCastingShadow", isCastingShadow); content.SetAttribute("isReceivingShadow", isReceivingShadow); + content.SetAttribute("useInstancing", useInstancing); auto &animationsElement = content.AddChild("animations"); animationsElement.ConsiderAsArrayOf("animation"); diff --git a/Extensions/3D/Model3DObjectConfiguration.h b/Extensions/3D/Model3DObjectConfiguration.h index c4a51b4f3..3d6c20f87 100644 --- a/Extensions/3D/Model3DObjectConfiguration.h +++ b/Extensions/3D/Model3DObjectConfiguration.h @@ -162,6 +162,7 @@ public: bool shouldKeepAspectRatio() const { return keepAspectRatio; }; bool shouldCastShadow() const { return isCastingShadow; }; bool shouldReceiveShadow() const { return isReceivingShadow; }; + bool shouldUseInstancing() const { return useInstancing; }; ///@} protected: @@ -188,8 +189,6 @@ private: bool isReceivingShadow; bool useInstancing; - bool shouldUseInstancing() const { return useInstancing; }; - std::vector animations; static Model3DAnimation badAnimation; //< Bad animation when an out of bound // animation is requested. diff --git a/Extensions/3D/Model3DRuntimeObject.ts b/Extensions/3D/Model3DRuntimeObject.ts index 7a58fb8b4..b9fb851a1 100644 --- a/Extensions/3D/Model3DRuntimeObject.ts +++ b/Extensions/3D/Model3DRuntimeObject.ts @@ -528,6 +528,17 @@ namespace gdjs { } super.onDeletedFromScene(); } + + /** + * Moving to another layer re-acquires the instance slot from that layer's + * pool (the pool key embeds the layer name). + */ + setLayer(layerName: string): void { + const wasInstanced = this._renderer.getInstanceSlot() !== null; + if (wasInstanced) this._renderer.releaseInstancing(); + super.setLayer(layerName); + if (wasInstanced) this._renderer.trySetupInstancing(); + } } /** @category Objects > 3D Model */ diff --git a/Extensions/3D/Model3DRuntimeObject3DRenderer.ts b/Extensions/3D/Model3DRuntimeObject3DRenderer.ts index 0025436a9..16798ecbc 100644 --- a/Extensions/3D/Model3DRuntimeObject3DRenderer.ts +++ b/Extensions/3D/Model3DRuntimeObject3DRenderer.ts @@ -89,6 +89,11 @@ namespace gdjs { */ private _instanceSlot: integer | null = null; private _instanceKey: string | null = null; + /** + * Bakes everything after the carrier (stretch/centering + the model's own + * node transforms) into one matrix so the instance matrix matches the clone. + */ + private _correctionMatrix: THREE.Matrix4 | null = null; private _layerRenderer: gdjs.LayerPixiRenderer | null = null; private static _instanceMatrix: THREE.Matrix4 | null = null; @@ -151,6 +156,15 @@ namespace gdjs { ) { return false; } + // An animated model morphs its vertices over time, which a single shared + // instance matrix cannot express. Skins/clips/morphs never take this path. + if ( + this._originalModel && + this._originalModel.animations && + this._originalModel.animations.length > 0 + ) { + return false; + } return !!this._getSingleMesh(); } @@ -164,6 +178,7 @@ namespace gdjs { let found: THREE.Mesh | null = null; let meshCount = 0; let skippedSkinnedMesh = false; + let hasMorphTargets = false; // An empty group contributes nothing; a mesh (or a mesh-less leaf) is one // drawable, so anything but exactly one drawable means no shared geometry. const visit = (node: THREE.Object3D) => { @@ -173,9 +188,16 @@ namespace gdjs { skippedSkinnedMesh = true; return; } + if ( + maybeMesh.morphTargetInfluences && + maybeMesh.morphTargetInfluences.length > 0 + ) { + hasMorphTargets = true; + } meshCount++; if (meshCount === 1) found = maybeMesh; - return; + // Fall through: a mesh node with child meshes is more than one + // drawable and must be counted as such (rejected below). } if (node.children.length === 0) { // A leaf that is not a mesh (a light, a camera, an empty). @@ -184,7 +206,7 @@ namespace gdjs { for (const child of node.children) visit(child); }; visit(scene); - if (skippedSkinnedMesh || meshCount !== 1) return null; + if (skippedSkinnedMesh || hasMorphTargets || meshCount !== 1) return null; return found; } @@ -202,7 +224,7 @@ namespace gdjs { * Returns false when instancing is not applicable, in which case the caller * keeps the clone path. */ - private _trySetupInstancing(): boolean { + trySetupInstancing(): boolean { if (!this._canUseInstancing()) return false; const mesh = this._getSingleMesh(); if (!mesh) return false; @@ -241,6 +263,16 @@ namespace gdjs { }); this._instanceKey = key; + // The clone draws `carrier * threeObject(stretch/centering) * mesh`; bake + // everything after the carrier into one correction matrix so the instance + // matrix (`carrier * correction`) matches the clone exactly. + this._originalModel.scene.updateMatrixWorld(true); + this._threeObject.updateMatrix(); + this._correctionMatrix = new THREE.Matrix4().multiplyMatrices( + this._threeObject.matrix, + mesh.matrixWorld + ); + // The instanced mesh draws this object now: drop the per-object clone and // take the carrier out of the scene graph. The carrier is never added to // the scene while instancing is on - it exists only to carry the transform @@ -249,6 +281,11 @@ namespace gdjs { const carrier = this.get3DRendererObject(); carrier.clear(); layerRenderer.remove3DRendererObject(carrier); + + // The first write was previously lost because `_updateModel` writes the + // matrix before the slot exists; write it now that the slot is acquired. + this._writeInstanceMatrix(); + this._updateShadow(); return true; } @@ -263,6 +300,7 @@ namespace gdjs { const carrier = this.get3DRendererObject(); carrier.updateMatrix(); matrix.copy(carrier.matrix); + if (this._correctionMatrix) matrix.multiply(this._correctionMatrix); if (this._layerRenderer) { this._layerRenderer .getModelInstancePool() @@ -306,6 +344,29 @@ namespace gdjs { this._writeInstanceMatrix(); } + override updateRotation() { + super.updateRotation(); + this._writeInstanceMatrix(); + } + + /** + * On the instanced path the real draw is an `InstancedMesh` slot (the carrier + * is a detached transform node), so hide/cull collapses or restores the slot + * instead of setting `visible` on the carrier. + */ + protected override applyRenderVisibility(): void { + if (this._instanceKey !== null && this._instanceSlot !== null) { + const drawn = this._isVisible && !this._isCulled; + if (this._layerRenderer) { + const pool = this._layerRenderer.getModelInstancePool(); + if (!drawn) pool.collapseSlot(this._instanceKey, this._instanceSlot); + else this._writeInstanceMatrix(); + } + return; + } + super.applyRenderVisibility(); + } + getOriginPoint(): FloatPoint3D { //@ts-ignore const point: FloatPoint3D = gdjs.staticArray( @@ -558,7 +619,7 @@ namespace gdjs { // Last, so the clone is in place if instancing cannot be used: the // instanced path replaces it, and silently leaves it alone otherwise. this.releaseInstancing(); - this._trySetupInstancing(); + this.trySetupInstancing(); } /** @@ -587,6 +648,21 @@ namespace gdjs { } _updateShadow() { + if ( + this._instanceKey !== null && + this._instanceSlot !== null && + this._layerRenderer + ) { + this._layerRenderer + .getModelInstancePool() + .setSlotShadows( + this._instanceKey, + this._instanceSlot, + this._model3DRuntimeObject._isCastingShadow, + this._model3DRuntimeObject._isReceivingShadow + ); + return; + } this._threeObject.traverse((child) => { child.castShadow = this._model3DRuntimeObject._isCastingShadow; child.receiveShadow = this._model3DRuntimeObject._isReceivingShadow; diff --git a/Extensions/TileMap/tilemapruntimeobject-pixi-renderer.ts b/Extensions/TileMap/tilemapruntimeobject-pixi-renderer.ts index 7cd5e0cc0..eebf18dd6 100644 --- a/Extensions/TileMap/tilemapruntimeobject-pixi-renderer.ts +++ b/Extensions/TileMap/tilemapruntimeobject-pixi-renderer.ts @@ -8,12 +8,15 @@ namespace gdjs { * The whole map (`[0, dimX, 0, dimY]`) is returned whenever culling must not * be applied: * - the map is small (`dimX + dimY <= 100`): the maths is not worth paying for; + * - the tile map is nested in a custom object: its box is in the container's + * space while the camera corners are scene-space, so it is drawn in full + * rather than culled with a wrong window; * - the container has no scene or the scene has no such layer; * - the layer's 3D camera is tilted, because the axis-aligned tile-bounds * maths is only valid for an unrotated camera. * - * The camera is resolved from the **scene's** layer, so a tile map nested in - * a custom object is culled too - its own container has no camera. + * The camera is resolved from the **scene's** layer (a nested tile map's own + * container has no camera). * * The computed bounds are clamped to `[0, dimX] x [0, dimY]`: a camera outside * the map must not produce out-of-range bounds that differ every frame and so @@ -38,6 +41,20 @@ namespace gdjs { const instanceContainer = object.getInstanceContainer(); const scene = instanceContainer.getScene(); + // Nested tile map: scene-space camera corners vs a container-space grid. + // Draw it in full rather than cull with a wrong window. + if (instanceContainer !== scene) return result; + // No loaded tile map yet: the grid lookup would map both corners to [0,0]. + if (!object.getTileMap()) return result; + // The axis-aligned tile bounds maths is only valid for an unrotated, + // unmirrored map (the transform uses |scale| and samples only two corners). + if ( + object.getAngle() !== 0 || + object.getScaleX() < 0 || + object.getScaleY() < 0 + ) { + return result; + } const layerName = object.getLayer(); const layer = scene ? scene.getLayer(layerName) : null; if (!layer) return result; @@ -71,22 +88,23 @@ namespace gdjs { cameraY + cameraHalfHeight ); - // Clamp to the map. + // Clamp to the map, expanded one tile on every side so a partially-visible + // tile at the window edge is not cut off. result[0] = Math.max( 0, - Math.min(dimX, Math.min(cameraLeftTile, cameraRightTile)) + Math.min(dimX, Math.min(cameraLeftTile, cameraRightTile) - 1) ); result[1] = Math.max( 0, - Math.min(dimX, Math.max(cameraLeftTile, cameraRightTile) + 1) + Math.min(dimX, Math.max(cameraLeftTile, cameraRightTile) + 2) ); result[2] = Math.max( 0, - Math.min(dimY, Math.min(cameraTopTile, cameraBottomTile)) + Math.min(dimY, Math.min(cameraTopTile, cameraBottomTile) - 1) ); result[3] = Math.max( 0, - Math.min(dimY, Math.max(cameraTopTile, cameraBottomTile) + 1) + Math.min(dimY, Math.max(cameraTopTile, cameraBottomTile) + 2) ); return result; }; diff --git a/GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts b/GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts index 2fa7cef82..bc0a02889 100644 --- a/GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts +++ b/GDJS/Runtime/CustomRuntimeObjectInstanceContainer.ts @@ -266,24 +266,18 @@ namespace gdjs { const object = allInstancesList[i]; // 3D objects report no `getRendererObject()` (see - // `RuntimeObject3D.getRendererObject`), so they need their own culling. - // A custom object has no camera of its own: resolve the scene's, for the - // layer the 3D object is on. + // `RuntimeObject3D.getRendererObject`), so they need their own + // visibility handling. // // Both 3D object families are caught by the capability, not by a base // class: `CustomRuntimeObject3D` does not extend `RuntimeObject3D`. const isThreeD = gdjs.CullableRuntimeObject3D.isCullable3D(object); if (isThreeD) { - const scene = this.getScene(); - const layer = scene ? scene.getLayer(object.getLayer()) : null; - const layerRenderer = layer ? layer.getRenderer() : null; - const frustum = layerRenderer - ? layerRenderer.getThreeFrustum() - : null; - const isVisible = - !object.isHidden() && (!frustum || object.isInFrustum(frustum)); - const threeObject = object.get3DRendererObject(); - if (threeObject) threeObject.visible = isVisible; + // A 3D child's box is in the custom object's container space, but the + // frustum is in scene space: cull nothing here (see + // AI_GAME_STUDIO_ASSESSMENT follow-up for container-space frustums). + object.setCullingVisible(false); + const isVisible = !object.isHidden(); if (isVisible) { this.getGame() diff --git a/GDJS/Runtime/RuntimeInstanceContainer.ts b/GDJS/Runtime/RuntimeInstanceContainer.ts index 28eed3b12..53e98637a 100644 --- a/GDJS/Runtime/RuntimeInstanceContainer.ts +++ b/GDJS/Runtime/RuntimeInstanceContainer.ts @@ -493,14 +493,9 @@ namespace gdjs { */ _updateObjectsPreRender() { const allInstancesList = this.getAdhocListOfAllInstances(); - // No 3D culling here: a plain instance container has no camera of its own, - // so 3D culling is only defined where a camera exists - on the scene - // (`RuntimeScene._updateObjectsPreRender`) and inside a custom object, - // which resolves the scene's camera for the object's layer - // (`CustomRuntimeObjectInstanceContainer._updateObjectsPreRender`). - // `RuntimeScene` overrides this method with the culling 2D and 3D objects - // both need; this implementation is the first-frame path for a scene and - // the every-frame path for a container without a camera. + // RuntimeScene overrides this with the culling both 2D and 3D objects + // need; this implementation runs only for containers without their own + // culling (custom object containers override it too). for (let i = 0, len = allInstancesList.length; i < len; ++i) { const object = allInstancesList[i]; const rendererObject = object.getRendererObject(); diff --git a/GDJS/Runtime/pixi-renderers/layer-pixi-renderer.ts b/GDJS/Runtime/pixi-renderers/layer-pixi-renderer.ts index afee82ad6..0f0b3e2fb 100644 --- a/GDJS/Runtime/pixi-renderers/layer-pixi-renderer.ts +++ b/GDJS/Runtime/pixi-renderers/layer-pixi-renderer.ts @@ -223,8 +223,25 @@ namespace gdjs { */ private _threeFrustum: THREE.Frustum | null = null; private _threeFrustumMatrix: THREE.Matrix4 | null = null; - /** Everything the frustum depends on, so it is rebuilt only on a change. */ - private _threeFrustumSignature: string | null = null; + /** + * Everything the frustum depends on, so it is rebuilt only on a change. + * Compared field by field - never concatenated - to avoid a per-object + * per-frame string allocation. `getCameraZ` is included so a camera depth + * change also invalidates the cached frustum. + */ + private _frustumSigValid: boolean = false; + private _frustumSigCameraX: float = 0; + private _frustumSigCameraY: float = 0; + private _frustumSigCameraZ: float = 0; + private _frustumSigCameraRotation: float = 0; + private _frustumSigCameraZoom: float = 0; + private _frustumSigWidth: float = 0; + private _frustumSigHeight: float = 0; + private _frustumSigRotX: float = 0; + private _frustumSigRotY: float = 0; + private _frustumSigFov: float = 0; + private _frustumSigNear: float = 0; + private _frustumSigFar: float = 0; // For a 2D+3D layer, the 2D rendering is done on the render texture // and then must be displayed on a plane in the 3D world: @@ -344,6 +361,14 @@ namespace gdjs { return this._modelInstancePool; } + /** Release the Model3D instance pool (called when the scene is unloaded). */ + disposeModelInstancePool(): void { + if (this._modelInstancePool) { + this._modelInstancePool.dispose(); + this._modelInstancePool = null; + } + } + /** The 3D group this layer renders its 3D objects into. */ getThreeGroup(): THREE.Group | null { return this._threeGroup; @@ -390,33 +415,36 @@ namespace gdjs { const camera = this._threeCamera; if (!camera) return null; - // A cheap signature of everything the frustum depends on. - const cameraSignature = - this._layer.getCameraX() + - '|' + - this._layer.getCameraY() + - '|' + - this._layer.getCameraRotation() + - '|' + - this._layer.getCameraZoom() + - '|' + - this._layer.getWidth() + - '|' + - this._layer.getHeight() + - '|' + - camera.rotation.x + - '|' + - camera.rotation.y + - '|' + - camera.fov + - '|' + - camera.near + - '|' + - camera.far; + // Everything the frustum depends on. Compared field by field (never + // concatenated) to avoid a per-object per-frame string allocation. + const cameraX = this._layer.getCameraX(); + const cameraY = this._layer.getCameraY(); + const cameraZ = this._layer.getCameraZ(null); + const cameraRotation = this._layer.getCameraRotation(); + const cameraZoom = this._layer.getCameraZoom(); + const width = this._layer.getWidth(); + const height = this._layer.getHeight(); + const rotX = camera.rotation.x; + const rotY = camera.rotation.y; + const fov = camera instanceof THREE.PerspectiveCamera ? camera.fov : 0; + const near = camera.near; + const far = camera.far; if ( this._threeFrustum && - this._threeFrustumSignature === cameraSignature + this._frustumSigValid && + this._frustumSigCameraX === cameraX && + this._frustumSigCameraY === cameraY && + this._frustumSigCameraZ === cameraZ && + this._frustumSigCameraRotation === cameraRotation && + this._frustumSigCameraZoom === cameraZoom && + this._frustumSigWidth === width && + this._frustumSigHeight === height && + this._frustumSigRotX === rotX && + this._frustumSigRotY === rotY && + this._frustumSigFov === fov && + this._frustumSigNear === near && + this._frustumSigFar === far ) { return this._threeFrustum; } @@ -429,13 +457,37 @@ namespace gdjs { } this._update3DCameraFromLayer(); + if (this._threeCameraDirty) { + camera.updateProjectionMatrix(); + this._threeCameraDirty = false; + } camera.updateMatrixWorld(); + if (this._threeScene) this._threeScene.updateMatrixWorld(); this._threeFrustumMatrix.multiplyMatrices( camera.projectionMatrix, camera.matrixWorldInverse ); + // Right-multiply by the group's world matrix to map the frustum into the + // space of the boxes built by `gdjs.Object3DCulling.computeAABB` (the + // group's world matrix carries the `_threeScene` Y mirror). + if (this._threeGroup) { + this._threeFrustumMatrix.multiply(this._threeGroup.matrixWorld); + } this._threeFrustum.setFromProjectionMatrix(this._threeFrustumMatrix); - this._threeFrustumSignature = cameraSignature; + + this._frustumSigCameraX = cameraX; + this._frustumSigCameraY = cameraY; + this._frustumSigCameraZ = cameraZ; + this._frustumSigCameraRotation = cameraRotation; + this._frustumSigCameraZoom = cameraZoom; + this._frustumSigWidth = width; + this._frustumSigHeight = height; + this._frustumSigRotX = rotX; + this._frustumSigRotY = rotY; + this._frustumSigFov = fov; + this._frustumSigNear = near; + this._frustumSigFar = far; + this._frustumSigValid = true; return this._threeFrustum; } diff --git a/GDJS/Runtime/runtimeobject.ts b/GDJS/Runtime/runtimeobject.ts index cb1c3691a..eb5e6ab99 100644 --- a/GDJS/Runtime/runtimeobject.ts +++ b/GDJS/Runtime/runtimeobject.ts @@ -72,6 +72,12 @@ namespace gdjs { getAABB3D(box: THREE.Box3): THREE.Box3; /** True when the object's box intersects `frustum`. */ isInFrustum(frustum: THREE.Frustum): boolean; + /** + * Hide/show the object because it left/entered the camera frustum. Routed + * through the renderer (not a detached three.js node) so instanced models + * can collapse the hidden instance's slot. + */ + setCullingVisible(culled: boolean): void; } /** @category Objects > 3D Objects */ @@ -98,6 +104,10 @@ namespace gdjs { getRotationX(): float; getRotationY(): float; angle: float; + /** Scene-space rotation center (not always the box center). */ + getCenterXInScene(): float; + getCenterYInScene(): float; + getCenterZInScene(): float; }; /** @@ -106,10 +116,14 @@ namespace gdjs { * @category Objects > 3D Objects */ export namespace Object3DCulling { - /** Scratch objects: never returned to callers, never read across calls. */ - const temporaryBox = new THREE.Box3(); - const temporaryMatrix = new THREE.Matrix4(); - const temporaryEuler = new THREE.Euler(); + /** + * Scratch objects: never returned to callers, never read across calls. + * Allocated lazily at first use so that 2D-only games - which never load + * `three.js` - can still boot this namespace (it is included unconditionally). + */ + let temporaryBox: THREE.Box3 | null = null; + let temporaryMatrix: THREE.Matrix4 | null = null; + let temporaryEuler: THREE.Euler | null = null; /** * Write the object's conservative axis-aligned box into `box` and return it. @@ -120,18 +134,26 @@ namespace gdjs { * conservative, not tight - a non-axis-aligned rotation yields a larger box * than the object's silhouette, which is the safe direction for culling. * + * The box is rotated around the object's rotation center + * (`getCenter*InScene`), which is not always the box center (e.g. a + * `Model3D` whose origin is at its box corner). A negative size is + * normalized so it can never invert the box. + * * Allocates nothing. */ export const computeAABB = ( object: Object3DBoxSource, box: THREE.Box3 ): THREE.Box3 => { - const halfWidth = object.getWidth() / 2; - const halfHeight = object.getHeight() / 2; - const halfDepth = object.getDepth() / 2; - const centerX = object.getDrawableX() + halfWidth; - const centerY = object.getDrawableY() + halfHeight; - const centerZ = object.getDrawableZ() + halfDepth; + const width = object.getWidth(); + const height = object.getHeight(); + const depth = object.getDepth(); + const halfWidth = Math.abs(width) / 2; + const halfHeight = Math.abs(height) / 2; + const halfDepth = Math.abs(depth) / 2; + const centerX = object.getDrawableX() + width / 2; + const centerY = object.getDrawableY() + height / 2; + const centerZ = object.getDrawableZ() + depth / 2; if ( object.getRotationX() === 0 && @@ -154,14 +176,16 @@ namespace gdjs { // Half-extents of the rotated box: for each world axis, the sum of the // absolute contributions of the three local half-extents. - temporaryEuler.set( + const matrix = temporaryMatrix || (temporaryMatrix = new THREE.Matrix4()); + const euler = temporaryEuler || (temporaryEuler = new THREE.Euler()); + euler.set( gdjs.toRad(object.getRotationX()), gdjs.toRad(object.getRotationY()), gdjs.toRad(object.angle), 'ZYX' ); - temporaryMatrix.makeRotationFromEuler(temporaryEuler); - const elements = temporaryMatrix.elements; + matrix.makeRotationFromEuler(euler); + const elements = matrix.elements; const halfExtentX = Math.abs(elements[0]) * halfWidth + Math.abs(elements[4]) * halfHeight + @@ -175,15 +199,39 @@ namespace gdjs { Math.abs(elements[6]) * halfHeight + Math.abs(elements[10]) * halfDepth; + // Rotate the box center around the object's rotation center: the box sits + // at C' = P + R*(C - P), where P is the rotation pivot and C the box center. + const pivotX = object.getCenterXInScene(); + const pivotY = object.getCenterYInScene(); + const pivotZ = object.getCenterZInScene(); + const offsetX = centerX - pivotX; + const offsetY = centerY - pivotY; + const offsetZ = centerZ - pivotZ; + const rotatedCenterX = + pivotX + + elements[0] * offsetX + + elements[4] * offsetY + + elements[8] * offsetZ; + const rotatedCenterY = + pivotY + + elements[1] * offsetX + + elements[5] * offsetY + + elements[9] * offsetZ; + const rotatedCenterZ = + pivotZ + + elements[2] * offsetX + + elements[6] * offsetY + + elements[10] * offsetZ; + box.min.set( - centerX - halfExtentX, - centerY - halfExtentY, - centerZ - halfExtentZ + rotatedCenterX - halfExtentX, + rotatedCenterY - halfExtentY, + rotatedCenterZ - halfExtentZ ); box.max.set( - centerX + halfExtentX, - centerY + halfExtentY, - centerZ + halfExtentZ + rotatedCenterX + halfExtentX, + rotatedCenterY + halfExtentY, + rotatedCenterZ + halfExtentZ ); return box; }; @@ -196,8 +244,9 @@ namespace gdjs { object: Object3DBoxSource, frustum: THREE.Frustum ): boolean => { - computeAABB(object, temporaryBox); - return frustum.intersectsBox(temporaryBox); + const scratchBox = temporaryBox || (temporaryBox = new THREE.Box3()); + computeAABB(object, scratchBox); + return frustum.intersectsBox(scratchBox); }; } diff --git a/GDJS/Runtime/runtimescene.ts b/GDJS/Runtime/runtimescene.ts index 4a04e0f2a..b124a3789 100644 --- a/GDJS/Runtime/runtimescene.ts +++ b/GDJS/Runtime/runtimescene.ts @@ -312,6 +312,14 @@ namespace gdjs { object.onDestroyed(); } + // Release each layer's Model3D instance pool so a reloaded scene does not + // leak shared InstancedMeshes. + for (const name in this._layers.items) { + if (this._layers.items.hasOwnProperty(name)) { + this._layers.items[name].getRenderer().disposeModelInstancePool(); + } + } + // Notify the renderer if (this._renderer) { this._renderer.onSceneUnloaded(); @@ -542,10 +550,9 @@ namespace gdjs { const frustum = layerRenderer ? layerRenderer.getThreeFrustum() : null; - const isVisible = - !object.isHidden() && (!frustum || object.isInFrustum(frustum)); - const threeObject = object.get3DRendererObject(); - if (threeObject) threeObject.visible = isVisible; + const culled = !!frustum && !object.isInFrustum(frustum); + object.setCullingVisible(culled); + const isVisible = !object.isHidden() && !culled; // Update effects and perform the pre-render update only if the // object is visible, like the 2D path below. diff --git a/GDJS/tests/karma.conf.js b/GDJS/tests/karma.conf.js index 955184113..9fbf03385 100644 --- a/GDJS/tests/karma.conf.js +++ b/GDJS/tests/karma.conf.js @@ -172,10 +172,10 @@ module.exports = function (config) { './newIDE/app/resources/GDJS/Runtime/Extensions/TextObject/textruntimeobject-pixi-renderer.js', './newIDE/app/resources/GDJS/Runtime/Extensions/3D/A_RuntimeObject3D.js', './newIDE/app/resources/GDJS/Runtime/Extensions/3D/A_RuntimeObject3DRenderer.js', - // Loaded before the object that uses them. + // Mirror Extensions/3D/JsExtension.js addIncludeFile order. './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Scene3DTools.js', - './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Model3DInstancePool.js', './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Model3DRuntimeObject.js', + './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Model3DInstancePool.js', './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Model3DRuntimeObject3DRenderer.js', './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Cube3DRuntimeObject.js', './newIDE/app/resources/GDJS/Runtime/Extensions/3D/Cube3DRuntimeObjectPixiRenderer.js', diff --git a/GDJS/tests/tests/Extensions/3DCulling.js b/GDJS/tests/tests/Extensions/3DCulling.js index f02e018b1..5f292ebbc 100644 --- a/GDJS/tests/tests/Extensions/3DCulling.js +++ b/GDJS/tests/tests/Extensions/3DCulling.js @@ -1,4 +1,3 @@ -// @ts-check // @ts-nocheck The expect.js assertions take an optional message argument, which // the installed `expect` typings do not declare. diff --git a/GDJS/tests/tests/Extensions/Model3DInstancing.js b/GDJS/tests/tests/Extensions/Model3DInstancing.js index 62c361f0e..a1011b233 100644 --- a/GDJS/tests/tests/Extensions/Model3DInstancing.js +++ b/GDJS/tests/tests/Extensions/Model3DInstancing.js @@ -1,7 +1,6 @@ -// @ts-check // @ts-nocheck The runtime types do not describe the internals these tests drive -// (the instance pool slot, the resource manager registry), which the built -// runtime does expose. +// (the instance pool, the resource manager registry), which the built runtime +// does expose. describe('Model3D instancing', function () { /** The single-mesh model shipped for these tests. */ @@ -29,21 +28,23 @@ describe('Model3D instancing', function () { return runtimeGame; }; + const make3DLayerData = (name) => ({ + name, + renderingType: '3d', + cameraType: 'perspective', + visibility: true, + cameras: [], + effects: [], + ambientLightColorR: 255, + ambientLightColorG: 255, + ambientLightColorB: 255, + isLightingLayer: false, + followBaseLayerCamera: false, + }); + const createSceneWith3DLayer = (runtimeGame) => { const runtimeScene = new gdjs.RuntimeScene(runtimeGame); - runtimeScene.addLayer({ - name: '', - renderingType: '3d', - cameraType: 'perspective', - visibility: true, - cameras: [], - effects: [], - ambientLightColorR: 255, - ambientLightColorG: 255, - ambientLightColorB: 255, - isLightingLayer: false, - followBaseLayerCamera: false, - }); + runtimeScene.addLayer(make3DLayerData('')); return runtimeScene; }; @@ -104,79 +105,148 @@ describe('Model3D instancing', function () { const countGroups = (runtimeScene) => getThreeChildren(runtimeScene).filter((child) => child.isGroup).length; - it('shares one InstancedMesh between instanced models and keeps the clone path for the others', async () => { + const getPoolKey = (runtimeScene) => getLayerPool(runtimeScene).getKeys()[0]; + + it('shares one InstancedMesh between same-key models and draws no clones', async () => { const runtimeGame = await makeGameWithSingleMeshModel(); const runtimeScene = createSceneWith3DLayer(runtimeGame); - const first = createModel3D(runtimeScene, 'First', true); - const second = createModel3D(runtimeScene, 'Second', true); - const third = createModel3D(runtimeScene, 'Third', true); - // Not opted in: must keep the per-object clone. - const cloned = createModel3D(runtimeScene, 'Cloned', false); - + createModel3D(runtimeScene, 'First', true); + createModel3D(runtimeScene, 'Second', true); runtimeScene.render(); - // the three instanced models share one InstancedMesh + // (i) one InstancedMesh in the layer group and no per-object clones. expect(countInstancedMeshes(runtimeScene)).to.be(1); - // the fourth, non-instanced model still gets its own clone - expect(countGroups(runtimeScene)).to.be(1); - - // moving the first instance rewrites its matrix - expect(getLayerPool(runtimeScene).getKeys().length).to.be(1); - expect(first.getRenderer().getInstanceSlot()).to.be(0); - expect(second.getRenderer().getInstanceSlot()).to.be(1); - expect(third.getRenderer().getInstanceSlot()).to.be(2); - expect(cloned.getRenderer().getInstanceSlot()).to.be(null); + expect(countGroups(runtimeScene)).to.be(0); }); - it('writes the instance matrix of each instance when it moves', async () => { + it('grows past the initial capacity without throwing or losing matrices', async () => { + const runtimeGame = await makeGameWithSingleMeshModel(); + const runtimeScene = createSceneWith3DLayer(runtimeGame); + + // 16 fill the initial capacity; the 17th grows the mesh. + for (let i = 0; i < 16; i++) { + const object = createModel3D(runtimeScene, 'O' + i, true); + object.setX(i * 1000); + } + runtimeScene.render(); + + // (ii) the 17th acquire grows the mesh (pre-fix: `TypeError` on `null.add`). + const seventeenth = createModel3D(runtimeScene, 'O16', true); + seventeenth.setX(16000); + + // Slots 0..15 keep the matrices written before the grow. + const instancedMesh = getLayerPool(runtimeScene).getInstancedMesh( + getPoolKey(runtimeScene) + ); + expect(instancedMesh).to.be.ok(); + const matrix = new THREE.Matrix4(); + for (let slot = 0; slot < 16; slot++) { + instancedMesh.getMatrixAt(slot, matrix); + // A lost/collapsed slot would have a zero scale; the grow copies it as-is. + expect(matrix.elements[0]).to.not.be(0); + } + }); + + it('collapses the slot of a deleted model to zero scale', async () => { const runtimeGame = await makeGameWithSingleMeshModel(); const runtimeScene = createSceneWith3DLayer(runtimeGame); const first = createModel3D(runtimeScene, 'First', true); - const second = createModel3D(runtimeScene, 'Second', true); + createModel3D(runtimeScene, 'Second', true); runtimeScene.render(); - const key = getLayerPool(runtimeScene).getKeys()[0]; - const instancedMesh = getLayerPool(runtimeScene).getInstancedMesh(key); - expect(instancedMesh).to.be.ok(); - + const instancedMesh = getLayerPool(runtimeScene).getInstancedMesh( + getPoolKey(runtimeScene) + ); const matrix = new THREE.Matrix4(); instancedMesh.getMatrixAt(0, matrix); - const firstX = matrix.elements[12]; + expect(matrix.elements[0]).to.not.be(0); - first.setX(1000); + // (iii) deleting the first model collapses its slot (pre-fix: drawn forever). + runtimeScene.markObjectForDeletion(first); runtimeScene.render(); instancedMesh.getMatrixAt(0, matrix); - expect(matrix.elements[12]).to.be.above(firstX); - - instancedMesh.getMatrixAt(1, matrix); - const secondX = matrix.elements[12]; - second.setX(2000); - runtimeScene.render(); - instancedMesh.getMatrixAt(1, matrix); - // the freed slot 0 is handed to the next instanced model - expect(matrix.elements[12]).to.be.above(secondX); + expect(matrix.elements[0]).to.be(0); }); - it('frees a slot when an instanced model leaves the scene, for reuse', async () => { + it('rewrites the instance matrix on rotation', async () => { const runtimeGame = await makeGameWithSingleMeshModel(); const runtimeScene = createSceneWith3DLayer(runtimeGame); - const first = createModel3D(runtimeScene, 'First', true); - const second = createModel3D(runtimeScene, 'Second', true); + const object = createModel3D(runtimeScene, 'First', true); runtimeScene.render(); - expect(first.getRenderer().getInstanceSlot()).to.be(0); - expect(second.getRenderer().getInstanceSlot()).to.be(1); + const instancedMesh = getLayerPool(runtimeScene).getInstancedMesh( + getPoolKey(runtimeScene) + ); + const matrix = new THREE.Matrix4(); + instancedMesh.getMatrixAt(0, matrix); + const before = matrix.elements[0]; - // Removing the first object gives slot 0 back. - runtimeScene.markObjectForDeletion(first); + // (iv) a rotation reaches the instance matrix (pre-fix: unchanged). + object.setAngle(Math.PI / 2); + runtimeScene.render(); + instancedMesh.getMatrixAt(0, matrix); + expect(matrix.elements[0]).to.not.be(before); + }); + + it('collapses the slot on hide and restores it on unhide', async () => { + const runtimeGame = await makeGameWithSingleMeshModel(); + const runtimeScene = createSceneWith3DLayer(runtimeGame); + + const object = createModel3D(runtimeScene, 'First', true); runtimeScene.render(); - const third = createModel3D(runtimeScene, 'Third', true); + const instancedMesh = getLayerPool(runtimeScene).getInstancedMesh( + getPoolKey(runtimeScene) + ); + const matrix = new THREE.Matrix4(); + + // (v) hide collapses the slot; unhide restores the transform matrix. + object.hide(true); runtimeScene.render(); - expect(third.getRenderer().getInstanceSlot()).to.be(0); + instancedMesh.getMatrixAt(0, matrix); + expect(matrix.elements[0]).to.be(0); + + object.hide(false); + runtimeScene.render(); + instancedMesh.getMatrixAt(0, matrix); + expect(matrix.elements[0]).to.not.be(0); + }); + + it('moves the instance matrix to the new layer pool on a layer change', async () => { + const runtimeGame = await makeGameWithSingleMeshModel(); + const runtimeScene = new gdjs.RuntimeScene(runtimeGame); + runtimeScene.addLayer(make3DLayerData('')); + runtimeScene.addLayer(make3DLayerData('other')); + + const object = createModel3D(runtimeScene, 'First', true); + runtimeScene.render(); + + const sourceMesh = runtimeScene + .getLayer('') + .getRenderer() + .getModelInstancePool() + .getInstancedMesh( + runtimeScene.getLayer('').getRenderer().getModelInstancePool().getKeys()[0] + ); + const matrix = new THREE.Matrix4(); + sourceMesh.getMatrixAt(0, matrix); + const before = matrix.elements[0]; + + // (vi) the slot is re-acquired from the destination layer's pool. + object.setLayer('other'); + runtimeScene.render(); + + const targetPool = runtimeScene + .getLayer('other') + .getRenderer() + .getModelInstancePool(); + const targetMesh = targetPool.getInstancedMesh(targetPool.getKeys()[0]); + expect(targetMesh).to.be.ok(); + targetMesh.getMatrixAt(0, matrix); + expect(matrix.elements[0]).to.be(before); }); it('silently keeps the clone path for a Basic material', async () => { @@ -184,10 +254,10 @@ describe('Model3D instancing', function () { const runtimeScene = createSceneWith3DLayer(runtimeGame); // "Basic" replaces every material per instance: not shareable. - const object = createModel3D(runtimeScene, 'Basic', true, 'Basic'); + createModel3D(runtimeScene, 'Basic', true, 'Basic'); runtimeScene.render(); - expect(object.getRenderer().getInstanceSlot()).to.be(null); + // Materials/skins/animation clips never take the instanced path. expect(countInstancedMeshes(runtimeScene)).to.be(0); expect(countGroups(runtimeScene)).to.be(1); }); diff --git a/newIDE/app/src/AI/CustomAIClient.js b/newIDE/app/src/AI/CustomAIClient.js index 63e96a648..7f95d8f13 100644 --- a/newIDE/app/src/AI/CustomAIClient.js +++ b/newIDE/app/src/AI/CustomAIClient.js @@ -1004,7 +1004,7 @@ export const GDEVELOP_OPENAI_TOOLS: Array<{| description: 'List of task IDs this task depends on.', }, }, - required: ['id', 'title', 'status'], + required: ['id', 'title', 'description', 'status'], }, }, }, @@ -2260,10 +2260,15 @@ export const customAddMessageToAiRequest = async ({ }); } + // The role a studio sub-agent was spawned with must shape every turn, not + // just the first: keep its prompt and tool subset. + const studioRoleId = existing.studioRoleId || null; + const systemPrompt = buildSystemPrompt({ gameProjectJson, projectSpecificExtensionsSummaryJson, mode: mode || existing.mode, + role: studioRoleId, }); const openAiMessages = transformGDevelopMessagesToOpenAi( @@ -2273,7 +2278,9 @@ export const customAddMessageToAiRequest = async ({ const assistantResponse = await sendChatCompletion({ messages: openAiMessages, - tools: GDEVELOP_OPENAI_TOOLS, + tools: studioRoleId + ? getToolsForRole((studioRoleId: any), GDEVELOP_OPENAI_TOOLS) + : GDEVELOP_OPENAI_TOOLS, }); const assistantMsgId = `msg-asst-${Date.now()}`; @@ -2284,6 +2291,23 @@ export const customAddMessageToAiRequest = async ({ output.push(assistantMessage); const currentCachedRequest = localAiRequestsCache[aiRequestId] || existing; + // A suggestion or suspension write that landed while the model was + // answering must not be discarded (W5): re-insert cache-only messages + // before the turn's own additions. + const turnIds = new Set(output.map(message => message.messageId)); + const cacheOnlyMessages = (currentCachedRequest.output || []).filter( + message => message && !turnIds.has(message.messageId) + ); + let merged = output; + if (cacheOnlyMessages.length > 0) { + const appendedThisTurn = (userMessage && userMessage.trim() ? 1 : 0) + 1; + const turnStart = Math.max(0, output.length - appendedThisTurn); + merged = [ + ...output.slice(0, turnStart), + ...cacheOnlyMessages, + ...output.slice(turnStart), + ]; + } const updatedAiRequest: AiRequest = { // Read the cache again, inside the lock: a suspend or a suggestion write // that landed while the model was answering must not be discarded. @@ -2294,7 +2318,7 @@ export const customAddMessageToAiRequest = async ({ // can read what the agent produced before it was stopped. status: currentCachedRequest.status === 'suspended' ? 'suspended' : 'ready', - output, + output: merged, }; localAiRequestsCache[aiRequestId] = updatedAiRequest; @@ -2384,6 +2408,7 @@ export const customCreateSubAgentAiRequest = async ({ toolsVersion: 'v14', toolOptions: null, parentAiRequestId: parentAiRequestId, + studioRoleId: roleId, error: null, output, lastUserMessagePriceInCredits: 0, @@ -2459,18 +2484,41 @@ export const customSuspendAiRequest = (aiRequestId: string): AiRequest => { if (existing) { // Replace the cached request rather than mutating it in place: a model turn // may be holding a reference to it and will write its own copy back. + const now = new Date().toISOString(); const suspended: AiRequest = { ...existing, status: 'suspended', - updatedAt: new Date().toISOString(), + updatedAt: now, }; localAiRequestsCache[aiRequestId] = suspended; + // A parent Stop must not leave BYOK-burning children running: suspend every + // sub-agent of this request too. + for (const key of Object.keys(localAiRequestsCache)) { + const child = localAiRequestsCache[key]; + if (child && child.parentAiRequestId === aiRequestId) { + localAiRequestsCache[key] = { + ...child, + status: 'suspended', + updatedAt: now, + }; + } + } saveLocalAiRequests(); return suspended; } return customGetAiRequest(aiRequestId); }; +/** + * Client-side update write-through: a mutated request is written back to the + * local cache so plan flips and the loop guard's `status: 'error'` are not + * reverted by the next cache fetch. Content-idempotent. + */ +export const customUpdateAiRequest = (aiRequest: AiRequest): void => { + localAiRequestsCache[aiRequest.id] = aiRequest; + saveLocalAiRequests(); +}; + /** * Client-side Fork AI Request. */ diff --git a/newIDE/app/src/AI/CustomAIClient.spec.js b/newIDE/app/src/AI/CustomAIClient.spec.js index 961999d85..4ae5766e1 100644 --- a/newIDE/app/src/AI/CustomAIClient.spec.js +++ b/newIDE/app/src/AI/CustomAIClient.spec.js @@ -14,7 +14,9 @@ import { LOCAL_BYOK_USER_ID, _resetCustomAiClientForTesting, customCreateAiRequest, + customCreateSubAgentAiRequest, customAddMessageToAiRequest, + customUpdateAiRequest, customGetAiRequest, customGetAiRequests, customGetAiRequestStatuses, @@ -27,6 +29,8 @@ import { testConnection, } from './CustomAIClient'; +import { getToolsForRole } from '../AiGeneration/Studio/Roles'; + jest.mock('axios'); describe('CustomAIClient', () => { @@ -616,4 +620,94 @@ describe('CustomAIClient', () => { ); }); }); + + describe('Studio sub-agent role enforcement', () => { + /** Mock one assistant reply so a turn completes. */ + const mockAssistantReply = (content: string) => { + // $FlowFixMe + axios.post.mockResolvedValueOnce({ + status: 200, + data: { choices: [{ message: { role: 'assistant', content } }] }, + }); + }; + + const spawnTester = async () => { + mockAssistantReply('Spawned.'); + return customCreateSubAgentAiRequest({ + parentAiRequestId: 'local-ai-parent', + roleId: 'tester', + userRequest: 'Verify the grid.', + gameProjectJson: null, + projectSpecificExtensionsSummaryJson: null, + spawnContextNote: null, + }); + }; + + const sentToolNames = (callIndex: number) => { + // $FlowFixMe + const body = axios.post.mock.calls[callIndex][1]; + return { + names: (body.tools || []).map(tool => tool.function.name), + systemPrompt: body.messages[0].content, + }; + }; + + it('offers a spawned sub-agent only its role tools and prompt', async () => { + await spawnTester(); + const { names, systemPrompt } = sentToolNames(0); + const testerToolNames = getToolsForRole( + 'tester', + GDEVELOP_OPENAI_TOOLS + ).map(tool => tool.function.name); + expect(names.length).toBeGreaterThan(0); + names.forEach(name => expect(testerToolNames).toContain(name)); + expect(systemPrompt).toContain('QA tester'); + }); + + it('keeps the role tool subset and prompt on later turns', async () => { + const child = await spawnTester(); + + // A second turn - the critical regression: pre-fix this offered all the + // tools and dropped the role prompt. + mockAssistantReply('Done.'); + await customAddMessageToAiRequest({ + aiRequestId: child.id, + userMessage: '', + functionCallOutputs: [], + }); + + const { names, systemPrompt } = sentToolNames(1); + const testerToolNames = getToolsForRole( + 'tester', + GDEVELOP_OPENAI_TOOLS + ).map(tool => tool.function.name); + expect(names.length).toBeGreaterThan(0); + names.forEach(name => expect(testerToolNames).toContain(name)); + expect(systemPrompt).toContain('QA tester'); + }); + + it('writes a mutated request through to the cache', async () => { + const child = await spawnTester(); + const before = (child.output || []).length; + const marker: any = { + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + status: 'completed', + text: 'marker', + annotations: [], + }, + ], + messageId: 'msg-marker', + }; + customUpdateAiRequest({ + ...child, + output: [...(child.output || []), marker], + }); + expect(customGetAiRequest(child.id).output.length).toBe(before + 1); + }); + }); }); diff --git a/newIDE/app/src/AiGeneration/AiRequestContext.js b/newIDE/app/src/AiGeneration/AiRequestContext.js index cf5ded286..27961baa3 100644 --- a/newIDE/app/src/AiGeneration/AiRequestContext.js +++ b/newIDE/app/src/AiGeneration/AiRequestContext.js @@ -33,6 +33,7 @@ import { getUserRequestText, } from './AiRequestUtils'; import { type EditApprovalRequest } from './Utils'; +import { customUpdateAiRequest } from '../AI/CustomAIClient'; type EditorFunctionCallResultsStorage = {| getEditorFunctionCallResults: ( @@ -374,6 +375,11 @@ export const useAiRequestsStorage = (): AiRequestStorage => { const newAiRequest = updateFn( previousState.aiRequests[aiRequestId] || null ); + // Write a local (BYOK) request through to the cache so plan flips and + // the loop guard's error state are not reverted by the next cache fetch. + if (aiRequestId.startsWith('local-ai-')) { + customUpdateAiRequest(newAiRequest); + } return { ...previousState, aiRequests: { diff --git a/newIDE/app/src/AiGeneration/AskAiEditorContainer.js b/newIDE/app/src/AiGeneration/AskAiEditorContainer.js index 6acbca97b..72b0e553f 100644 --- a/newIDE/app/src/AiGeneration/AskAiEditorContainer.js +++ b/newIDE/app/src/AiGeneration/AskAiEditorContainer.js @@ -839,17 +839,17 @@ export const AskAiEditor: React.ComponentType = React.memo( console.info( 'Skipping send for AI request: some function call results are not finished yet.' ); - return; + return false; } if (hasFunctionsCallsToProcess) { console.info( 'Skipping send for AI request: there are still function calls to process.' ); - return; + return false; } // If nothing to send, stop there. - if (functionCallOutputs.length === 0 && !userMessage) return; + if (functionCallOutputs.length === 0 && !userMessage) return false; // Paying with credits is only when a user message is sent (and quota is exhausted). let payWithCredits = false; @@ -1004,6 +1004,7 @@ export const AskAiEditor: React.ComponentType = React.memo( onOpenExternalLayout(externalLayoutName); }); } + return true; }, [ profile, @@ -1069,7 +1070,7 @@ export const AskAiEditor: React.ComponentType = React.memo( createdProject?: ?gdProject, |} ) => { - await onSendMessage({ + return onSendMessage({ aiRequestId, userMessage: '', createdProject: options.createdProject, @@ -1112,6 +1113,7 @@ export const AskAiEditor: React.ComponentType = React.memo( editorCallbacks, aiRequestsToProcess, onSendEditorFunctionCallResults, + isStudioEnabled: true, getEditorFunctionCallResults, addEditorFunctionCallResults, onSceneEventsModifiedOutsideEditor, diff --git a/newIDE/app/src/AiGeneration/AskAiStandAloneForm.js b/newIDE/app/src/AiGeneration/AskAiStandAloneForm.js index 0409c8b9a..36e0f5ff0 100644 --- a/newIDE/app/src/AiGeneration/AskAiStandAloneForm.js +++ b/newIDE/app/src/AiGeneration/AskAiStandAloneForm.js @@ -494,13 +494,13 @@ export const AskAiStandAloneForm = ({ // If anything is not finished yet, stop there (we only send all // results at once, AI do not support partial results). - if (hasUnfinishedResult) return; - if (hasFunctionsCallsToProcess) return; + if (hasUnfinishedResult) return false; + if (hasFunctionsCallsToProcess) return false; // If nothing to send, stop there. // When in a standalone form, this can happen if the agent did not // decide to create a project, in this case, abort and clear the form. - if (functionCallOutputs.length === 0) return; + if (functionCallOutputs.length === 0) return false; try { setSendingAiRequest(aiRequestId, true); @@ -578,6 +578,7 @@ export const AskAiStandAloneForm = ({ // is up-to-date after an AI request. await delay(500); await refreshLimits({ withRetry: true }); + return true; }, [ profile, @@ -605,7 +606,7 @@ export const AskAiStandAloneForm = ({ createdProject?: ?gdProject, |} ) => { - await onSendMessage({ + return onSendMessage({ aiRequestId, userMessage: '', createdSceneNames: options.createdSceneNames, @@ -631,6 +632,7 @@ export const AskAiStandAloneForm = ({ editorCallbacks, aiRequestsToProcess, onSendEditorFunctionCallResults, + isStudioEnabled: false, getEditorFunctionCallResults, addEditorFunctionCallResults, i18n, diff --git a/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.js b/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.js index f8c836086..e019425e1 100644 --- a/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.js +++ b/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.js @@ -68,11 +68,28 @@ export const isSubAgentAtTurnCap = ({ export const isSubAgentFinished = ({ subAgentRequest, editorFunctionCallResults, + aiRequests, }: {| subAgentRequest: AiRequest, editorFunctionCallResults: Array | null, + aiRequests?: { [string]: AiRequest } | null, |}): boolean => { - if (subAgentRequest.status !== 'ready') return false; + // A sub-agent at its turn cap is finalized with its last message even if it + // still has calls left: the dispatcher has stopped issuing its turns. + if ( + aiRequests && + isSubAgentAtTurnCap({ aiRequest: subAgentRequest, aiRequests }) + ) { + return true; + } + // The loop guard's terminal state is 'error'; treat it as finished so the + // studio writes its (failed) report back instead of waiting forever. + if ( + subAgentRequest.status !== 'ready' && + subAgentRequest.status !== 'error' + ) { + return false; + } if ( getFunctionCallsToProcess({ aiRequest: subAgentRequest, @@ -92,21 +109,56 @@ export const isSubAgentFinished = ({ ) { return false; } + // A result that was executed but whose output has not landed in the child's + // transcript yet: finalizing now would drop the child's real wrap-up forever. + const writtenBackCallIds = new Set( + (subAgentRequest.output || []) + .filter(message => message.type === 'function_call_output') + .map(message => (message: any).call_id) + ); + if ( + editorFunctionCallResults && + editorFunctionCallResults.some( + result => !writtenBackCallIds.has(result.call_id) + ) + ) { + return false; + } return true; }; +/** Reports are capped so one runaway sub-agent cannot flood the parent. */ +export const MAX_SUB_AGENT_REPORT_LENGTH = 4000; + /** * The text written into the parent's `function_call_output`: the sub-agent's - * last assistant message, prefixed with the role's display name. - * - * The message's text lives in a `content` entry whose type is `'output_text'` - * for a message the server produced, and `'text'` for one the local BYOK path - * parsed (`parseAssistantMessage` emits `'text'`); both are read, and the - * message's own `text` field is the fallback, so a report is never lost to the - * difference. + * last assistant message, prefixed with any failure or turn-limit context and + * suffixed with its failed calls, then truncated. The message's text is read + * from a `content` entry (`'output_text'` from the server, `'text'` from the + * local BYOK parser) with the message's own `text` field as fallback. */ -export const buildSubAgentReport = (subAgentRequest: AiRequest): string => { +export const buildSubAgentReport = ( + subAgentRequest: AiRequest, + aiRequests?: { [string]: AiRequest } | null +): string => { const output = subAgentRequest.output || []; + let report = ''; + + if (subAgentRequest.error) { + report += `Failed: ${subAgentRequest.error.message}\n\n`; + } + if ( + aiRequests && + isSubAgentAtTurnCap({ aiRequest: subAgentRequest, aiRequests }) + ) { + report += 'Reached its turn limit.\n\n'; + } + + // The sub-agent's last assistant message is the core of the report. Its text + // lives in a `content` entry whose type is `'output_text'` for a message the + // server produced, and `'text'` for one the local BYOK path parsed; both are + // read, and the message's own `text` field is the fallback. + let core = '(no report)'; for (let i = output.length - 1; i >= 0; i--) { const message = output[i]; if (message.type !== 'message' || message.role !== 'assistant') continue; @@ -115,13 +167,39 @@ export const buildSubAgentReport = (subAgentRequest: AiRequest): string => { ); const textFromContent = textContent ? (textContent: any).text : null; if (typeof textFromContent === 'string' && textFromContent) { - return textFromContent; + core = textFromContent; + } else { + const messageText = (message: any).text; + core = + typeof messageText === 'string' && messageText + ? messageText + : '(no report)'; } - const messageText = (message: any).text; - if (typeof messageText === 'string' && messageText) return messageText; - return '(no report)'; + break; } - return '(no report)'; + report += core; + + // Surface failed tool calls so the parent sees what went wrong. + const failedMessages = output + .filter( + message => + message.type === 'function_call_output' && + (message: any).success === false + ) + .map(message => { + const messageOutput = (message: any).output; + return (messageOutput && messageOutput.message) || '(failed call)'; + }); + if (failedMessages.length > 0) { + report += `\n\nFailed calls:\n- ${failedMessages.join('\n- ')}`; + } + + if (report.length > MAX_SUB_AGENT_REPORT_LENGTH) { + return ( + report.slice(0, MAX_SUB_AGENT_REPORT_LENGTH) + '\n…(report truncated)' + ); + } + return report; }; /** diff --git a/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.spec.js b/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.spec.js index 70b5de1d9..e3471778d 100644 --- a/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.spec.js +++ b/newIDE/app/src/AiGeneration/Studio/FinalizeSubAgents.spec.js @@ -38,6 +38,17 @@ const spawnCall = ( ...(subAgentId ? { subAgentAiRequestId: subAgentId } : {}), }); +const functionCallOutput = ( + callId: string, + success: boolean, + message?: string +): any => ({ + type: 'function_call_output', + call_id: callId, + success, + output: message !== undefined ? { message } : {}, +}); + const makeSubAgent = (overrides: any): any => ({ id: 'child-1', createdAt: '2026-01-01T00:00:00.000Z', @@ -125,7 +136,13 @@ describe('FinalizeSubAgents', () => { ).toBe(true); expect( isSubAgentFinished({ - subAgentRequest: makeSubAgent({}), + subAgentRequest: makeSubAgent({ + output: [ + assistantMessage('Done.'), + // The result's output has landed in the transcript (written back). + functionCallOutput('call-y', true), + ], + }), editorFunctionCallResults: [ { status: 'finished', @@ -137,6 +154,42 @@ describe('FinalizeSubAgents', () => { }) ).toBe(true); }); + it('treats a sub-agent that errored as finished (4i)', () => { + expect( + isSubAgentFinished({ + subAgentRequest: makeSubAgent({ + status: 'error', + error: { code: 'repeated-tool-call-loop', message: 'stuck' }, + }), + editorFunctionCallResults: null, + }) + ).toBe(true); + }); + + it('is false for an executed-but-unsent result (4i)', () => { + expect( + isSubAgentFinished({ + subAgentRequest: makeSubAgent({}), + editorFunctionCallResults: [ + { + status: 'finished', + call_id: 'call-x', + success: false, + output: { message: 'boom' }, + }, + ], + }) + ).toBe(false); + }); + + it('is false for an aborted/timeout-shaped request', () => { + expect( + isSubAgentFinished({ + subAgentRequest: makeSubAgent({ status: 'aborted' }), + editorFunctionCallResults: null, + }) + ).toBe(false); + }); }); describe('buildSubAgentReport', () => { diff --git a/newIDE/app/src/AiGeneration/Studio/LoopGuard.js b/newIDE/app/src/AiGeneration/Studio/LoopGuard.js index b62415c6c..d69993d15 100644 --- a/newIDE/app/src/AiGeneration/Studio/LoopGuard.js +++ b/newIDE/app/src/AiGeneration/Studio/LoopGuard.js @@ -45,14 +45,14 @@ export type LoopGuardDecision = {| export const DEFAULT_LOOP_GUARD_CONFIG = { enabled: true, - /** Consecutive identical tool calls (same name AND same arguments) allowed. */ + /** Trips on the Nth consecutive identical call; N-1 are tolerated. */ repeatedToolLimit: 8, - /** Consecutive failed tool calls allowed. */ + /** Trips on the Nth consecutive failed call; N-1 are tolerated. */ errorStormLimit: 5, /** - * Assistant turns allowed without a user message in between. Generous on - * purpose: a legitimate long task (a `run_script` doing hundreds of calls, a - * gameplay test) is not a loop. + * Trips on the Nth assistant turn without a user message; N-1 are tolerated. + * Generous on purpose: a legitimate long task (a `run_script` doing hundreds + * of calls, a gameplay test) is not a loop. */ maxTurnsWithoutUserInput: 60, }; @@ -80,24 +80,53 @@ const actionFor = (level: LoopGuardLevel): LoopGuardAction => { return 'none'; }; +/** Recursively stringify with object keys sorted, so key order is not identity. */ +const stableStringify = (value: any): string => { + if (Array.isArray(value)) { + return `[${value.map(stableStringify).join(',')}]`; + } + if (value && typeof value === 'object') { + const keys = Object.keys(value).sort(); + return `{${keys + .map(key => `${JSON.stringify(key)}:${stableStringify(value[key])}`) + .join(',')}}`; + } + return JSON.stringify(value); +}; + /** - * The identity of a tool call: its name and a hash of its **whole** arguments - * string. + * Parse `argsJson` and re-emit it canonically (keys sorted) so two argument + * objects that differ only in key order hash the same. A non-JSON string is + * hashed as-is. + */ +const canonicalizeArgs = (argsJson: string): string => { + try { + return stableStringify(JSON.parse(argsJson || 'null')); + } catch (error) { + return argsJson || ''; + } +}; + +/** + * The identity of a tool call: its name and a hash of its arguments, taken + * whole and in canonical (key-sorted) form. * - * Hashing the whole string is the point. munder-difflin's #377 records that its - * old `input.slice(0, 200)` collided on Bash commands sharing a long identical - * preamble - nine *different* measurements in a row read as nine identical calls - * and constrained the agent. Different arguments must produce different keys, - * however long their common prefix. + * Hashing the whole arguments is the point. munder-difflin's #377 records that + * its old `input.slice(0, 200)` collided on Bash commands sharing a long + * identical preamble - nine *different* measurements in a row read as nine + * identical calls and constrained the agent. Different arguments must produce + * different keys, however long their common prefix; canonicalizing means two + * calls that differ only in argument key order are the *same* call. */ export const toolCallKey = (name: string, argsJson: string): string => { const shaObj = new jsSHA('SHA-256', 'TEXT', { encoding: 'UTF8' }); - shaObj.update(argsJson || ''); + shaObj.update(canonicalizeArgs(argsJson)); return `${name}:${shaObj.getHash('HEX')}`; }; export type LoopGuard = {| recordToolCall: (call: {| name: string, arguments: string |}) => void, + recordSuccess: () => void, recordError: () => void, recordTurn: () => void, reset: () => void, @@ -118,6 +147,7 @@ export const createLoopGuard = ( let repeatCount = 0; let errorCount = 0; let turnCount = 0; + let actionEmittedForTrip = false; const evaluateTrips = (): {| tripping: boolean, reason: string |} => { if (repeatCount >= effectiveConfig.repeatedToolLimit) { @@ -151,11 +181,12 @@ export const createLoopGuard = ( } else { repeatKey = key; repeatCount = 1; - // A distinct tool call is progress: it clears the error storm - // (munder-difflin `breaker.ts` `recordToolUse`). - errorCount = 0; } }, + recordSuccess: () => { + // Only a real success clears the error storm: an attempt is not progress. + errorCount = 0; + }, recordError: () => { errorCount++; }, @@ -171,6 +202,7 @@ export const createLoopGuard = ( repeatCount = 0; errorCount = 0; turnCount = 0; + actionEmittedForTrip = false; }, evaluate: () => { if (!effectiveConfig.enabled) { @@ -200,12 +232,20 @@ export const createLoopGuard = ( level = target; reason = trip.tripping ? trip.reason : changed ? 'recovering' : reason; + // Emit on an escalation (a new level carries its own action) or on a + // fresh trip after a reset - but never re-send a durable action on every + // tick. Re-armed by `reset` and on recovery. + const shouldEmit = escalated || (trip.tripping && !actionEmittedForTrip); + if (shouldEmit) { + actionEmittedForTrip = true; + } else if (!trip.tripping) { + actionEmittedForTrip = false; + } + return { level, reason, - // Only an escalation carries an action, so a durable steer is not - // re-sent on every tick. - action: escalated ? actionFor(target) : 'none', + action: shouldEmit ? actionFor(target) : 'none', changed, }; }, diff --git a/newIDE/app/src/AiGeneration/Studio/LoopGuard.spec.js b/newIDE/app/src/AiGeneration/Studio/LoopGuard.spec.js index a16476374..674ca62b3 100644 --- a/newIDE/app/src/AiGeneration/Studio/LoopGuard.spec.js +++ b/newIDE/app/src/AiGeneration/Studio/LoopGuard.spec.js @@ -92,15 +92,23 @@ describe('LoopGuard', () => { expect(decision.reason).toContain('5'); }); - it('is reset by a successful (distinct) tool call', () => { - const guard = createLoopGuard({ errorStormLimit: 3 }); - guard.recordError(); - guard.recordError(); - guard.recordToolCall({ name: 'create_scene', arguments: '{}' }); - guard.recordError(); - guard.recordError(); - // Only two consecutive errors since the successful call. - expect(guard.evaluate().level).toBe('healthy'); + it('is reset by a success but not by a mere attempt', () => { + // A distinct tool call is an attempt, not progress: the storm continues. + const attempt = createLoopGuard({ errorStormLimit: 3 }); + attempt.recordError(); + attempt.recordError(); + attempt.recordToolCall({ name: 'create_scene', arguments: '{}' }); + attempt.recordError(); + expect(attempt.evaluate().level).toBe('steering'); + + // A real success clears the storm. + const success = createLoopGuard({ errorStormLimit: 3 }); + success.recordError(); + success.recordError(); + success.recordSuccess(); + success.recordError(); + success.recordError(); + expect(success.evaluate().level).toBe('healthy'); }); }); @@ -211,6 +219,46 @@ describe('LoopGuard', () => { }); }); + describe('canonicalization and re-arming', () => { + it('treats arguments that differ only in key order as the same call', () => { + expect(toolCallKey('create_scene', '{"a":1,"b":2}')).toBe( + toolCallKey('create_scene', '{"b":2,"a":1}') + ); + }); + + it('is not tripped by a benign A-B-A-B alternation', () => { + const guard = createLoopGuard({ repeatedToolLimit: 3 }); + for (const args of [ + '{"a":1}', + '{"a":2}', + '{"a":1}', + '{"a":2}', + '{"a":1}', + ]) { + guard.recordToolCall({ name: 'create_scene', arguments: args }); + } + expect(guard.evaluate().level).toBe('healthy'); + }); + + it('re-emits an action on a re-trip after reset at the ceiling', () => { + const guard = createLoopGuard({ + repeatedToolLimit: 1, + maxTurnsWithoutUserInput: 1000, + }); + guard.recordToolCall({ name: 'a', arguments: '{}' }); + guard.evaluate(); // steering + guard.evaluate(); // constrained + guard.evaluate(); // constrained (ceiling, durable: no action) + expect(guard.evaluate().action).toBe('none'); + + guard.reset(); + guard.recordToolCall({ name: 'a', arguments: '{}' }); + const retripped = guard.evaluate(); + expect(retripped.level).toBe('constrained'); + expect(retripped.action).toBe('constrain'); + }); + }); + it('defaults are the documented ones', () => { expect(DEFAULT_LOOP_GUARD_CONFIG).toEqual({ enabled: true, diff --git a/newIDE/app/src/AiGeneration/Studio/PlanStore.js b/newIDE/app/src/AiGeneration/Studio/PlanStore.js index 4dcf8af04..94bf11c8a 100644 --- a/newIDE/app/src/AiGeneration/Studio/PlanStore.js +++ b/newIDE/app/src/AiGeneration/Studio/PlanStore.js @@ -23,7 +23,7 @@ export type StudioPlanTask = {| title: string, description: string, status: StudioPlanTaskStatus, - dependsOn: Array, + dependsOn?: Array, /** * Set by the studio when the task is delegated, so the plan row can be linked * to the sub-agent's function call (matches @@ -74,7 +74,9 @@ export const areTaskDependenciesSatisfied = ( .filter(otherTask => otherTask.status === 'done') .map(otherTask => otherTask.id) ); - return task.dependsOn.every(dependencyId => doneIds.has(dependencyId)); + return (task.dependsOn || []).every(dependencyId => + doneIds.has(dependencyId) + ); }; /** The first pending task whose dependencies are satisfied, or null. */ @@ -118,6 +120,32 @@ export const mergePlanTasks = ( }); }; +/** + * Fold the plan inside a finished tool result's `output` over the plan the + * request already has. `output` is the object `EditorFunctionCallRunner` + * builds (`{ success, meta, ...output }`), so it is read as an object - never + * parsed. A result without a plan, or a non-success one, passes through + * untouched. + */ +export const mergePlanResultOutput = ( + result: Object, + existingTasks: Array | null +): Object => { + if (result.status !== 'finished' || !result.success) return result; + const output: any = result.output; + if (!output || !output.plan || !Array.isArray(output.plan.tasks)) { + return result; + } + if (!existingTasks) return result; + return { + ...result, + output: { + ...output, + plan: { tasks: mergePlanTasks(existingTasks, output.plan.tasks) }, + }, + }; +}; + /** * Apply `patch` to one task, leaving every other task — and every other field of * the patched task — untouched. @@ -155,6 +183,22 @@ const describeValue = (value: any): string => { * sent it, naming the first offending index. A model mistake becomes a normal * failed tool call, never a thrown error inside the editor. */ +// Bounds on a plan the model sends, so a runaway tool call cannot flood the +// store (W12). +const MAX_PLAN_TASKS = 32; +const MAX_PLAN_TASK_ID_LENGTH = 64; +const MAX_PLAN_TASK_TITLE_LENGTH = 200; +const MAX_PLAN_TASK_DESCRIPTION_LENGTH = 1000; + +const ALLOWED_TASK_FIELDS = [ + 'id', + 'title', + 'description', + 'status', + 'dependsOn', + 'agentCallId', +]; + export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { if (!Array.isArray(rawTasks)) { return { @@ -162,8 +206,17 @@ export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { message: 'create_or_update_plan requires a tasks array.', }; } + if (rawTasks.length > MAX_PLAN_TASKS) { + return { + success: false, + message: `create_or_update_plan accepts at most ${MAX_PLAN_TASKS} tasks, got ${ + rawTasks.length + }.`, + }; + } const tasks: Array = []; + const seenIds = new Set(); for (let index = 0; index < rawTasks.length; index++) { const rawTask = rawTasks[index]; const invalid = (detail: string): StudioPlanTaskValidation => ({ @@ -174,11 +227,36 @@ export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { if (!rawTask || typeof rawTask !== 'object' || Array.isArray(rawTask)) { return invalid(`expected an object, got ${describeValue(rawTask)}.`); } + // Fail closed (I11): a field this validator does not model would be + // silently dropped by a rebuild, so reject the whole task instead. + for (const key of Object.keys(rawTask)) { + if (!ALLOWED_TASK_FIELDS.includes(key)) { + return invalid(`unexpected field ${JSON.stringify(key)}.`); + } + } + if ('agentCallId' in rawTask && typeof rawTask.agentCallId !== 'string') { + return invalid( + `\`agentCallId\` must be a string when present, got ${describeValue( + rawTask.agentCallId + )}.` + ); + } if (typeof rawTask.id !== 'string' || !rawTask.id.trim()) { return invalid( `\`id\` must be a non-empty string, got ${describeValue(rawTask.id)}.` ); } + // Validate and store the trimmed id (I12); every later match is on it. + const id = rawTask.id.trim(); + if (id.length > MAX_PLAN_TASK_ID_LENGTH) { + return invalid( + `\`id\` must be at most ${MAX_PLAN_TASK_ID_LENGTH} characters.` + ); + } + if (seenIds.has(id)) { + return invalid(`duplicate \`id\` ${JSON.stringify(id)}.`); + } + seenIds.add(id); if (typeof rawTask.title !== 'string' || !rawTask.title.trim()) { return invalid( `\`title\` must be a non-empty string, got ${describeValue( @@ -186,6 +264,11 @@ export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { )}.` ); } + if (rawTask.title.trim().length > MAX_PLAN_TASK_TITLE_LENGTH) { + return invalid( + `\`title\` must be at most ${MAX_PLAN_TASK_TITLE_LENGTH} characters.` + ); + } if ( typeof rawTask.description !== 'string' || !rawTask.description.trim() @@ -196,6 +279,11 @@ export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { )}.` ); } + if (rawTask.description.trim().length > MAX_PLAN_TASK_DESCRIPTION_LENGTH) { + return invalid( + `\`description\` must be at most ${MAX_PLAN_TASK_DESCRIPTION_LENGTH} characters.` + ); + } if (!isStudioPlanTaskStatus(rawTask.status)) { return invalid( `\`status\` must be one of ${PLAN_TASK_STATUSES.join( @@ -211,7 +299,7 @@ export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { Array.isArray(rawDependsOn) && rawDependsOn.every((id: any) => typeof id === 'string') ) { - dependsOn = rawDependsOn; + dependsOn = rawDependsOn.map((dependencyId: any) => dependencyId.trim()); } else { return invalid( `\`dependsOn\` must be an array of strings, got ${describeValue( @@ -221,12 +309,16 @@ export const validatePlanTasks = (rawTasks: any): StudioPlanTaskValidation => { } const task: StudioPlanTask = { - id: rawTask.id, + id, title: rawTask.title, description: rawTask.description, status: (rawTask.status: StudioPlanTaskStatus), - dependsOn, }; + // `dependsOn` is set only when the caller sent it (W11): a re-plan that + // omits it must not clear dependencies another writer set. + if (rawDependsOn != null) { + task.dependsOn = dependsOn; + } if (typeof rawTask.agentCallId === 'string') { task.agentCallId = rawTask.agentCallId; } diff --git a/newIDE/app/src/AiGeneration/Studio/PlanStore.spec.js b/newIDE/app/src/AiGeneration/Studio/PlanStore.spec.js index cbb118d35..cb73353f6 100644 --- a/newIDE/app/src/AiGeneration/Studio/PlanStore.spec.js +++ b/newIDE/app/src/AiGeneration/Studio/PlanStore.spec.js @@ -139,7 +139,7 @@ describe('PlanStore', () => { }); describe('validatePlanTasks', () => { - it('accepts a well-formed plan and defaults dependsOn to an empty array', () => { + it('accepts a well-formed plan and omits dependsOn when not provided', () => { const result = validatePlanTasks([ { id: 'a', @@ -150,7 +150,9 @@ describe('PlanStore', () => { ]); expect(result.success).toBe(true); if (result.success) { - expect(result.tasks[0].dependsOn).toEqual([]); + // W11: an omitted `dependsOn` is left off the task so a re-plan cannot + // clear a dependency another writer set. + expect(result.tasks[0].dependsOn).toBeUndefined(); expect(result.tasks[0].status).toBe('pending'); } }); @@ -298,4 +300,98 @@ describe('PlanStore', () => { expect(reread).toEqual(patched); }); }); + + describe('composition and bounds (W11-W13, I11)', () => { + it('lets an omitted dependsOn survive a re-plan', () => { + const existing: any = [ + makeTask({ id: 'a', dependsOn: ['b'], agentCallId: 'call-1' }), + makeTask({ id: 'b', status: 'done' }), + ]; + const validated = validatePlanTasks([ + { id: 'a', title: 'A2', description: 'D', status: 'pending' }, + ]); + expect(validated.success).toBe(true); + const merged: any = mergePlanTasks( + existing, + validated.success ? validated.tasks : [] + ); + // W11 regression: the omitted dependsOn (and agentCallId) survive. + expect(merged[0].dependsOn).toEqual(['b']); + expect(merged[0].agentCallId).toBe('call-1'); + expect(merged[0].title).toBe('A2'); + }); + + it('rejects duplicate ids (W13)', () => { + const validated = validatePlanTasks([ + { id: 'a', title: 'A', description: 'D', status: 'pending' }, + { id: 'a', title: 'A2', description: 'D', status: 'pending' }, + ]); + expect(validated.success).toBe(false); + }); + + it('rejects an oversized plan and oversized strings (W12)', () => { + const many = Array.from({ length: 33 }, (_, i) => ({ + id: `t${i}`, + title: 'T', + description: 'D', + status: 'pending', + })); + expect(validatePlanTasks(many).success).toBe(false); + expect( + validatePlanTasks([ + { + id: 'x'.repeat(65), + title: 'T', + description: 'D', + status: 'pending', + }, + ]).success + ).toBe(false); + expect( + validatePlanTasks([ + { + id: 'a', + title: 'y'.repeat(201), + description: 'D', + status: 'pending', + }, + ]).success + ).toBe(false); + expect( + validatePlanTasks([ + { + id: 'a', + title: 'T', + description: 'z'.repeat(1001), + status: 'pending', + }, + ]).success + ).toBe(false); + }); + + it('rejects unexpected fields and a null agentCallId (I11)', () => { + expect( + validatePlanTasks([ + { + id: 'a', + title: 'T', + description: 'D', + status: 'pending', + extra: 1, + }, + ]).success + ).toBe(false); + expect( + validatePlanTasks([ + { + id: 'a', + title: 'T', + description: 'D', + status: 'pending', + agentCallId: null, + }, + ]).success + ).toBe(false); + }); + }); }); diff --git a/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.js b/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.js index 905eb9794..5abb7671e 100644 --- a/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.js +++ b/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.js @@ -16,6 +16,9 @@ * - `createRequestWriteQueue` serializes the writes per request, keeping at most * one pending write (latest wins) and retrying a blocked one rather than * dropping it. + * + * A `write` must never `await` an `enqueue` for the same `aiRequestId`: the drain + * loop cannot interleave and that promise would never settle. */ export type RequestWriteBlock = @@ -104,8 +107,16 @@ export const createRequestWriteQueue = (): RequestWriteQueue => { while (entry.pending) { const pending = entry.pending; if (pending.isBlocked()) { - // Do not drop it: the next enqueue retries it. Nothing more to do - // now, because the queue only advances when the caller enqueues. + // Do not drop it, and do not wait for another enqueue (none may ever + // come): schedule a self-recheck. The promise stays pending until the + // write actually runs. + setTimeout(() => { + if (!entry.isRunning && entry.pending) { + runEntry(aiRequestId).catch(error => { + console.error('[studio] request write queue failure:', error); + }); + } + }, 50); break; } entry.pending = null; @@ -132,12 +143,19 @@ export const createRequestWriteQueue = (): RequestWriteQueue => { ): Promise => { const entry = getEntry(aiRequestId); // Latest wins: replace whatever was waiting, and settle it as superseded - // (resolved, not rejected - the caller asked for a write that a newer one - // made pointless). + // (rejected, not resolved - a replaced write never ran and must not report + // success). if (entry.pending) { const superseded = entry.pending; entry.pending = null; - superseded.resolve(); + superseded.reject( + Object.assign( + new Error('Superseded by a newer write for this request.'), + { + code: 'superseded', + } + ) + ); } return new Promise((resolve, reject) => { entry.pending = { write, isBlocked, resolve, reject }; diff --git a/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.spec.js b/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.spec.js index f24232fc1..761971c27 100644 --- a/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.spec.js +++ b/newIDE/app/src/AiGeneration/Studio/RequestWriteGate.spec.js @@ -190,7 +190,15 @@ describe('RequestWriteGate', () => { () => false ); - await Promise.all([first, second, third]); + // The superseded write never ran and must reject, not report success. + let supersededError = null; + try { + await second; + } catch (error) { + supersededError = error; + } + await Promise.all([first, third]); + expect(supersededError && supersededError.code).toBe('superseded'); expect(events).toEqual(['first', 'third']); }); @@ -199,10 +207,10 @@ describe('RequestWriteGate', () => { let blocked = true; const events = []; - const write = queue.enqueue( + const first = queue.enqueue( 'req-1', async () => { - events.push('ran'); + events.push('first-ran'); }, () => blocked ); @@ -211,17 +219,22 @@ describe('RequestWriteGate', () => { await new Promise(resolve => setTimeout(resolve, 10)); expect(events).toEqual([]); - // Unblock and enqueue again: the retried write runs. + // Unblock: the queue retries the blocked write itself (no new enqueue). blocked = false; + await new Promise(resolve => setTimeout(resolve, 80)); + await first; + expect(events).toEqual(['first-ran']); + + // A later write runs after it: both ran, so the first was retried, not + // dropped. await queue.enqueue( 'req-1', async () => { - events.push('ran'); + events.push('second-ran'); }, - () => blocked + () => false ); - await write; - expect(events).toEqual(['ran']); + expect(events).toEqual(['first-ran', 'second-ran']); }); it('rejects the caller when its write throws', async () => { diff --git a/newIDE/app/src/AiGeneration/Studio/Roles.js b/newIDE/app/src/AiGeneration/Studio/Roles.js index 3bb14039a..759491f40 100644 --- a/newIDE/app/src/AiGeneration/Studio/Roles.js +++ b/newIDE/app/src/AiGeneration/Studio/Roles.js @@ -25,7 +25,8 @@ export type StudioRole = {| /** * Exact tool names exposed to this role. Any name not listed here is never * sent to the model (see `getToolsForRole`), so a role cannot be talked into - * using a tool it was not granted. + * calling a tool it was not granted. (`run_script` excepted: it reaches any + * registry function not in `NonScriptableFunctionNames`.) */ allowedToolNames: Array, /** @@ -81,6 +82,9 @@ export const MUTATING_TOOL_NAMES: Array = [ 'change_custom_behavior', 'create_custom_function', 'change_custom_function', + 'run_gameplay_test', + 'change_gameplay_tests', + 'initialize_project', ]; export const STUDIO_ROLES: { [StudioRoleId]: StudioRole } = { @@ -202,17 +206,17 @@ export const SPAWNABLE_ROLE_IDS: Array = [ ]; export const isStudioRoleId = (value: any): boolean => - typeof value === 'string' && !!STUDIO_ROLES[(value: any)]; + typeof value === 'string' && + Object.prototype.hasOwnProperty.call(STUDIO_ROLES, (value: any)); export const isSpawnableRoleId = (value: any): boolean => isStudioRoleId(value) && SPAWNABLE_ROLE_IDS.includes((value: StudioRoleId)); export const getStudioRole = (roleId: StudioRoleId): StudioRole => { - const role = STUDIO_ROLES[roleId]; - if (!role) { + if (!Object.prototype.hasOwnProperty.call(STUDIO_ROLES, (roleId: any))) { throw new Error(`Unknown studio role: ${String(roleId)}.`); } - return role; + return STUDIO_ROLES[roleId]; }; /** diff --git a/newIDE/app/src/AiGeneration/Studio/Roles.spec.js b/newIDE/app/src/AiGeneration/Studio/Roles.spec.js index 47d583914..a66d3c56f 100644 --- a/newIDE/app/src/AiGeneration/Studio/Roles.spec.js +++ b/newIDE/app/src/AiGeneration/Studio/Roles.spec.js @@ -12,9 +12,7 @@ describe('Studio roles', () => { const allToolNames: Array = GDEVELOP_OPENAI_TOOLS.map( tool => tool.function.name ); - // `spawn_agent` is part of the studio's own tool schema addition (Phase 4), - // not of the upstream-derived part of `GDEVELOP_OPENAI_TOOLS`. - const availableToolNames = new Set([...allToolNames, 'spawn_agent']); + const availableToolNames = new Set(allToolNames); const roleIds = Object.keys(STUDIO_ROLES); @@ -106,11 +104,15 @@ describe('Studio roles', () => { expect( designerReadOnlyTools.filter(name => !READ_ONLY_TOOL_NAMES.includes(name)) ).toEqual([]); - // And the tester is granted no mutating tool at all. + // And the tester writes no game content: its only mutating tools (per W1) + // are the two gameplay-test tools, which change test definitions, never the + // game itself. const testerMutations = STUDIO_ROLES.tester.allowedToolNames.filter(name => MUTATING_TOOL_NAMES.includes(name) ); - expect(testerMutations).toEqual([]); + expect(testerMutations.sort()).toEqual( + ['change_gameplay_tests', 'run_gameplay_test'].sort() + ); }); it('only the manager may delegate, so a sub-agent never spawns a sub-agent', () => { diff --git a/newIDE/app/src/AiGeneration/Studio/SpawnSubAgents.js b/newIDE/app/src/AiGeneration/Studio/SpawnSubAgents.js index 4c74c2883..c61c9d879 100644 --- a/newIDE/app/src/AiGeneration/Studio/SpawnSubAgents.js +++ b/newIDE/app/src/AiGeneration/Studio/SpawnSubAgents.js @@ -2,11 +2,14 @@ import { makeSimplifiedProjectBuilder } from '../../EditorFunctions/SimplifiedProject/SimplifiedProject'; import { customCreateSubAgentAiRequest } from '../../AI/CustomAIClient'; -import { getStudioRole, isSpawnableRoleId, type StudioRoleId } from './Roles'; +import { isSpawnableRoleId, type StudioRoleId } from './Roles'; import { type AiRequestMessageAssistantFunctionCall } from '../../Utils/GDevelopServices/Generation'; const gd: libGDevelop = global.gd; +/** Cap on live sub-agents one parent may have (W8). */ +export const MAX_SUB_AGENTS_PER_PARENT = 8; + /** Whether a function call is the studio's delegation tool. */ export const isSpawnAgentCall = ( functionCall: AiRequestMessageAssistantFunctionCall @@ -61,12 +64,23 @@ export const parseSpawnAgentArgs = ( const task = rawTask.trim(); if (!task) return null; + // Reject (do not coerce) a present-but-wrongly-typed optional field. + if (rawContext !== undefined && typeof rawContext !== 'string') return null; + if (rawRelatedTaskId !== undefined && typeof rawRelatedTaskId !== 'string') + return null; + const context = typeof rawContext === 'string' ? rawContext.trim() : ''; const relatedTaskId = typeof rawRelatedTaskId === 'string' && rawRelatedTaskId.trim() ? rawRelatedTaskId.trim() : null; + // Reject oversized strings (W6): a runaway argument must not become a child + // request the user cannot read. + if (shortTitle.length > 100 || task.length > 4000 || context.length > 4000) { + return null; + } + return { role, shortTitle, task, context, relatedTaskId }; }; @@ -118,9 +132,10 @@ export const buildGddContextNote = (project: ?gdProject): string => { : JSON.stringify(variable.variableChildren || variable.type) }` ); - return `The studio's design document, as GDD_ project variables:\n${lines.join( + const note = `The studio's design document, as GDD_ project variables:\n${lines.join( '\n' )}`; + return note.length > 4000 ? note.slice(0, 4000) + '\n…(truncated)' : note; }; export type SpawnSubAgentResult = @@ -140,6 +155,7 @@ export const spawnSubAgent = async ({ gameProjectJson, projectSpecificExtensionsSummaryJson, project, + gddContextNote, onStamped, }: {| parentAiRequestId: string, @@ -147,6 +163,9 @@ export const spawnSubAgent = async ({ gameProjectJson: string | null, projectSpecificExtensionsSummaryJson: string | null, project: ?gdProject, + // The GDD note, computed once per batch and passed in (W7). When provided it + // is used verbatim instead of rebuilding the simplified project per call. + gddContextNote?: string | null, onStamped: (subAgentAiRequestId: string) => void, |}): Promise => { const parsedArgs = parseSpawnAgentArgs(functionCall); @@ -154,13 +173,6 @@ export const spawnSubAgent = async ({ return { error: 'Invalid spawn_agent arguments.' }; } - const role = getStudioRole(parsedArgs.role); - if (!role) { - // `isSpawnableRoleId` already gated this, but keep the failure a normal - // result rather than a throw, so a model mistake is a failed tool call. - return { error: `Unknown role: ${String(parsedArgs.role)}.` }; - } - let userRequest = `Task: ${parsedArgs.task}`; if (parsedArgs.context) { userRequest += `\n\nContext: ${parsedArgs.context}`; @@ -170,7 +182,9 @@ export const spawnSubAgent = async ({ // it, so handing it back to the designer would be noise. const spawnContextNote = parsedArgs.role === 'developer' || parsedArgs.role === 'tester' - ? buildGddContextNote(project) + ? gddContextNote != null + ? gddContextNote + : buildGddContextNote(project) : null; const subAgentRequest = await customCreateSubAgentAiRequest({ diff --git a/newIDE/app/src/AiGeneration/Studio/UseStudioRuntime.js b/newIDE/app/src/AiGeneration/Studio/UseStudioRuntime.js index 3fb2e9b58..d2301e00f 100644 --- a/newIDE/app/src/AiGeneration/Studio/UseStudioRuntime.js +++ b/newIDE/app/src/AiGeneration/Studio/UseStudioRuntime.js @@ -11,6 +11,7 @@ import { isSubAgentFinished, buildSubAgentReport, getSubAgentReportLabel, + getSubAgentRoleId, } from './FinalizeSubAgents'; import { buildPlanOutput, @@ -132,7 +133,7 @@ export const useStudioRuntime = ({ aiRequestId: string, editorFunctionCallResults: Array, options: Object - ) => Promise, + ) => Promise, /** * Runs a write once the request is free (see `RequestWriteGate`). Optional so * the hook can be used without the gate; the write then runs immediately. @@ -172,6 +173,10 @@ export const useStudioRuntime = ({ const subAgentRequest = aiRequests[subAgentId]; if (!subAgentRequest) continue; + // A hosted sub-agent is retired by the server, never finalized here. + if (!getSubAgentRoleId({ aiRequest: subAgentRequest, aiRequests })) { + continue; + } const parentRequest = aiRequests[subAgentInfo.parentAiRequestId]; if (!parentRequest) continue; @@ -229,33 +234,43 @@ export const useStudioRuntime = ({ const parentRequest = aiRequests[parentAiRequestId]; if (parentRequest) { - const updatedOutput = buildPlanStatusUpdateOutput( - parentRequest, - callId - ); - if (updatedOutput) { - const writePlan = async () => { - updateAiRequest(parentAiRequestId, currentRequest => - currentRequest - ? { ...currentRequest, output: updatedOutput } - : // The request disappeared: hand back the same empty shell - // the mutator expects rather than null. - ({ - id: parentAiRequestId, - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), - userId: 'local-byok-user', - status: 'ready', - error: null, - output: updatedOutput, - }: any) - ); - }; - if (enqueueRequestWrite) { - await enqueueRequestWrite(parentAiRequestId, writePlan); - } else { - await writePlan(); - } + const writePlan = async () => { + updateAiRequest(parentAiRequestId, currentRequest => { + const base = + currentRequest || + ({ + id: parentAiRequestId, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + userId: 'local-byok-user', + status: 'ready', + error: null, + output: [], + }: any); + // Recompute inside the updater: two finalizations in one pass + // would otherwise write the first snapshot twice. + const updated = buildPlanStatusUpdateOutput(base, callId); + return updated ? { ...base, output: updated } : base; + }); + }; + if (enqueueRequestWrite) { + // Fire-and-forget: a blocked gate write no longer settles + // inline, so awaiting it would freeze every later finalization + // behind `isRunningRef`. + enqueueRequestWrite(parentAiRequestId, writePlan).catch( + error => { + if (error.code === 'superseded') { + console.info('[studio] plan write superseded'); + } else { + console.error( + '[studio] error while writing the plan update:', + error + ); + } + } + ); + } else { + await writePlan(); } } @@ -263,7 +278,7 @@ export const useStudioRuntime = ({ aiRequest: subAgentRequest, aiRequests, }); - await onSendEditorFunctionCallResults( + const sent = await onSendEditorFunctionCallResults( parentAiRequestId, [ { @@ -272,13 +287,17 @@ export const useStudioRuntime = ({ success: true, output: { message: `Report from the ${label}:\n\n${buildSubAgentReport( - subAgentRequest + subAgentRequest, + aiRequests )}`, }, }, ], {} ); + // Report delivery is not droppable: throw so the `catch` retries + // (it deletes `startedCallIds`, so the next pass redoes this). + if (!sent) throw new Error('Sub-agent report was not sent.'); } } catch (error) { console.error('[studio] error while finalizing a sub-agent:', error); diff --git a/newIDE/app/src/AiGeneration/Utils.js b/newIDE/app/src/AiGeneration/Utils.js index b0afcf85a..8ed4cb040 100644 --- a/newIDE/app/src/AiGeneration/Utils.js +++ b/newIDE/app/src/AiGeneration/Utils.js @@ -62,9 +62,16 @@ import { makeSimplifiedProjectBuilder } from '../EditorFunctions/SimplifiedProje import { prepareAiUserContent } from './PrepareAiUserContent'; import { extractGDevelopApiErrorStatusAndCode } from '../Utils/GDevelopServices/Errors'; import { isCustomEndpointEnabled } from '../AI/CustomAIClient'; -import { isSpawnAgentCall, spawnSubAgent } from './Studio/SpawnSubAgents'; +import { + isSpawnAgentCall, + spawnSubAgent, + buildGddContextNote, + MAX_SUB_AGENTS_PER_PARENT, +} from './Studio/SpawnSubAgents'; import { createLoopGuard } from './Studio/LoopGuard'; -import { mergePlanTasks } from './Studio/PlanStore'; +import { mergePlanResultOutput } from './Studio/PlanStore'; +import { isSubAgentAtTurnCap } from './Studio/FinalizeSubAgents'; +import { getStudioRole } from './Studio/Roles'; import { createRequestWriteQueue, getRequestWriteBlock, @@ -253,6 +260,7 @@ export const useProcessFunctionCalls = ({ editorCallbacks, aiRequestsToProcess, onSendEditorFunctionCallResults, + isStudioEnabled, getEditorFunctionCallResults, addEditorFunctionCallResults, onSceneEventsModifiedOutsideEditor, @@ -289,7 +297,7 @@ export const useProcessFunctionCalls = ({ createdExternalLayoutNames?: Array, createdProject?: ?gdProject, |} - ) => Promise, + ) => Promise, getEditorFunctionCallResults: string => Array | null, addEditorFunctionCallResults: ( string, @@ -327,6 +335,9 @@ export const useProcessFunctionCalls = ({ onWillInstallExtension: (extensionNames: Array) => void, onExtensionInstalled: (extensionNames: Array) => void, isReadyToProcessFunctionCalls: boolean, + // Whether this view runs the multi-agent studio (the editor does; the + // stand-alone form does not, so it cannot spawn sub-agents). + isStudioEnabled?: boolean, getIsAutoEditEnabled: () => boolean, suspendAiRequest: (aiRequestId: string) => Promise, requestEditApproval: (request: EditApprovalRequest) => Promise, @@ -436,6 +447,9 @@ export const useProcessFunctionCalls = ({ // consecutive identical calls, consecutive failures and turns, so a local // agent that loops cannot run forever. const loopGuardsRef = React.useRef>(new Map()); + const loopGuardLastUserMessageIdRef = React.useRef>( + new Map() + ); // Forget all previously-granted edit approvals so the next modifying call // asks again. Called when the user toggles auto-edit: turning it on then off @@ -453,6 +467,16 @@ export const useProcessFunctionCalls = ({ if (!isReadyToProcessFunctionCalls) return; if (aiRequest.status === 'suspended') return; + // A studio sub-agent that has used up its role's turn budget takes no + // further turn (the studio finalizes it with its last message). Return + // before taking any in-flight lock. + if ( + isCustomEndpointEnabled() && + isSubAgentAtTurnCap({ aiRequest, aiRequests: aiRequestsRef.current }) + ) { + return; + } + const functionCallsToProcess = functionCalls.filter( functionCall => !inFlightFunctionCallIdsRef.current.has( @@ -473,14 +497,44 @@ export const useProcessFunctionCalls = ({ ); }); + // The studio role a sub-agent was spawned with restricts its tool subset + // (defense in depth: the model is offered only these, and anything else it + // still calls is refused here). All denied calls stay in + // `functionCallsToProcess` so their in-flight locks are released below. + const studioRole = aiRequest.studioRoleId + ? getStudioRole((aiRequest.studioRoleId: any)) + : null; + const studioDeniedCallIds = new Set(); + if (studioRole) { + for (const functionCall of functionCallsToProcess) { + if (!studioRole.allowedToolNames.includes(functionCall.name)) { + studioDeniedCallIds.add(functionCall.call_id); + addEditorFunctionCallResults(aiRequest.id, [ + { + status: 'finished', + call_id: functionCall.call_id, + success: false, + output: { + message: `The tool ${ + functionCall.name + } is not available for the ${studioRole.displayName} role.`, + }, + }, + ]); + } + } + } + // An explorer sub-agent's script is read-only (see below: it is exposed // only non-mutating functions). Knowing this lets us both skip its edit - // approval and restrict the functions its `run_script` can call. + // approval and restrict the functions its `run_script` can call. A + // read-only studio role is read-only too. const subAgentKind = getSubAgentKind({ aiRequest, aiRequests: aiRequestsRef.current, }); - const isReadOnlyScriptContext = subAgentKind === 'explorer'; + const isReadOnlyScriptContext = + subAgentKind === 'explorer' || (!!studioRole && !!studioRole.readOnly); // Gate project-modifying calls behind a user confirmation when auto-edit // is off. Read-only calls (exploration, inspection) always run. The first @@ -510,6 +564,9 @@ export const useProcessFunctionCalls = ({ // non-mutating functions), so it never needs an edit approval even // though `run_script` is declared as project-modifying. !(isReadOnlyScriptContext && functionCall.name === 'run_script') && + // A read-only studio role never raises the prompt (its subset was + // fixed at spawn and is enforced by the dispatch filter above). + !(studioRole && studioRole.readOnly) && !isCallApproved(functionCall) ); @@ -567,21 +624,39 @@ export const useProcessFunctionCalls = ({ } } - addEditorFunctionCallResults( - aiRequest.id, - functionCallsToProcess.map(functionCall => ({ - status: 'working', - call_id: functionCall.call_id, - })) - ); - // The local loop guard: with no server under BYOK, nothing else stops an // agent looping on fresh call ids. Only recorded for local requests - the // hosted backend enforces its own `repeated-tool-call-loop` error, and a // second client-side trip over the same condition would double-report. - if (isCustomEndpointEnabled()) { - const guard = createLoopGuard(); + // One guard per request (persisted across passes) so its counters + // accumulate; reset on a new user message. + const loopGuardHandledCallIds = new Set(); + const guard = isCustomEndpointEnabled() + ? loopGuardsRef.current.get(aiRequest.id) || createLoopGuard() + : null; + if (guard) { loopGuardsRef.current.set(aiRequest.id, guard); + const lastUserMessage = (aiRequest.output || []) + .filter( + message => message.type === 'message' && message.role === 'user' + ) + .pop(); + const currentLastUserMessageId = lastUserMessage + ? (lastUserMessage: any).messageId + : null; + const seenLastUserMessageId = loopGuardLastUserMessageIdRef.current.get( + aiRequest.id + ); + if ( + currentLastUserMessageId && + currentLastUserMessageId !== seenLastUserMessageId + ) { + guard.reset(); + loopGuardLastUserMessageIdRef.current.set( + aiRequest.id, + currentLastUserMessageId + ); + } functionCallsToProcess.forEach(functionCall => { guard.recordToolCall({ name: functionCall.name, @@ -595,17 +670,19 @@ export const useProcessFunctionCalls = ({ const loopGuardMessage = `The local agent looks stuck (${ decision.reason }). Stop repeating this action and change approach, or report the problem.`; + // Pair the warning with the batch's first real call: an unpaired + // `function_call_output` (a call the model never made) is rejected by + // OpenAI-compatible APIs. + const guardedCall = functionCallsToProcess[0]; addEditorFunctionCallResults(aiRequest.id, [ { status: 'finished', - // A synthetic id: the model sees the warning in its transcript - // as the output of a call it did not make, which is enough for - // it to change course. - call_id: `loop-guard-${aiRequest.id}-${Date.now()}`, + call_id: guardedCall.call_id, success: false, output: { message: loopGuardMessage }, }, ]); + loopGuardHandledCallIds.add(guardedCall.call_id); } if (decision.action === 'constrain') { // Stop issuing further turns for this request. The request is marked @@ -651,11 +728,54 @@ export const useProcessFunctionCalls = ({ // hosted backend never emits this tool name, so hosted behaviour is // untouched and this pre-pass is skipped entirely. const spawnCalls = isCustomEndpointEnabled() - ? functionCallsToProcess.filter(isSpawnAgentCall) + ? functionCallsToProcess.filter( + functionCall => + isSpawnAgentCall(functionCall) && + !studioDeniedCallIds.has(functionCall.call_id) + ) : []; const spawnHandledCallIds = new Set(); + const spawnFailureResults: Array = []; if (spawnCalls.length > 0) { + // Computed once per batch (W7), not once per spawn. + const gddContextNote = buildGddContextNote(project); + // Fan-out cap (W8): bound the live children one parent may have. + const existingChildCount = Object.keys(aiRequestsRef.current).filter( + id => + (aiRequestsRef.current[id] || {}).parentAiRequestId === aiRequest.id + ).length; + let spawnedThisBatch = 0; for (const spawnCall of spawnCalls) { + // The stand-alone view has no studio: sub-agents cannot be spawned. + if (!isStudioEnabled) { + spawnHandledCallIds.add(spawnCall.call_id); + const denial = { + status: 'finished', + call_id: spawnCall.call_id, + success: false, + output: { message: 'Sub-agents are not available in this view.' }, + }; + spawnFailureResults.push(denial); + addEditorFunctionCallResults(aiRequest.id, [denial]); + continue; + } + // Beyond the fan-out cap, refuse instead of spawning. + if ( + existingChildCount + spawnedThisBatch >= + MAX_SUB_AGENTS_PER_PARENT + ) { + spawnHandledCallIds.add(spawnCall.call_id); + const capDenial = { + status: 'finished', + call_id: spawnCall.call_id, + success: false, + output: { message: 'Sub-agent limit reached for this request.' }, + }; + spawnFailureResults.push(capDenial); + addEditorFunctionCallResults(aiRequest.id, [capDenial]); + continue; + } + spawnedThisBatch++; let spawnResult; try { spawnResult = await spawnSubAgent({ @@ -664,6 +784,7 @@ export const useProcessFunctionCalls = ({ gameProjectJson: aiRequest.gameProjectJson || null, projectSpecificExtensionsSummaryJson: null, project, + gddContextNote, onStamped: subAgentAiRequestId => { // Stamp the call in place so the shared loop skips it from here // on, then activate the child so AiRequestContext polls it and @@ -684,26 +805,44 @@ export const useProcessFunctionCalls = ({ spawnHandledCallIds.add(spawnCall.call_id); if (spawnResult.error) { // Surface it as a failed call the model sees, rather than a crash. - addEditorFunctionCallResults(aiRequest.id, [ - { - status: 'finished', - call_id: spawnCall.call_id, - success: false, - output: { message: spawnResult.error }, - }, - ]); + const failure = { + status: 'finished', + call_id: spawnCall.call_id, + success: false, + output: { message: spawnResult.error }, + }; + spawnFailureResults.push(failure); + addEditorFunctionCallResults(aiRequest.id, [failure]); } } } - const callsForEditor = spawnHandledCallIds.size - ? functionCallsToProcess.filter( - functionCall => !spawnHandledCallIds.has(functionCall.call_id) - ) - : functionCallsToProcess; + const callsForEditor = functionCallsToProcess.filter( + functionCall => + !spawnHandledCallIds.has(functionCall.call_id) && + !loopGuardHandledCallIds.has(functionCall.call_id) && + !studioDeniedCallIds.has(functionCall.call_id) + ); + // Mark only the editor calls as "working": a spawn (or a loop-guard hit) + // already has a terminal result, and a permanent "working" entry would + // make `hasUnfinishedResult` silently drop sibling sends. + addEditorFunctionCallResults( + aiRequest.id, + callsForEditor.map(functionCall => ({ + status: 'working', + call_id: functionCall.call_id, + })) + ); if (callsForEditor.length === 0) { - // Nothing left for the editor: the spawn results were already recorded, - // and the parent's next turn is driven by the sub-agent reporting back. - callsForEditor.forEach(functionCall => { + // Nothing left for the editor: surface any spawn failures to the model + // (they are already recorded but not yet sent), then release the locks. + if (spawnFailureResults.length > 0) { + await onSendEditorFunctionCallResults( + aiRequest.id, + spawnFailureResults, + {} + ); + } + functionCallsToProcess.forEach(functionCall => { inFlightFunctionCallIdsRef.current.delete( `${aiRequest.id}:${functionCall.call_id}` ); @@ -899,47 +1038,43 @@ export const useProcessFunctionCalls = ({ return; } + // Record outcomes for the loop guard's error-storm trip (a real success + // clears it; a failure advances it). + if (guard) { + results.forEach(result => { + if (result.status !== 'finished') return; + if (result.success) guard.recordSuccess(); + else guard.recordError(); + }); + } + const newResults = addEditorFunctionCallResults(aiRequest.id, results); // A plan written by the plan tool is folded over the plan the request // already has, matching tasks by id: the tool sends the tasks it knows // about, so a field another writer set (the studio's `agentCallId` when - // it delegates a task, Phase 5) must survive. Without this, re-planning - // would silently strip it. - const mergedResults = newResults.map(result => { - if (result.status !== 'finished' || !result.success) return result; - let output; - try { - output = JSON.parse(result.output); - } catch (error) { - return result; - } - if (!output || !output.plan || !Array.isArray(output.plan.tasks)) { - return result; - } - const existingPlan = getLatestActivePlan(aiRequest); - const existingTasks = existingPlan ? existingPlan.tasks : null; - if (!existingTasks) return result; - return { - ...result, - output: JSON.stringify({ - ...output, - plan: { - tasks: mergePlanTasks( - // $FlowFixMe[incompatible-type] - existingTasks, - output.plan.tasks - ), - }, - }), - }; - }); + // it delegates a task) must survive. `result.output` is the object + // `EditorFunctionCallRunner` builds, so it is read as an object, never + // parsed. + const existingPlan = getLatestActivePlan(aiRequest); + const existingTasks = existingPlan ? existingPlan.tasks : null; + const mergedResults = newResults.map(result => + // $FlowFixMe[incompatible-type] + mergePlanResultOutput(result, existingTasks) + ); - await onSendEditorFunctionCallResults(aiRequest.id, mergedResults, { - createdSceneNames, - createdExternalLayoutNames, - createdProject, - }); + const sent = await onSendEditorFunctionCallResults( + aiRequest.id, + mergedResults, + { + createdSceneNames, + createdExternalLayoutNames, + createdProject, + } + ); + if (!sent) { + console.error('[studio] function call results were not sent'); + } } finally { // Flush the coalesced editor notifications for everything modified in // this batch (one hot reload instead of one per call). In `finally` so @@ -959,8 +1094,10 @@ export const useProcessFunctionCalls = ({ } // Release the lock so these calls can be retried if needed - // (e.g. after an error or a suspension). - callsForEditor.forEach(functionCall => { + // (e.g. after an error or a suspension). Release every call that was + // locked (including spawn/loop-guard handled ones), not just the ones + // sent to the editor. + functionCallsToProcess.forEach(functionCall => { inFlightFunctionCallIdsRef.current.delete( `${aiRequest.id}:${functionCall.call_id}` ); @@ -993,6 +1130,7 @@ export const useProcessFunctionCalls = ({ getAssetStoreTagForNewObject, generateEvents, onSendEditorFunctionCallResults, + isStudioEnabled, getIsAutoEditEnabled, suspendAiRequest, requestEditApproval, diff --git a/newIDE/app/src/Utils/GDevelopServices/Generation.js b/newIDE/app/src/Utils/GDevelopServices/Generation.js index 329b424c8..d750f2bdc 100644 --- a/newIDE/app/src/Utils/GDevelopServices/Generation.js +++ b/newIDE/app/src/Utils/GDevelopServices/Generation.js @@ -164,6 +164,9 @@ export type AiRequest = { forkedAfterOriginalMessageId?: string | null, forkedAfterNewMessageId?: string | null, parentAiRequestId?: string | null, + // The studio role a sub-agent was spawned with (fork-only; set only on local + // sub-agent requests) so its tool subset and prompt survive every turn. + studioRoleId?: string | null, error: AiRequestError | null, contextStats?: AiRequestContextStats | null,