Skip to content

Config Reverting [SYNTH-201]#1425

Open
rutmanz wants to merge 20 commits into
devfrom
zachr/201/config-reveting
Open

Config Reverting [SYNTH-201]#1425
rutmanz wants to merge 20 commits into
devfrom
zachr/201/config-reveting

Conversation

@rutmanz

@rutmanz rutmanz commented Jul 17, 2026

Copy link
Copy Markdown
Member

Task

SYNTH-201

Symptom

Previously, config reverting was somewhat suspicious and didn't always revert everything as would be expected

Solution

Added a registerCleanupCallback function to all configuration subpanels, which allows revert callbacks to be applied locally within each subpanel. This means each subpanel is responsible for reverting its own changes, making it much clearer what's going on. The system also supports confirm callbacks, but as most changes are applied immediately, this is only used for some memory freeing right now.

Verification

Spawn an asset, change config things, and try to find a way to have changes not revert on cancel.


Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@rutmanz
rutmanz requested review from a team as code owners July 17, 2026 19:22
@rutmanz
rutmanz requested review from 0xda157, AlexD717, PepperLola, azaleacolburn and elliefre and removed request for elliefre July 17, 2026 19:22

@0xda157 0xda157 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there's a lot going on here that seems unrelated to the goal of the pr and makes it quite difficult to review imo. splitting this up into a few commits would make me a lot more confident about reviewing it

Comment thread fission/src/systems/input/InputSchemeManager.ts Outdated
Comment thread fission/src/systems/simulation/Brain.ts
import type SynthesisBrain from "@/systems/simulation/synthesis_brain/SynthesisBrain.ts"
import type WPILibBrain from "@/systems/simulation/wpilib_brain/WPILibBrain.ts"

export type BrainType = "synthesis" | "wpilib" | "unknown"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is braintype ever unknown? I only seen WPILibBrain and SynthesisBrain as implementations of this class.

@rutmanz

rutmanz commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Not sure what you want me to do about that. I agree it would make more sense as multiple commits, but just due to how I was working on this that didn't make sense to me in the moment (I kept getting interrupted and doing things out of order). It's something I'll be more mindful of going forward, but I also don't think it makes a lot of sense to try and retroactively split into multiple commits because the intent is already lost.

I had Claude split my commit up into features (as it saw fit). I checked that the diff is the same, and it should be easier for you to review. The groups were made by Claude though, so they are a little off, but I don't think putting more effort into that is a good use of time.

@rutmanz rutmanz closed this Jul 17, 2026
@rutmanz
rutmanz deleted the zachr/201/config-reveting branch July 17, 2026 21:23
@rutmanz

rutmanz commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

oh didn't mean to close this let me work on that

@rutmanz
rutmanz restored the zachr/201/config-reveting branch July 17, 2026 21:38
@rutmanz rutmanz reopened this Jul 17, 2026
rutmanz added 11 commits July 17, 2026 17:49
Rename the public field to a private `_brainIndexSchemeMap` with a
public getter, and add `getBrainIndexSchemeMapping()` alongside the
existing `setBrainIndexSchemeMapping()` so all map access goes through
explicit methods.
Replace `instanceof` and `brainType ===` checks with narrowing type
guards on Brain. Also rename `getPositionTransform` to
`getXZPositionTransform` for clarity, add `getRootBody()` helper to
MirabufSceneObject, and remove the AutoTestPanel shortcut from the
WPILib context menu.
Makes SelectMenu<OptionType> type-safe so callers no longer need to
cast `val as SomeSelectionOption`. Removes redundant key prop from
ItemCard in ImportMirabufPanel and updates WiringPanel accept text.
Add CleanupRegisterFunction, ConfigurationSubpanelProps, and
ConfigurationSubpanelComponent types that all config subpanels will
implement. Rename SUBSYSTEMS→JOINT_SUBSYSTEMS and
SEQUENTIAL→JOINT_SEQUENCE for clarity. Move the per-configuration-type
mode lists (synthesisRobotConfigModes, wpilibRobotConfigModes,
fieldConfigModes) out of ConfigurePanel into ConfigTypes.
… system

Remove the monolithic ConfigInterface switch and the per-panel
originalPrefs refs. Replace with a registerCleanupFunction callback
that each subpanel calls to register its own apply/revert logic.
ConfigurePanel accumulates these and runs them in FIFO order on accept
or LIFO on cancel.

Extract MoveInterface and ControlsConfigInterface as standalone
subpanel components. Add ConfirmChangesModal for the case where the
user navigates away from Controls with unsaved changes.
…a panels

Convert each panel to ConfigurationSubpanelComponent and register a
revert callback that restores the original value when the user cancels.
Convert ConfigureGamepiecePickupInterface and
ConfigureShotTrajectoryInterface to ConfigurationSubpanelComponent and
register revert callbacks that restore intake/ejector preferences on
cancel.
Convert ConfigureSubsystemsInterface and SequentialBehaviorsInterface
to ConfigurationSubpanelComponent with revert callbacks. Fix
SubsystemRowInterface to track invertMotor state locally and restrict
the unstick-force slider to the drivetrain group only.
Convert ConfigureScoringZonesInterface, ConfigureProtectedZonesInterface,
and ConfigureCameraPointsInterface to ConfigurationSubpanelComponent.
Each registers a revert callback that restores the original zone/camera
point lists and re-syncs the scene on cancel.
Convert SimulationInterface to ConfigurationSubpanelComponent and
register a revert callback for SimAutoReconnect. Thread
registerCleanupFunction through ConfigureInputsInterface and
ConfigureSchemeInterface so input binding changes can also be reverted
on cancel.
@rutmanz
rutmanz force-pushed the zachr/201/config-reveting branch from 4bf06af to 68ac376 Compare July 18, 2026 01:40
@rutmanz
rutmanz requested a review from a team as a code owner July 18, 2026 01:40
@rutmanz
rutmanz requested a review from 0xda157 July 20, 2026 15:17
Comment thread fission/src/mirabuf/MirabufSceneObject.ts
Comment thread fission/src/ui/panels/configuring/assembly-config/ConfigurePanel.tsx Outdated
@rutmanz
rutmanz requested a review from Dhruv-0-Arora July 21, 2026 17:42

@AlexD717 AlexD717 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The inputs configuration do not revert when opening a panel that closes the configure panel. Also if a panel were to close the configure panel the ConfirmChangesModal should probably open.

Screen.Recording.2026-07-22.at.1.20.37.PM.mov

@rutmanz
rutmanz requested a review from 0xda157 July 23, 2026 17:01
@rutmanz

rutmanz commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

These can't use the "correct" react pattern of setting to new objects because I'm trying to mutate the underlying objects that selectedAssembly and selectedScheme reference. Ideally they would be keyed and the react state is based on that key, but I think that's out of scope for this PR (SYNTH-251 tracks this for schemes)

@rutmanz
rutmanz requested a review from AlexD717 July 24, 2026 18:08
@rutmanz

rutmanz commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

@AlexD717, rather than try to interrupt the panel closing process, which would be rather obnoxious, I reframed the language and behavior so that save is the default and reverting is explicitly "revert" instead of "cancel." Does that seem like an appropriate way to deal with this?

@rutmanz
rutmanz force-pushed the zachr/201/config-reveting branch from 2e78e6e to 28a2c4d Compare July 24, 2026 18:10
@AlexD717

Copy link
Copy Markdown
Member

That seems reasonable

@AlexD717 AlexD717 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lotm

Note: there are some merge conflicts

@PepperLola PepperLola added ui/ux Relating to user interface, or in general, user experience gameplay Relating to the playability of Synthesis refactor The most important part of software development. labels Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gameplay Relating to the playability of Synthesis refactor The most important part of software development. ui/ux Relating to user interface, or in general, user experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants