Skip to content

Comments

Feature/complete gameplay#4

Open
okenic03-code wants to merge 76 commits intomainfrom
feature/complete-gameplay
Open

Feature/complete gameplay#4
okenic03-code wants to merge 76 commits intomainfrom
feature/complete-gameplay

Conversation

@okenic03-code
Copy link
Owner

No description provided.

okenic03-code and others added 21 commits February 13, 2026 03:28
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…port

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
… all levels

- Fix duplicate prefix numbers across all directory levels
- Add missing numeric prefixes to all sub-folders
- Merge scattered UI folders (20_UI, 60_UI) into unified 60_UI
- Move root Editor/ scripts into 10_Scripts/20_Editor/
- Rename UI Toolkit to 90_UIToolkit, Screenshots to 88_Screenshots
- Rename 90_ImportedAssets to 92_ThirdParty
- Update hardcoded asset paths in 3 C# files
…ructure

- Fix sprite paths in MainMenu.uss (MainMenu/ → 10_MainMenu/)
- Fix relative USS paths in 3 UXML files (../USS/ → ../50_USS/)
- Suppress CS0414 warning for _createdNewNet field
…gers

- Rewrite GameUI.uxml with proper element names matching all C# controllers
- Rewrite GameUI.uss with professional dark CAD/EDA theme (KiCad-inspired)
- Add StageManager, ProgressionManager, SceneFlowManager to GamePlay scene
- Wire all SerializeField references between managers and UI controllers
- Set camera to orthographic top-down view
- Add Screenshots directory to .gitignore

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Implements ECAD-style unbounded grid inspired by KiCad/Altium. BoardBounds now supports negative coordinates, BoardState uses suggested area semantics without enforcement, GridRenderer rewritten for viewport-based dynamic rendering, CameraController removes pan limits, GridCursor always visible at any coordinate with visual hints for suggested area.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…onents

Enable user-specified custom values (R/C/L/V/I) for passive and source components while FET/BJT/Diode continue using predefined SO definitions. Changes span the full data flow: PlacedComponent → BoardState → Commands → SaveLoad → NetlistConverter → PlacementController.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…e grid indicator

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…gle-button components

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…c serialized fields

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…add missing meta files

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- Expose CommandHistory instance from GameManager for controller access
- Add CircuitCraft.Commands assembly reference to Managers asmdef
- Fix GroundNode from 'GND' to '0' to match SpiceSharp convention
- UIController: cache status bar label values, skip .text updates when unchanged
- GridCursor: gate Update() on mouse position delta to avoid redundant raycasts
- PlacementController: gate Update() on mouse position delta, wrap Debug.Log in #if UNITY_EDITOR
- GridRenderer: separate visual rebuild (_visualsDirty) from position-only updates, serialize Shader reference
- BoardState: add Dictionary lookups for O(1) component/net access, cache ReadOnlyNets, optimize RemoveComponent iteration
- PlacedComponent: direct pin indexing with GetPin(), cached ReadOnlyPins collection
- ComponentView: use MaterialPropertyBlock to avoid material instance allocations, wrap Debug.Log in #if UNITY_EDITOR
- TraceRenderer: serialize Shader reference to eliminate Shader.Find() at runtime
- WireRoutingController: serialize Shader reference to eliminate Shader.Find() at runtime
- DRCChecker: reuse HashSet/List collections across validation passes to reduce GC pressure
- CommandHistory: add bounded undo history (max 100) to prevent unbounded memory growth
- SimulationRunner: replace .ToArray()[0] with .First() to avoid full array allocation
…ebug logs

- SceneFlowManager: cache WaitForSeconds to avoid per-yield allocation, wrap Debug.Log in #if UNITY_EDITOR
- GridPosition: add FillNeighbors() non-allocating API returning neighbors via Span-like output parameter
- BoardView: wrap Debug.Log calls in #if UNITY_EDITOR preprocessor guards
- ComponentInteraction: wrap Debug.Log calls in #if UNITY_EDITOR preprocessor guards
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

CircuitNetlist netlist = _netlistConverter.Convert(boardState);

P1 Badge Populate simulation probes before evaluating stage objectives

RunSimulationAsync converts the board with _netlistConverter.Convert(boardState) but does not add any probes, and SimulationRunner.CreateExports only exports entries from netlist.Probes. In this flow SimulationResult.ProbeResults is empty, so objective evaluation cannot read required voltages and stage checks fail even when the circuit is otherwise correct.

ℹ️ 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".

{
_suggestedWidth = width;
_suggestedHeight = height;
_boardState = new BoardState(width, height);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve board-state references when resetting the board

ResetBoard replaces _boardState with a new instance, but multiple runtime systems cache the previous instance during Awake/Start (for example BoardView, TraceRenderer, WireRoutingController, and ComponentInteraction). After a stage load, those systems keep listening to and mutating the stale board, so rendering and interaction can diverge from the active gameplay state.

Useful? React with 👍 / 👎.

_stageCards.Add(card);

// Register click event
card.RegisterCallback<ClickEvent>(evt => HandleStageClick(stageIndex));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Unregister stage-select callbacks when re-enabling UI

FindAndSetupUI registers click callbacks every OnEnable, but there is no matching unregistration path. Since SceneFlowManager repeatedly shows/hides this screen, handlers accumulate across visits and a single click can invoke stage selection/back actions multiple times.

Useful? React with 👍 / 👎.

…cy files

- Delete SpiceSharpTestRunner.cs (unreferenced test utility)
- Delete SampleScene.unity and SampleSceneProfile.asset (not in build settings)
- Delete legacy GameHUD.uxml/uss (replaced by 60_UI/GameUI)
- Delete duplicate LED_Red/Diode_1N4148 in 40_Components (20_Components is active)
- Delete Unity boilerplate (Readme.asset, TutorialInfo, ReadmeEditor)
- Delete dev screenshots (88_Screenshots/, Screenshots/)
- Delete 30+ empty placeholder dirs (50_Audio, 85_Step, 92_ThirdParty, 99_Sandbox, etc.)
- Delete root temp files (REFACTORING_REPORT.md, UNITY_SETUP_REQUIRED.md)
- Clean .sisyphus: remove 17 completed notepad dirs and 3 finished plan files
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

CircuitNetlist netlist = _netlistConverter.Convert(boardState);

P1 Badge Attach stage measurement probes before running simulation

Stage evaluation reads voltages through SimulationResult.GetVoltage(...), which only searches ProbeResults, but the runtime netlist is always built via _netlistConverter.Convert(boardState) with no probes provided. That means successful simulations can still return zero voltage measurements, causing test-case evaluation to fail with missing readings and making stages with test cases effectively unwinnable unless probes are added to the generated netlist.

ℹ️ 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 150 to 153
foreach (var trace in _boardState.GetTraces(sourceNetId).ToList())
{
_boardState.AddTrace(targetNetId, trace.Start, trace.End);
_boardState.RemoveTrace(trace.SegmentId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve source-net state for merged-route undo

Connecting two existing nets mutates topology by copying each source-net trace onto the target net and deleting the original trace, but this merge state is not tracked for rollback. Undo() only removes _addedSegmentIds from the newly drawn route, so after undo the board can remain partially/fully merged instead of restoring the original two-net configuration.

Useful? React with 👍 / 👎.

Comment on lines 170 to 171
// lookup service (IComponentDefinitionProvider). PlacementController handles
// Initialize() when placing via UI interaction. BoardView provides automatic

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Initialize spawned component views with definitions

BoardView instantiates persistent ComponentView objects but never calls Initialize with the component definition, so placed/loaded components skip per-component sprite and label setup and default to prefab visuals. The runtime call site here only sets GridPosition; the only other Initialize(...) usage is the placement preview path, not the actual board instances.

Useful? React with 👍 / 👎.

okenic03-code and others added 4 commits February 17, 2026 00:54
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ives

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
… initialization

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

if (_selectedComponent.Pins != null)
{
for (int i = 0; i < _selectedComponent.Pins.Length; i++)
{

P1 Badge Populate pins when a definition has no explicit pins

Placement only copies pins from _selectedComponent.Pins, so definitions with empty pin arrays create PlacedComponents with zero pins. WireRoutingController selects connection points from PlacedComponent.Pins, so those components become impossible to wire; with the current data this is severe because the component assets used by stages have empty _pins arrays. Add a fallback (for example via StandardPinDefinitions) before creating PinInstances.

ℹ️ 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".

{
foreach (var tc in _currentStage.TestCases)
{
probes.Add(ProbeDefinition.Voltage($"V_{tc.TestName}", tc.TestName));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Target probes with measurable node names

StageManager builds each probe with tc.TestName as the node target, but StageTestCase defines that field as a human-readable test name (tooltip example: "DC Output Check"), and the shipped stage assets use labels like "VOUT Voltage Check" rather than actual net IDs. Because ObjectiveEvaluator also queries SimulationResult.GetVoltage(testCase.TestName), these probes will not match real node names and stage evaluations can fail even when the circuit is correct; this needs a separate probe-node field (or mapping) instead of reusing the display label.

Useful? React with 👍 / 👎.

}

// Neither pin connected — create a new net
string netName = $"NET{_boardState.Nets.Count + 1}";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Name ground-connected nets as ground references

When neither endpoint is already connected, routing always creates NET{n}; however, netlist conversion explicitly drops ComponentKind.Ground and expects ground to be represented by net names (0/GND). This means a net first created from a ground pin never becomes a ground reference, so gameplay-routed circuits can be simulated without a valid reference node (or fail with floating-node behavior).

Useful? React with 👍 / 👎.

…ory textbook

Reorder stages to follow Alexander & Sadiku textbook progression:
- 1-1: Ohm's Law (V=IR, 5V+1kΩ, expect 5.0V)
- 1-2: Series Resistors (KVL, 9V+1kΩ+2.2kΩ, expect 6.19V)
- 1-3: Voltage Divider (Vout=Vin×R2/(R1+R2), 12V+10kΩ+4.7kΩ, expect 3.84V)
- 1-4: Parallel Resistors (KCL, 5V+1kΩ∥2.2kΩ+470Ω, expect 2.97V)
- 1-5: Series-Parallel Combo (9V+1kΩ+(2.2kΩ∥4.7kΩ), expect 5.40V)

All stages now have _probeNode set for proper test evaluation.
Remove LED and Diode from tutorial (moved to later chapters).
- Add FixedPlacement struct for pre-placing V+/GND/Probe on board

- Add PlacedComponent.IsFixed to prevent player removal of fixed components

- Add PinInstanceFactory utility extracted from PlacementController

- StageManager places fixed components after stage load with OUT net creation

- BoardView syncs existing components as defensive safety net

- Exclude fixed components from budget/cost scoring

- Add circuit diagram modal with per-stage description

- Add help overlay with keyboard shortcuts and placement guide
…hortcut

- Populate _fixedPlacements (V+/GND) in tutorial stages 1-1 through 1-5

- Add _circuitDiagramDescription Korean text for each tutorial stage

- Remove V+/GND from _allowedComponents (now pre-placed, not in palette)

- Skip IsFixed components in SaveLoadService serialization

- Add F1 and ? keyboard shortcuts to toggle help overlay
…tches

- Move PinInstanceFactory to CircuitCraft.Data assembly (Core has noEngineReferences)
- Remove UnityEngine dependency from BoardState.cs (Core assembly constraint)
- Rename stage asset files to match m_Name values (Unity requirement)
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fbac57fd03

ℹ️ 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 +46 to +48
_hasCapturedState = true;

_boardState.RemoveComponent(_currentInstanceId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Skip recording non-removable component deletes

Execute() captures undo state and marks _hasCapturedState before verifying that the removal actually happened. For fixed components, BoardState.RemoveComponent returns false, so this command is still added to history even though nothing was deleted; when the player later undoes, Undo() tries to PlaceComponent into an already occupied cell and throws. This is reproducible by pressing Delete on a fixed stage component and then Ctrl+Z, and it breaks the undo stack for the session.

Useful? React with 👍 / 👎.

// Serialize components and their pins
foreach (var comp in boardState.Components)
{
if (comp.IsFixed) continue;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Persist fixed components when saving board state

ConvertToSaveData skips fixed components, but LoadBoard restores into a fresh empty BoardState; during restore, pin connections referencing those omitted instance IDs are dropped (componentIdMap lookup fails), leaving traces/nets disconnected from their fixed anchors. In stages that rely on pre-placed parts, saving and reloading produces a different circuit topology than what the player built.

Useful? React with 👍 / 👎.

…stage load

StageCameraFramer subscribes to StageManager.OnStageLoaded and calls
CameraController.FrameSuggestedArea() to center the camera on the active
board area. Uses a bridge pattern to cross the assembly boundary between
CircuitCraft.Managers (asmdef) and Assembly-CSharp (Controllers).

Scene wiring: StageCameraFramer component added to StageManager GO in
GamePlay scene with refs to StageManager, CameraController, GridSettings.
…s, Korean help text

- Fix TryGetClickedPin fallback: use mouse grid position when hit-view
  component lookup fails, enabling wiring from placed component pins
- Add VerticalTwoPin standard definition for VoltageSource/CurrentSource
  with vertical pin layout matching the +/- icon orientation
- Add DrawVerticalLeads for voltage/current source symbol rendering
- Translate Help overlay text and button labels to Korean
… mode

- HelpOverlayController disables CameraController and WireRoutingController
  when Help modal is open, preventing scroll-wheel zoom and input leaking
- Add ECAD-style Ctrl+W wiring mode toggle in WireRoutingController with
  grid-based pin finding (no raycast dependency), status bar feedback,
  and auto-deactivation when palette component is selected
- Fix stage-title/stage-targets picking-mode to Ignore so overlay labels
  no longer block 3D clicks in game view
- Update Help text with Ctrl+W shortcut documentation (Korean)
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d5aa80279

ℹ️ 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 181 to 185
if (!tc.HasProbeNode)
{
Debug.LogWarning(
$"StageManager: Test case '{tc.TestName}' has no ProbeNode configured. " +
"Skipping probe and evaluation for this test case.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fall back to legacy test name when probe node is empty

RunSimulationAndEvaluateAsync now skips any test case without ProbeNode, and later marks the stage as failed when no valid probes remain. I checked stage assets under Assets/70_Data/10_ScriptableObjects/60_Stages and many shipped stages (for example Stage3-4_DiodeANDGate.asset) only serialize _testName/_expectedVoltage/_tolerance with no _probeNode, so those stages will deterministically end as FAILED (0/0 test cases) even when the circuit is correct. This needs a backward-compatible fallback to the legacy test-name field when ProbeNode is absent.

Useful? React with 👍 / 👎.

Comment on lines 120 to 122
finally
{
UpdateButtonState();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep simulate button disabled until stage evaluation finishes

The finally block re-runs UpdateButtonState() immediately after invoking RunSimulationAndEvaluate(), but that call is fire-and-forget and often returns before _gameManager.IsSimulating flips true. This can re-enable the button in the same frame, so rapid double-clicks can queue overlapping stage evaluations; the second run can hit SimulationManager's "already in progress" early return and leave StageManager consuming stale/null LastSimulationResult.

Useful? React with 👍 / 👎.

…ent handling, button race

- StageManager: fall back to TestName when ProbeNode is empty so legacy
  stage assets no longer auto-fail with 0/0 test cases
- RemoveComponentCommand: early-return for fixed components to prevent
  undo stack corruption when attempting to delete pre-placed parts
- SimulationButtonController: keep simulate button disabled until
  OnStageCompleted fires instead of re-enabling in finally block
- SaveLoadService/BoardSaveData: persist fixed components with isFixed
  flag so save/load round-trips preserve circuit topology
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86b984d5d1

ℹ️ 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 +152 to +154
string netName = IsGroundPin(_startPin) || IsGroundPin(_endPin)
? GroundNetName
: $"NET{_boardState.Nets.Count + 1}";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Generate unique names for newly created nets

Using NET{_boardState.Nets.Count + 1} can reuse an existing net name after net deletions (for example, if NET1 is removed while NET2 remains, the next new net is named NET2 again). BoardToNetlistConverter.GetComponentNodes(...) uses net.NetName as the simulation node identifier, so duplicate names collapse electrically separate nets into one SPICE node, which can produce incorrect voltages/currents and wrong stage evaluation results.

Useful? React with 👍 / 👎.

height = side;
}

_gameManager.ResetBoard(width, height);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reload the stage instead of hard-resetting the board

OnClearBoard directly calls GameManager.ResetBoard(...), which replaces the board with an empty state and skips stage re-initialization (StageManager.LoadStage/fixed placements), so required fixed components disappear. This breaks stage solvability in configured content (for example Stage1-1_OhmsLaw.asset has fixed source/ground while the allowed palette only includes a resistor), leaving users unable to complete the stage after pressing Clear Board unless they manually restart the stage flow.

Useful? React with 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85a39b5244

ℹ️ 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 284 to 285
if (!previousNetId.HasValue || previousNetId.Value == _netId)
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore previous net even when undo removes all segments

Undo currently skips RestorePreviousPinConnection whenever previousNetId == _netId, but _boardState.RemoveTrace(...) can delete that net when the command removes its last trace. This breaks command reversibility for nets that existed before routing but had no traces (for example probe nets created in StageManager), because after undo the pin is left disconnected instead of returning to its original net.

Useful? React with 👍 / 👎.

Comment on lines 231 to 235
foreach (var component in boardState.Components)
{
// Exclude fixed (pre-placed) components from budget calculation
if (component.IsFixed) continue;
var def = _simulationManager.GetComponentDefinition(component.ComponentDefinitionId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Score from an immutable simulation snapshot

This loop computes budget/score inputs from the live boardState after awaiting simulation, so it can diverge from the circuit that was actually simulated if the player edits during the async run. I checked runtime input paths (PlacementController/WireRoutingController) and they do not gate edits on IsSimulating, so users can change components/traces between submit and scoring and receive stars for a different board than the one evaluated.

Useful? React with 👍 / 👎.

… undo net restoration

- StageManager: capture totalCost/boardArea/traceCount before async simulation
  to prevent race condition with live board edits during simulation
- RouteTraceCommand: fix Undo for probe nets that have no traces — restore
  previous net connection even when RemoveTrace auto-deletes the net
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

if (element.Parameters.Count > 0)
{
var model = new DiodeModel(modelName);

P1 Badge Reuse semiconductor models instead of redefining each instance

CreateDiode emits a new DiodeModel every time a diode has parameters, but model names come from shared definitions (GetDiodeModelName), so placing two of the same diode model generates duplicate model entities with the same name in one circuit. SpiceSharp rejects duplicate entity names (its runtime error text includes An entity with the name "{0}" already exists.), which means otherwise valid circuits with repeated semiconductors can fail simulation. Deduplicate model creation per modelName (and apply the same fix pattern to BJT/MOSFET model emission).

ℹ️ 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 +122 to +125
// Note: In a real production environment, we should unregister all callbacks
// However, for brevity and since UI elements are often destroyed with the scene,
// we primarily focus on the main interaction buttons.
// If the controller persists or is pooled, unregistering everything is mandatory.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Unregister settings callbacks when the panel is hidden

OnEnable registers many UI callbacks each time the settings screen is shown, but OnDisable intentionally skips unregistering most of them. In this project the settings screen is toggled active/inactive from the main menu, so handlers accumulate across visits and each slider/dropdown interaction starts firing multiple times (repeating side effects like Screen.SetResolution, quality updates, and PlayerPrefs writes). This should unregister all callbacks (or register once) to avoid repeated invocation.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant