Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/viewer errors #343

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Fix/viewer errors #343

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43d17f1
fixed errors in viewer
meganrm Nov 1, 2023
95d91f3
revert to old default
meganrm Nov 15, 2023
e8297f5
add commented out import statement and note
meganrm Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions examples/Viewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@ import type {
ISimulariumFile,
UIDisplayData,
SelectionStateInfo,
SelectionEntry,
} from "../type-declarations";
import SimulariumViewer, {
SimulariumController,
RenderStyle,
ErrorLevel,
FrontEndError,
loadSimulariumFile,
} from "../es";
FrontEndError,
ErrorLevel,
} from "../src/index";
Copy link
Contributor

Choose a reason for hiding this comment

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

The original from "../es" was there intentionally to help simulate an environment more like simularium-website where we are loading from the packaged build. This was relevant because of differences in the way the web worker and style sheets are loaded, for example. So if changing this, we should be intentional about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it seemed like this was what was causing a lot of the typing errors and it means you don't have to rebuild to make sure all your changes are reflected in the app. I don't see any differences after changes these to the way the app behaves. do you know of something I could test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change the server to production because the staging one is down, but I was able to load COVIDLUNG and it seemed to behave the same in both cases (changing the imports)

Copy link
Contributor

@toloudis toloudis Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is expected to behave the same if you :

  • use from ../es and run npm build before npm start
  • compared to use from ../src and don't build before start.

Main thing to test is loading a trajectory with pdbs.
But the ../es case is more important to make sure things are working for production.
Being intentional probably means just keeping it easy to switch between the two (keep all the imports in this one statement if possible) and maybe put something in the readme, and a comment at the import to say what to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I know, I meant is there any behavior you know about that does behave differently in the two states? Because either way we're running the same commands, ie, npm start calls npm build and the dev server setup is supposed to serve the built artifacts just while watching for changes. I'm in favor of using the /src because it removes the typescript errors so it's more obvious when we do have an actual error in the test bed viewer. But I can add a commented out import statement? Is that what you're suggesting?

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no actual differences in runtime behavior between the two cases of importing from src vs es. In the past, however, there HAVE been differences which had to be fixed. Loading it here lets you find the error before you test in simularium-website. This is the reason why this demo app is loading from es/.

There definitely shouldn't be any typescript errors when we import from the es/ dir, so if there are, maybe that means we need to make sure that the test app can see the type-declarations directory.

Copy link
Contributor

@toloudis toloudis Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So I would like to keep the ability to run the test app with the post-build artifacts.

In order to support both situations (import from es and requiring a npm build, versus importing from src and not requiring a npm build) I would suggest a separate start script in package.json and some kind of dev-or-production switch in the code. The alternative is to have a commented-out version of both like this:

import foo, bar, baz from 'src';
// import foo, bar, baz from 'es';

As long as there is only one place to do this, then you have to just change the comment to make it use the right thing. And then every PR from now on will have this comment/uncomment battle... 🤣

import "../style/style.css";

import PointSimulator from "./PointSimulator";
import PointSimulatorLive from "./PointSimulatorLive";
import PdbSimulator from "./PdbSimulator";
Expand Down Expand Up @@ -118,7 +118,9 @@ interface InputParams {
useOctopus: boolean;
}

const simulariumController = new SimulariumController({});
const simulariumController = new SimulariumController(
{}
);

let currentFrame = 0;
let currentTime = 0;
Expand Down Expand Up @@ -147,8 +149,6 @@ const initialState: ViewerState = {
simulariumFile: null,
};

type FrontEndError = typeof FrontEndError;

class Viewer extends React.Component<InputParams, ViewerState> {
private viewerRef: React.RefObject<SimulariumViewer>;
private panMode = false;
Expand Down Expand Up @@ -205,6 +205,7 @@ class Viewer extends React.Component<InputParams, ViewerState> {
viewerContainer.addEventListener("drop", this.onDrop);
viewerContainer.addEventListener("dragover", this.onDragOver);
}
this.configureAndLoad()
}

public onDragOver = (e: Event): void => {
Expand Down Expand Up @@ -273,6 +274,7 @@ class Viewer extends React.Component<InputParams, ViewerState> {
acc[cur.name] = parsedFiles[index];
return acc;
}
return acc;
}, {});
const fileName = filesArr[simulariumFileIndex].name;
this.loadFile(simulariumFile, fileName, geoAssets);
Expand Down Expand Up @@ -399,7 +401,7 @@ class Viewer extends React.Component<InputParams, ViewerState> {

public turnAgentsOnOff(nameToToggle: string) {
let currentHiddenAgents = this.state.selectionStateInfo.hiddenAgents;
let nextHiddenAgents = [];
let nextHiddenAgents: SelectionEntry[] = [];
if (currentHiddenAgents.some((a) => a.name === nameToToggle)) {
nextHiddenAgents = currentHiddenAgents.filter(
(hiddenAgent) => hiddenAgent.name !== nameToToggle
Expand All @@ -422,7 +424,7 @@ class Viewer extends React.Component<InputParams, ViewerState> {
public turnAgentHighlightsOnOff(nameToToggle: string) {
let currentHighlightedAgents =
this.state.selectionStateInfo.highlightedAgents;
let nextHighlightedAgents = [];
let nextHighlightedAgents: SelectionEntry[] = [];
if (currentHighlightedAgents.some((a) => a.name === nameToToggle)) {
nextHighlightedAgents = currentHighlightedAgents.filter(
(hiddenAgent) => hiddenAgent.name !== nameToToggle
Expand Down Expand Up @@ -631,7 +633,9 @@ class Viewer extends React.Component<InputParams, ViewerState> {
<div className="container" style={{ height: "90%", width: "75%" }}>
<select
onChange={(event) => {
simulariumController.stop();
playbackFile = event.target.value;
this.configureAndLoad();
}}
defaultValue={playbackFile}
>
Expand Down Expand Up @@ -683,10 +687,9 @@ class Viewer extends React.Component<InputParams, ViewerState> {
<option value="TEST_FIBERS">TEST FIBERS</option>
<option value="TEST_POINTS">TEST POINTS</option>
<option value="TEST_METABALLS">TEST METABALLS</option>
<option value="TEST_BINDING">TEST BINDING</option>
</select>
<button onClick={() => this.configureAndLoad()}>
Load model
</button>

<button onClick={() => this.translateAgent()}>
TranslateAgent
</button>
Expand Down