Feature/new build simulators #489

interim17 · 2025-04-21T18:56:08Z

Time Estimate or Size

medium-large

Problem

Final round of controller refactoring of controller and simulator params.

@meganrm @toloudis The approach to most of these changes was discussed in detail at our meeting a few weeks back. I officially futzed with this for too long and then went on vacation. Simularium is paused, would be good to get this done but I know it's not high priority.

More than half of the lines of changes are new tests or moved utils, so the line count isn't as bad as it seems.

After this I will finalize prefetching and controller speed with these new changes and that will get me close to putting simularium work to bed for now.

The utilities that are shared by the BinaryFIleReader and SimulariumController tests I moved to a util file.

Solution

We now have typed interfaces for the argument passed to changeFile, there is no more code path to initialize the controller with params. I've updated the readme to show that changeFile needs to be called imperatively or in a lifecycle method in order to load a trajectory.

I added quite a bit of testing to the controller, where previously there wasn't much. There could be more, but the goal was to add tests relevant to this change set, and establish a base of coverage so that in the future when we add features, adding tests doesn't require as big of alift.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
This change requires updated or new tests

…n conversion complete for the front end

…lient

…arium/simularium-viewer into feature/base-remote-client

…riable

…arium/simularium-viewer into feature/base-remote-client

…n constructor

…arium/simularium-viewer into feature/new-build-simulators

interim17 · 2025-05-06T22:51:06Z

src/controller/index.ts

+    ///// File Changing /////
+
+    private clearFileResources(reuseSimulator = false): void {
+        this.closeConversionConnection();
        this.stop();
-        this.isFileChanging = false;
+        if (!reuseSimulator) {
+            this.simulator = undefined;
+        }
        this.playBackFile = "";
        this.visData.clearForNewTrajectory();
-        this.simulator?.abort();
-        this.pause();
        if (this.visGeometry) {
            this.visGeometry.clearForNewTrajectory();
            this.visGeometry.resetCamera();
        }
    }

-    public handleFileChange(
-        simulariumFile: ISimulariumFile,
-        fileName: string,
-        geoAssets?: { [key: string]: string }
-    ): Promise<FileReturn> {
-        if (!fileName.includes(".simularium")) {
-            throw new Error("File must be a .simularium file");
-        }
-
-        if (geoAssets) {
-            return this.changeFile({ simulariumFile, geoAssets }, fileName);
-        } else {
-            return this.changeFile({ simulariumFile }, fileName);
-        }
-    }
-
-    public cancelCurrentFile(newFileName: string): void {
-        this.isFileChanging = true;
-        this.playBackFile = newFileName;
-
-        // calls simulator.abort()
-        this.stop();
-
-        this.visData.WaitForFrame(0);
-        this.visData.clearForNewTrajectory();
+    public clearFile(): void {
+        this.isFileChanging = false;
+        this.clearFileResources();


clearFile as a part of the public API is not functionally changed, but the business logic has been moved into clearFileResources to allow some internal flexibility when we want to switch between remote files and reuse the simulator.

interim17 · 2025-05-06T22:53:10Z

src/Simulator/ISimulator.ts

+export const getSimulatorClassFromParams = (params?: SimulatorParams) => {
+    if (!params || !params.fileName) {
+        return { simulatorClass: null, typedParams: null };
+    }
+    if ("netConnectionSettings" in params) {
+        return {
+            simulatorClass: RemoteSimulator,
+            typedParams: params as RemoteSimulatorParams,
+        };
+    } else if ("clientSimulatorImpl" in params) {
+        return {
+            simulatorClass: ClientSimulator,
+            typedParams: params as ClientSimulatorParams,
+        };
+    } else if ("simulariumFile" in params) {
+        return {
+            simulatorClass: LocalFileSimulator,
+            typedParams: params as LocalFileSimulatorParams,
+        };
+    } else {
+        return { simulatorClass: null, typedParams: null };
+    }
+};
+


getSimulatorClassFromParams handles generating the right type of simulator for the incoming params, the simulators have runtime checks to make sure they have received the right params as this can be run in JS on the client side. @meganrm outlined this function in comments on previous PR

interim17 · 2025-05-06T22:55:33Z

src/test/SimulariumController.test.ts

+const mockConversionClient = {
+    setOnConversionCompleteHandler: vi.fn().mockImplementation(() => {
+        console.log("mocked handler");
+    }),
+    cancelConversion: vi.fn(),
+    disconnect: vi.fn(),
+    convertTrajectory: vi.fn().mockResolvedValue(undefined),
+    sendSmoldynData: vi.fn().mockResolvedValue(undefined),
+    lastRequestedFile: "",
+};


This doesn't mock the internal functionality of the conversion client or the messages we expect to receive, but rather the presence of the functions and if they get called at the right time. Internal testing out of scope imo.

interim17 · 2025-05-06T23:10:11Z

src/test/SimulariumController.test.ts

+vi.mock("../Simulator/ISimulator", async () => {
+    const actual = await vi.importActual<
+        typeof import("../Simulator/ISimulator")
+    >("../Simulator/ISimulator");
+
+    return {
+        ...actual,
+        getSimulatorClassFromParams: (params?: SimulatorParams) => {
+            const result = actual.getSimulatorClassFromParams(params);
+            if (result.simulatorClass === RemoteSimulator) {
+                return { ...result, simulatorClass: DummyRemoteSimulator };
+            }
+            return result;
+        },
+    };
+});


This may seem a bit convoluted and I'm open to suggestions, but what we are achieving here is getSimulatorClassFromParams returns a dummy remote simulator.

A reason to do this is that by removing the old controller params, we lost the code path to let the client provide the simulator they want to use, and so we need a way to get the dummy in play for testing. This is bit of a hack, but keeps the mess here in the testing and out of the production code.

toloudis · 2025-05-14T18:27:53Z

src/Simulator/ISimulator.ts

+    } else {
+        return { simulatorClass: null, typedParams: null };
+    }
+};


this seems to be the wrong place to put this code. ISimulator should not depend on the files that implement ISimulator.

Also do you think this function could probably be more like a factory method that actually returns a ISimulator instance (call new ClientSimulator, or new RemoteSimulator etc, inside the if statements)?

@meganrm want to weigh in on this? I left this where and as you wrote it for the most part.

toloudis · 2025-05-14T18:28:45Z

src/controller/index.ts

-import { ISimulator } from "../simularium/ISimulator.js";
-import { LocalFileSimulator } from "../simularium/LocalFileSimulator.js";
+import {
+    getSimulatorClassFromParams,


prob could just define getSimulatorClassFromParams in here, or make a new ts module called SimulatorFactory.ts

Moved into SimulatorFactory.ts

toloudis · 2025-05-14T18:30:29Z

src/controller/index.ts

+            "netConnectionSettings" in params &&
+            this.simulator instanceof RemoteSimulator &&
+            this.simulator.isConnectedToRemoteServer() &&
+            this.lastNetConnectionConfig === params.netConnectionSettings


I like this function, but does this need to be a deeper equality check to compare the netConnectionSettings?

Changed to ==

that (changing from === to ==) would be shallower... deeper check would be to compare the equality of the actual properties of the object. (technically double equals is not "shallower" but is less strict)

Ha, I misunderstood your comment

Pushed a more robust check

toloudis · 2025-06-10T15:14:16Z

src/Simulator/types.ts

+    prefetchFrames?: boolean;
+}
+
+export interface ClientSimulatorParams extends BaseSimulatorParams {


just curious, why do these all extend BaseSimulatorParams? it feels like BaseSimulatorParams is never going to grow beyond fileName and ClientSimulatorParams doesn't even have a fileName..
Is BaseSimulatorParams used anywhere outside of this file?

Maybe I'm misunderstanding you, but ClientSimulatorParams does have a "file name" in a sense. If you look at the way the test bed loads client simulators we use strings in the ID field to serve as file name, and the downstream simulator factory method and initialize calls expect all incoming params to have a value for that field.

It's true that you currently can't actually load a client simulator file from your local machine (like via drag and drop, although that feature should exist imho) but even the "file name" for a remote trajectory is more of an ID. I think we could have something saved on octopus with a random string and as long as the server sent the right data back it's just a convention that the remote files have a .simularium extension.

I'd have to poke around but it seems like a pretty baked-in convention/assumption that everything that gets loaded in the viewer has a file name, would take some refactoring in a few places to take it out.

From test bed configs:

{ id: "blood-plasma-1.0.simularium", name: "Blood Plasma", simulatorType: SimulatorModes.remotePrecomputed, }, { id: "TEST_POINTS", name: "Point Simulation", simulatorType: SimulatorModes.localClientSimulator, }, ``` As for the base class we could just make each param interface have fileName rather than use a base class though, I don't remember why we did that.

Maybe trajectoryId or simulationId would be more general than filename then. I think my comment was partly motivated because it seemed odd to introduce a base class for just one single variable.

Ok, pushed a quick change removing the base class but retaining fileName.

If you'd like to also see the move towards a different naming I'll work on that as well, but that naming convention is a bit more spread out so might be more involved.

src/Simulator/ClientSimulator.ts

src/Simulator/LocalFileSimulator.ts

Co-authored-by: toloudis <[email protected]>

…ularium/simularium-viewer into feature/new-build-simulators

toloudis · 2025-07-18T16:24:16Z

examples/src/Viewer.tsx

-    public clearPendingFile() {
-        this.setState({ filePending: null });
-    }
-


quick sanity check: this looks like something important, where the code wanted to know that there was a file in-flight? is it really ok to remove it and within scope of this PR? or is this dead code that was never used?

toloudis

I think this is looking good. Only concern now, is there more work after merging this to update simularium-website? 😢 And mucho testing with existing production simulations from the server!

toloudis · 2025-07-18T16:31:38Z

src/Simulator/SimulatorFactory.ts

+    LocalFileSimulatorParams,
+} from "./types";
+
+export const getSimulatorClassFromParams = (params?: SimulatorParams) => {


Nit: just for the record, a true "factory" would actually CREATE and return instances of these classes. Just for clarity of terminology, as this is sort of a meta-factory which is just finding out what kind of class to create.

toloudis · 2025-07-18T16:32:53Z

src/Simulator/types.ts

+}
+
+export interface ClientSimulatorParams {
+    fileName: string;


I think my comments around this code earlier were based on thinking that for a ClientSimulator, this string is not really a file name.

toloudis · 2025-07-18T16:34:02Z

src/controller/index.ts

is there already a PR in simularium-website that accounts for the interface changes in here?

toloudis · 2025-07-18T16:35:04Z

src/controller/index.ts

+    // export interface NetConnectionParams {
+    // serverIp?: string;
+    // serverPort?: number;
+    // }


can delete now!

toloudis · 2025-07-18T16:36:11Z

src/controller/index.ts

+            newConfig?.serverIp != null &&
+            newConfig?.serverPort != null &&
+            lastConfig.serverIp === newConfig.serverIp &&
+            lastConfig.serverPort === newConfig.serverPort


if you used lastConfig?.serverPort a couple lines up, why not use ?. here?

can we make this shouldReuseSimulator testable to demonstrate it returns true or false for the conditions we care about?... (edit) I found the tests but they are higher level. would be cool if we could actually unit test just this function but i am basically satisfied for this.

meganrm · 2025-08-04T16:47:16Z

I have read through this and looks good, but I still want to test it with the education app.

Could be of interest: One thing I've noticed using the main branch is the "wait for frame" is never really used because the cache is cleared so often the waited for frame gets reset before the frames arrive

interim17 · 2025-08-04T19:37:20Z

I have read through this and looks good, but I still want to test it with the education app.

Could be of interest: One thing I've noticed using the main branch is the "wait for frame" is never really used because the cache is cleared so often the waited for frame gets reset before the frames arrive

I'll check that out. It's all getting a little foggy, but in some branch related to prefetching I have a callback based version of waitForFrame that is more robust. Could also try to make sure we aren't running it when its not needed.

meganrm · 2025-08-05T18:35:03Z

src/controller/index.ts

+        if (
+            "simulariumFile" in params &&
+            !params.fileName.includes(".simularium")
+        ) {
+            this.handleError("File must be a .simularium file");
+            return Promise.reject({ status: FILE_STATUS_FAIL });
+        }


I'm trying to use this for the binding affinity app. For local files, simulariumFile is the implementation, not a filename, so it's not going to include a .simularium extension.

I removed these lines and the file loaded. For context, this is how I call changeFile:

const response = await fetch(url); if (response.ok) { const blob = await response.blob(); const simulariumFile = await loadSimulariumFile(blob); await simulariumController.changeFile({ fileName: "example", simulariumFile: simulariumFile, }); const plotData = simulariumFile.getPlotData(); setTrajectoryPlotData(plotData[0].data as ScatterTrace[]); // we're not using histograms } else { throw new Error(`Failed to fetch - ${response.status}`); }

meganrm · 2025-08-05T18:44:43Z

if you haven't already, I think you should make a branch on the website repo that installs this version and make sure every way of loading a file works: url, local, drag and drop local, examples etc

toloudis · 2025-08-05T18:55:36Z

if you haven't already, I think you should make a branch on the website repo that installs this version and make sure every way of loading a file works: url, local, drag and drop local, examples etc

YES. test all the things.

interim17 added 29 commits March 17, 2025 15:58

remove loadInitialData/postConnect flow and controller params

43693cc

fix placeholder test

19fc1b7

move plots and metrics API into ISimulator

aa0dcee

remove server health checks

58c9324

extend BaseRemoteClient for classes that need websockets and handle o…

5e409f4

…n conversion complete for the front end

update tests for remote simulator and add tests for base remote client

18f282a

make DummyRemoteSimulator properly extend RemoteSimulator

c3c1bde

separate conversion client getter and connection check

1571508

remove code path for requesting remote metrics for local file

b38f79a

implement callback pattern for plots and metrics

7a28fd2

Merge branch 'feature/isimulator-plot-api' into feature/base-remote-c…

b8344b4

…lient

Merge branch 'feature/controller-updates' of https://github.com/simul…

74ecc3c

…arium/simularium-viewer into feature/base-remote-client

make remote abstractions extend web socket client

13adefa

get unused file name arg out of test

8ce4a6b

use file name from params

bf6f3d7

use composition over inheritance with websocket client

caee3d4

fix tests for remote simulator to use websocket client as composed va…

69f405a

…riable

use old capitalization scheme on web socket client

5f05fa0

use old capitalization scheme on web socket client

8161f72

Merge branch 'feature/base-remote-client' of https://github.com/simul…

b4a51f5

…arium/simularium-viewer into feature/base-remote-client

Merge branch 'feature/base-remote-client' of https://github.com/simul…

ac0c092

…arium/simularium-viewer into feature/base-remote-client

Merge branch 'feature/base-remote-client' of https://github.com/simul…

fb4dd2e

…arium/simularium-viewer into feature/base-remote-client

pass file name to conversion client when calling methods instead of i…

cdfe7cb

…n constructor

reorg for new simulator params and controller testing

6f65909

move testing binary utils into separate file

aa10bd1

update readme

1577a0a

Merge branch 'feature/controller-updates' of https://github.com/simul…

6677ff3

…arium/simularium-viewer into feature/new-build-simulators

dont unset simulator in call of stop

1aa7e30

fix linting errors on controller tests for empty arrow functions

6e196ae

interim17 commented May 6, 2025

View reviewed changes

only mock the result of getSimulatorClassFromParams if its a remote sim

784019c

interim17 commented May 6, 2025

View reviewed changes

interim17 marked this pull request as ready for review May 12, 2025 17:50

interim17 requested a review from a team as a code owner May 12, 2025 17:50

interim17 requested review from frasercl, meganrm and toloudis and removed request for a team and frasercl May 12, 2025 17:50

toloudis reviewed May 14, 2025

View reviewed changes

make new module SimulatorFactory

8780cf0

interim17 requested a review from toloudis May 27, 2025 17:41

toloudis reviewed Jun 10, 2025

View reviewed changes

deeper equality check on net connection configs

24b1330

toloudis reviewed Jul 7, 2025

View reviewed changes

src/Simulator/ClientSimulator.ts Outdated Show resolved Hide resolved

toloudis reviewed Jul 7, 2025

View reviewed changes

src/Simulator/LocalFileSimulator.ts Outdated Show resolved Hide resolved

interim17 and others added 4 commits July 7, 2025 10:56

Update src/Simulator/ClientSimulator.ts

c6516c4

Co-authored-by: toloudis <[email protected]>

Update src/Simulator/LocalFileSimulator.ts

46bb76f

Co-authored-by: toloudis <[email protected]>

Merge branch 'feature/new-build-simulators' of https://github.com/sim…

34fca1c

…ularium/simularium-viewer into feature/new-build-simulators

remove basesimulatorparams class

591b000

interim17 requested a review from toloudis July 9, 2025 17:02

toloudis reviewed Jul 18, 2025

View reviewed changes

toloudis approved these changes Jul 18, 2025

View reviewed changes

meganrm reviewed Aug 5, 2025

View reviewed changes

Feature/new build simulators #489

Are you sure you want to change the base?

Feature/new build simulators #489

Uh oh!

Conversation

interim17 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Time Estimate or Size

Problem

Solution

Type of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interim17 May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toloudis Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toloudis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meganrm commented Aug 4, 2025

Uh oh!

interim17 commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meganrm commented Aug 5, 2025

Uh oh!

interim17 commented Apr 21, 2025 •

edited

Loading

interim17 May 6, 2025 •

edited

Loading

toloudis Jun 9, 2025 •

edited

Loading

interim17 commented Aug 4, 2025 •

edited

Loading