Repository navigation
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61ca4587a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+189
to
+192
| $starts = $this->commonEventStarts; | ||
| foreach ($this->commonEvents as $id => $event) { | ||
| $start = isset($starts[$id]) ? self::mergeContexts($starts[$id]) : []; | ||
| $this->walkCommands($event['commands'], [], $start, $event['file'], [$id]); |
There was a problem hiding this comment.
When a map invokes Common Event A and A invokes Common Event B, this snapshot is taken before walking A, so B's newly discovered start context is never used when B's own source is processed. Coordinates in B therefore remain unchanged during a row/column insertion even though they execute in the edited map, silently leaving the project spatially inconsistent; compute the transitive Common Event contexts before rewriting their files.
Useful? React with 👍 / 👎.
| return $npcs; | ||
| } | ||
|
|
||
| $references = $this->workspace instanceof ProjectWorkspace ? new NpcReferences($this->workspace)->describe($map, $id) : []; |
There was a problem hiding this comment.
When an NPC is referenced only by a Common Event reached from an NPC/inline script, or by another Common Event, NpcReferences::describe() returns no references because it only considers event definitions' direct scriptId values and never follows common_event commands. Renaming that NPC therefore reaches the fallback below and changes its ID while the reachable script retains the old npcId, breaking the route/reference that this guard is intended to preserve.
Useful? React with 👍 / 👎.
Comment on lines
+141
to
+142
| $this->layers[self::BASE]['path'] = $this->buildPath(0, 'terrain', false); | ||
| $this->legacy = false; |
There was a problem hiding this comment.
Saving the first added layer converts the map and removes its legacy <map>.map.php, but PlaytestOverlay::installCinematicLaunch() still rejects any start map without exactly that file in its ['data', 'map', 'event'] preflight. Consequently every map converted here can no longer launch a cinematic playtest, even though the overlay mirrors the layer directory and only rewrites the data and event members.
Useful? React with 👍 / 👎.
Comment on lines
3300
to
3305
| $issues = [...$issues, ...$this->walkEventCommands( | ||
| $laneCommands, | ||
| $where, | ||
| $known, | ||
| $npcIds, | ||
| $mapId, | ||
| $npcIdsByMap, | ||
| $context, | ||
| $commonEventStack, |
There was a problem hiding this comment.
When one parallel lane contains a transfer, passing the same by-reference context into each lane means that transfer mutates the starting map used to validate every later lane. For example, a second lane routing an NPC on the original map is checked against the first lane's destination and receives a false missing-NPC error; each lane should start from a copy of the pre-parallel context, with lane results merged afterward.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
An effect sequence's Cadence is a choice of the Engine's cadences: fixed plays at its FPS; battle_phase spreads its frames over the battle phase it plays in. A battle-paced sequence has no FPS, as the Engine requires: its FPS row is hidden, and choosing battle_phase puts its FPS aside so choosing fixed again gives it back; undo restores both. The preview plays a battle-paced sequence over its phase as the Engine times it (BattleTurnTimings for the command's category at a battle pace): the action animation for a source stage, the effect animation for a target stage. It starts at the project's own pace, the target stage unless every battle binding uses the effect as a source, and A, P and S change the kind of command, the pace and the stage.
Legacy actor identity repair now covers battle pose sets keyed by actor and dialogue speaker bindings to actors, so a repair no longer strands them. Speaker bindings to artwork resources stay untouched. Presentation constructor arguments are read from the Engine class by reflection instead of a copied parameter list, so new catalog fields such as a battle scale are matched without editing a signature here. Nested constructors and single string arguments are rewritten in place; variable or computed arguments are still refused rather than flattened.
Legacy actor identity repair now rewrites the battle scale's reference actor and its party profile keys. Enemy profiles and every calibration value are preserved.
The map world asks the Engine for tile shadows, so the GUI editor shows the same bands the game draws, rebuilt from the tiles after every stroke. The tile palette keeps its shadow-free world.
map.world now includes tile shadows only when the request sets tileShadows, as a game renderer negotiates tile_shadows. A GUI editor built against a renderer that cannot paint shadow layers keeps receiving the world it can draw.
An effect image track offers the Engine's battle attachment (center, head or ground) and its pivot, the point of the sheet cell that sits on that placement. The pivot is one x, y row from 0 to 1, stored whole or removed whole, so a half-written pivot can never reach the Engine.
A record or NPC row that names a field keeps its key whether or not it can be edited. Read-only rows now say editable false, and the session had read that as having no key, so an interface could no longer name an id row. Editability only makes the row info, which every edit refuses.
Region was a text field. It is now a reference picker over the region names the project's maps already use, so one region is spelled one way. Choosing a region changes only the map data's region; the map stays in its folder.
A data file whose returned value is an array literal of plain data is saved through the shared source writer: only changed values are edited where they sit, so comments, nowdocs and layout inside the data survive. Such a file with interior comments is no longer read-only; Last Legend's animations.php and troops.php become editable. A change the source cannot express is refused rather than flattened, and a file changed on disk since it was read is refused rather than overwritten. Files that hold objects keep their constructor-argument edits or regeneration.
troops.formation returns the battle canvas, the project's arenas with the one previewed and its background, the party's slots with the starting party's names, and each member's enemy and battle placement. The arena is a preview only and writes nothing. A troop member's graphicalPlacement is now a Battle Placement row, one x, y, width, height value, so moving a member is one edit and one undo step; the terminal position is separate and untouched.
Animations become a schema-driven category over assets/Data/animations.php, edited through the shared record database in the terminal editor and the GUI session alike. A record names its caster and target effect timelines from the effects picker, and its battle roles as a list picked a member at a time; a role another animation holds is refused, naming it, and the picker labels say who holds each role. A new animation takes the next numeric id. Saving rewrites only changed values, keeping the file's comments, and an older record's own frames, cues and unknown fields are kept exactly as written. Removed: the bespoke animation screen and ProjectAnimationDatabase. The terminal frame-grid painter, its brush rows, the frame and cue panes' animation content, frame sound and flash rows, and the Shift+P frame preview are gone; an older record's frames and cues are shown read-only and still played by the Engine's importer, and new animation is authored as effect timelines. The database's frames and preview panes no longer take focus. Shared fixes: a schema record edit the record refused, or that left it unchanged, no longer records an empty undo step; the session passes the multi-pick flag so an interface toggles list members; back-navigation restores every schema category's selection.
troops.formation now asks the Engine's BattleFormationLayout, the placement the battle itself uses, for the formation: the arenas and the previewed one's backgrounds, the starting party in its slots, and for each placed member where its feet stand, the bounds its idle art is drawn in, that art and its body span at battle scale. A placement the Engine refuses is reported on its member.
…and copy A new or duplicated record with a numeric identity now always takes the next number after the largest any record holds or held this session, so it never reuses an id something may still name. Adding or duplicating a record leaves out list members another record holds alone, such as an animation's roles: the copy starts without them and the original keeps them, so each role still plays exactly one animation.
A data file with comments inside its data was read-only even when its entries are constructor calls the record database edits argument by argument, which keeps every comment. Comments now refuse only a save that would regenerate the returned expression; a file edited in place stays editable.
The enemies category reads assets/Data/Enemies, each file returning ['class' => Enemy::class, 'data' => [...]], in the form the engine's EnemyRecord reads. Values are edited in the file's own source, action patterns name catalogue skills with conditions as data, speed and state resistances are now editable, and new or duplicated enemies get a file named after them. Per-file categories now list a file that cannot be read as a record, read-only with the reason, instead of leaving it out, and can duplicate a record into a file of its own. The guide no longer says items are not writable.
…scale enemies.preview reads the open enemy's sprite by the game's rule, from the record as it is now, and composes its battle art with the starting party through BattleFormationLayout, standing opposite the lead party member for comparison. Troop formations and the preview share one composition. A project without a graphical battle still gets the sprite.
Classes were built by hand in the terminal editor over their own database, which regenerated classes.php on every save and which the GUI session could not open. They are now a schema-driven category, so both editors edit them the same way and saves change only what was edited. Every stat curve's growth and per-level values, grace and evasion bases, equipment types and skills learned are now editable as well. The terminal's experience and stat curve panes read the shared record. The terminal list now offers Shift+A on every category that can create an entry, and marks unsaved record categories, which it did not before.
The class list showed each class's id before its name until classes moved onto the shared record service, which listed names only. Every record category identified by a number now lists it before the name, classes and animations alike.
Quests were built by hand in the terminal editor over their own database, which rewrote quests.php on every save and which the GUI session could not open. They are now a schema-driven category, so both editors edit them the same way and saves change only what was edited. Optional and reward item quantities are now editable too. The record service gains what quests needed, for every category: - A record can hold lists beside its own, each under a heading row that is where an entry is added to it (a quest's reward items). - A list entry authored as a bare value reads as that value's field and is written back bare while it holds nothing else, and a list can drop its key when it empties. - An identity can follow its label while nothing refers to it; the workspace says what refers to a quest. The rename note reaches both editors. - A unique field now refuses a single value another record has, ignoring case, not only list members. Two classes or two enemies can no longer share a name. Changed in the terminal: Shift+O and Shift+X act on the row under the cursor, adding after or removing that entry in whichever list it is in, instead of always the end of the record's own list; objective rows read Objective 1 Type rather than Obj 1 Type. The GUI names the record's own list for its add button and shows only the add button on a list heading.
An actor's Attack Skill row picks the battle-usable basic skill its Attack command uses, from the skill catalogue, or leaves it to the Engine's built-in attack. Validation reports one that is not such a skill.
…record service System was built by hand in the terminal editor over its own database, which regenerated system.php on every save, offered only the battle engine and ATB fields, and could not be opened in the GUI. It is now a schema-driven category over the whole of system.php: title, starting gold, starting party (bare actor ids) and inventory, starting map, position and facing, elements, battle engine, openings and every ATB setting, saved by editing only what changed. Field Zoom moves from System to a new Configuration category over the rest of config.php (save, accessibility, ui, graphics, audio, inn): one row per setting, typed by what it holds, enum settings offered as their cases by name and written back as cases. A setting the engine reads with a default is listed where the file leaves it out; choosing the default writes nothing. ProjectConfig owns the zoom's range for both editors. For every category, an edit the file could not be saved with, such as a value the author wrote as an expression, is now refused when it is made rather than when the category is saved; and a value written as an enum case's value (MovementHeading::SOUTH->value) is rewritten as the new case of the same enum rather than refused. Enum fields accept a case by value or name and work for enums without values.
ActorAuthoring owns an actor's rows, previews, edits, creation, deletion and identity freeze, with what the panes show (variant, assumed growth, optimize slot) as its own view state. The terminal editor calls it in place of its private actor code; actor creation is now an undo step. The session reaches every category through DatabaseCategory: RecordCategory for schema-driven ones, ActorCategory for actors. Rows gain an action kind (the identity freeze) and the none choice's label. Undo and redo name actors when a step changed them.
…nces Enemies gain a Drops list (rewards.items): an inventory item and its chance, the list removed with its last entry. RecordReferenceValidator checks every schema record's fields and list entries that name a shared definition, leaving the lists a check with more context owns to it. The validator's clean baseline defines the enemies the fixture's troops name.
…reads them Types now edits the elements list of system.php, one element per row, and System no longer carries a second copy. The element pickers read the same identities the game knows (ElementRegistry::getCanonicalIdentities), not the unused enum under assets/Data/Types; validation reports a list the game would refuse. Categories sharing system.php each own their top-level keys: WholeFileProjection reads and writes only those, so a save no longer carries a stale reading of another category's settings over the file. A schema whose fields leave its owned keys is refused when declared. Tests that needed a read-only category use a file the editor cannot rewrite.
…ditors Skills move onto the shared record service over assets/Data/Skills, so attacks, abilities and spells, with their effects, are edited in the terminal and the GUI alike. A skill's kind is a value; a spell alone offers its effect type, and a changed kind or effect type drops what only the old one read. ProjectSkill and ProjectSkillDatabase are gone; the terminal's Effects, Scope and preview panes read the record. Records gain numberedFiles: a new or copied record takes the next number and is listed last, as the Engine's file-order catalogue lists it. A record whose field set depends on it drops the fields its old shape alone offered, and an inline list's type change drops the old type's values, as command frames already did. Validation covers every skill file.
…t subjects, as cinematics do
…directly ProjectReachability::analyze() now takes current map sources (Engine 69d0881), so reachability judges unsaved maps in memory. Removed: UnsavedMapStage, the disposable asset root it copied maps into and linked the rest of assets beside, and ProjectMap::exportSources(), which only that stage used. Untiled floor is judged from each map as it stands in the workspace. The branch now needs Engine 69d0881.
…map authoring gaps into develop
# Conflicts: # tests/Unit/Session/WorldObjectAuthoringTest.php
Editor develop uses Engine APIs published as Engine 5c77c5a (map atlas and category catalogues, battle announcements, map completeness, subject-relative cinematic positions, chest grant capabilities); the lock still named Engine 739da66, so CI analysed and tested against the old Engine.
…olds them A field resource added in a change set was invisible to the operations after it: the resource pickers, world object resolution, map completeness validation and the effect check all loaded field.php from disk. They now read ReferenceCatalog::getFieldPresentationCatalog(), the file as the Field Resources page would leave it, which the workspace gives each map like its other current-state providers. Landmarks and cinematic preview origins read events through the Engine's MapEventResolver, so an event bound to a world object keeps its cells there; renaming a landmark anchored to a bound event no longer reports the event missing. Save All and change set commits write databases and cutscenes before maps, so a map is never on disk naming a record that is not.
…ate keys The duplicate-key check only counted [ ] as array scopes, so a map file in array ( ) syntax, as var_export writes it, had every key read at the top level: two NPCs with the same fields were reported as duplicate keys and refused the map. Parentheses now open an array scope only after the array keyword. Adds the scene rebuild tests: a resource, its world object, a bound event, relative staging, an NPC and common event removed, committed as one change set or refused with nothing written; and a landmark renamed after its event is bound.
… long-syntax duplicate keys, into develop
…ving coordinates A map event that starts a reusable event script by id recorded only that script's start. Scripts it ran were walked from a snapshot taken before that walk, so their coordinates on the edited map stayed put during a row or column insertion or removal. The line inventory now settles every reusable event script's starts by following the scripts that run them until no new start appears, then moves coordinates. Starts are kept once each, so the settling ends, also for scripts that run each other in a cycle. Subject-relative positions stay unrewritten.
NpcReferences only read the event scripts a map event names by scriptId, and never followed common_event commands. An NPC named only by an event script started from an NPC script, an event script, a dialogue variant or a cinematic, directly or through other event scripts, had no references, so a rename let its derived id follow the name and the Id row changed it while the script kept the old npcId. The finder now collects the scripts every map-local owner starts and follows them transitively, reading each once so cycles end.
…maps A cinematic playtest locates its origin on the start map and hands the Engine a launch context; the game then reads that map from the project's own root. The test covers a map converted to layers (no single map file) and a single-file map, and reads each start map with the Engine's MapSourceReader from the launch's project root. The review finding about PlaytestOverlay's fixed data/map/event preflight no longer applies: 76918d5 removed the overlay, and PlaytestLaunch copies and checks no map files.
The Engine ticks parallel lanes together on the one current map, and a transfer in any lane suspends the session and moves every lane. Validation walked lanes in file order with one shared context, so a lane listed before a sibling's transfer was checked only against the starting map, and a block with several transferring lanes ended on the last-listed destination. Each lane now starts on the incoming map or any map a sibling lane transfers to, a transfer inside a lane can still be overtaken by a sibling's, and the block ends on any map its transferring lanes can finish on. Event scripts, cinematics and map line edits share this model through CommandMapContext lane and transfer helpers. Validation keeps the possible maps as a list in ScriptMapState, so joined names no longer repeat.
…s escaped The strict decoders split on every colon and kept only the parts each type expects, so 'event:a & !event:b' was accepted as one event named 'a & !event' with ':b' silently lost, an unknown quest status became completed and an unreadable switch value became true. Conditions and writes now share one line grammar (WorldStateLine): entries split on unescaped ';', parts on unescaped ':', and a name or value escapes its backslash, ':' and ';', so any name the Engine accepts, such as the flag deckers:won, round-trips as event:deckers\:won. A part the type does not take, an unknown type, or a value the Engine would refuse (quest status, switch and quest flags, item quantity, variable operator) refuses the whole line, naming the entry and why, and nothing changes. Behaviour changes: lines that used to be quietly rewritten or truncated are now refused, and a colon inside a name must be escaped. The script guide documents the line.
…ences
Two NPCs may anchor on one tile only when their visibility conditions
provably exclude each other: one holds a condition the other negates, read
as the Engine's WorldConditionEvaluator runs it (event and !event, switch
and switch:false, variable == and != one value). Creating, moving or
editing the conditions of an NPC checks every NPC on its tile, so an edit
that would show two bodies there together is refused with nothing changed.
Validation applies the same rule, and Enter in the terminal editor steps
through the phases on a tile.
NPC reference walkers read kind, subject and ids only as text: a
structured subject such as {kind: npc, id} is a reference read where the
walk reaches it, and only a common_event command's id names a script.
Duplicating and renaming no longer raise "Array to string conversion".
ScriptEventTrigger validation kept its own copies of the root fields every map event carries and of the Script trigger's data fields. The root copy drifted when the Engine bound events to world objects by `subject`, so a Story Script event on an object was refused while the game ran it. Validation now reads MapEventDefinition::ROOT_FIELDS and ScriptEventTrigger::DATA_FIELDS and MODES from the Engine, which owns them. The event inspector offers the modes of the event's own trigger class, and the Story Script default mode is the Engine's. Unknown root and data fields, unsupported modes and subjects naming no object are still errors.
The source-preserving writer keeps an authored key where it stands and places a new key where the edited value orders it. Maps already adopted that order for what they hold; cutscenes and actors did not, so an edit that rebuilt an entry or set a key again held its keys in another order than the file would read back, and the strict read-back refused the save. Replacing a branch arm's only route with a new one built row by row was refused this way. The alignment maps used is now PhpArraySourceDocument::alignValue, shared by maps, by every cutscene edit and by actor saves. The strict read-back is unchanged. A list entry that moves now plans its own nested removals, insertions and appends inside the bytes it carries; before, they were deferred against bytes the move had cut, and the rewrite failed with overlapping edits.
Editor develop consumes MapEventDefinition::ROOT_FIELDS and ScriptEventTrigger::DATA_FIELDS from Engine 799203d, published today; the lock still named Engine 5c77c5a.
changes.commit saved each document through its own transaction in turn, so a document that refused late, such as a cinematic whose staged script did not read back, left the documents saved before it written: a partial change set on disk. Every document now works out its save before anything is written: maps, cutscenes, record databases, actors, settings files and the save relocations a map needs each declare their files into one transaction, prove their staged bytes, and adopt their written state only once the whole set is installed. SaveSet installs them; a file that cannot be installed puts back the ones already installed. Each document's own save runs the same prepared path alone, and Save All keeps saving documents one by one. A document that refuses refuses the change set as an error does: nothing is written, the change set is undone, and the reason starts "Nothing was written:" and names the document. A saved result's `written` names each document and the files it wrote or removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings main up to develop. Locks the Engine at its published develop, 2e32b6f.
Map kinds
Pieces and canvas
NPCs
Earlier work on develop
Breaking changes are marked
!in the commits.Git workflow governance
This update removes direct-main publishing allowances in repositories where Andrew created
develop, and removes universal workflow wording that imposed it on repositories without that branch. It installs mirrored agent/Claude/Copilot instructions, a workflow policy, PR template, and portable Git guards. Remote working branch publication, branch deletion and history rewriting retain their explicit permission requirements..github,docs, anddemo-repositoryhave no remotedevelopand are excluded; no branches were created there.The local guards reject commits/merge commits and pushes to main. The push guard accepts only authorized fast-forward updates from local develop to existing remote develop; it does not grant publishing permission. Existing hooks and work are preserved. Local hooks can be bypassed by APIs or direct ref operations, so they are one layer alongside repository instructions and available server enforcement.
Validation: 42 guard/installer checks in real temporary Git repositories and bare destinations; five PR source-validator cases; shell syntax and governance whitespace checks. No gameplay, asset or dependency changes are in the governance commits; Linux/native gameplay suites were not run. This develop-to-main PR also includes earlier queued work, which must be reviewed separately.
The trusted
pull_request_targetsource validator does not check out or execute PR code. It must first reach main through this PR before its status can be required. Main update and PR/history rules are active on the four public participating repositories: engine, editor, console and gpui-renderer. Private branch rules are unavailable on the current Free organization plan. No direct-main push, history rewriting, branch deletion, tag or release occurred.Governance/develop head:
d6624149e857c5eb5c585002e2d477b18aff3771.