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

Feature/session colors #400

Merged
merged 30 commits into from
Aug 20, 2024
Merged

Feature/session colors #400

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
e5bc443
add util for comparing new and old ui data to get color changes
interim17 May 23, 2024
65985e3
take sessionUIData prop in viewport and add method to apply color cha…
interim17 May 23, 2024
1b04d38
throw error if ui data don't match
interim17 May 28, 2024
f5935c6
Merge branch 'main' of https://github.com/simularium/simularium-viewe…
interim17 Jun 4, 2024
130044b
don't throw errors when ui data doesn't match session data
interim17 Jun 5, 2024
687e4f3
call receiveSessionUIData in onTrajectoryFileInfo
interim17 Jun 5, 2024
40d9425
use controller methods to handle color changes
interim17 Jun 10, 2024
4ca2123
remove ColorChange from SelectionStateInfo
interim17 Jun 10, 2024
5f27d77
Merge branch 'main' of https://github.com/simularium/simularium-viewe…
interim17 Jun 28, 2024
fc3f96f
initial consolidation of updateAgentColors and updateUiDataColor
interim17 Jul 8, 2024
f3e0816
remove controller methods for applying color changes
interim17 Jul 10, 2024
d015892
go back to using SelectionStateInfo prop to receive and apply color s…
interim17 Jul 10, 2024
627c414
csonsolidate changeAgentsColor and handler, and remove named ColorSet…
interim17 Jul 18, 2024
dfb53d9
remove unused types, imports, and comments
interim17 Jul 18, 2024
6f9bfa8
fix whitespace
interim17 Jul 18, 2024
9678d19
remove unused agnet name from color update arguments
interim17 Jul 18, 2024
1bab4fe
rename getIdsAndColorsFromUIData
interim17 Jul 18, 2024
d6a6a76
remove duplicate length check
interim17 Jul 18, 2024
a355255
rename colorSettings to appliedColors in SelectionStateInfo
interim17 Jul 18, 2024
e3f8f82
revert updateUiDataColor to not update entries object
interim17 Jul 18, 2024
cc52ba7
rename argument for changeAgentsColor
interim17 Jul 18, 2024
6cae2d5
rename settings to colorAssignments
interim17 Jul 18, 2024
a158e4e
use new UIDisplayData structure in applying color changes in test bed…
interim17 Jul 19, 2024
8e54623
consolidate helper function into changeAgentsColor
interim17 Jul 19, 2024
98662fb
declare ids before applying to agents for readability in changeAgents…
interim17 Jul 19, 2024
2337d40
typo fix
interim17 Jul 19, 2024
a9b8fed
Merge branch 'main' of https://github.com/simularium/simularium-viewe…
interim17 Aug 12, 2024
311175c
use appliedColors in place of uiDisplayData in test bed state
interim17 Aug 13, 2024
f2de0a1
process batches of assigments in when applying colors to agents to re…
interim17 Aug 13, 2024
80bb750
overwrite subagent colors when color changes are applied to parent
interim17 Aug 13, 2024
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
35 changes: 23 additions & 12 deletions examples/src/ColorPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import React, { useState } from "react";
import { SelectionEntry, UIDisplayData } from "../../src";
import { UIDisplayData } from "../../src";

type ColorPickerProps = {
uiDisplayData: UIDisplayData;
particleTypeNames: string[];
agentColors: string[] | number[];
setColorSelectionInfo: (data: {
agent: SelectionEntry;
color: string;
}) => void;
setColorSelectionInfo: (data: UIDisplayData) => void;
updateAgentColorArray: (color: string) => void;
};

Expand Down Expand Up @@ -54,14 +51,28 @@ const ColorPicker = ({
if (selectedSubagent === "<unmodified>") {
subAgent = [""];
}
const entry: SelectionEntry = {
name: selectedAgent,
tags: subAgent,
};
setColorSelectionInfo({
agent: entry,
color: selectedColor,
const appliedColors = uiDisplayData.map((agent) => {
const newAgent = { ...agent };
if (agent.name === selectedAgent) {
if (subAgent.includes("")) {
newAgent.color = selectedColor;
}
const newDisplayStates = agent.displayStates.map(
(state: any) => {
if (subAgent.includes(state.id)) {
return {
...state,
color: selectedColor,
};
}
return state;
}
);
newAgent.displayStates = newDisplayStates;
}
return newAgent;
Copy link
Contributor Author

@interim17 interim17 Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Preview of front end work here in the test bed. Instead of passing in a colorChange we send in a uiDisplayData

In pairing w @meganrm we decided to make the front end responsible for generating and storing this state, viewer just renders

});
setColorSelectionInfo(appliedColors);
}
};

Expand Down
7 changes: 4 additions & 3 deletions examples/src/Viewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ const initialState: ViewerState = {
selectionStateInfo: {
highlightedAgents: [],
hiddenAgents: [],
colorChange: null,
appliedColors: [],
},
filePending: null,
simulariumFile: null,
Expand Down Expand Up @@ -676,14 +676,15 @@ class Viewer extends React.Component<InputParams, ViewerState> {
this.setState({ agentColors });
};

public setColorSelectionInfo = (colorChange) => {
public setColorSelectionInfo = (appliedColors) => {
this.setState({
...this.state,
uiDisplayData: appliedColors,
selectionStateInfo: {
hiddenAgents: this.state.selectionStateInfo.hiddenAgents,
highlightedAgents:
this.state.selectionStateInfo.highlightedAgents,
colorChange: colorChange,
appliedColors: appliedColors,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that state is now duplicated here. I don't have quite enough context on why this would be necessary, or whether this change would imply a similar duplication in the actual simularium-website code. But, particularly if it does, I'd ask a couple questions:

  • If these two values got out of sync with each other, would that represent a meaningful UI state or be some sort of error?
  • If it would be an error, could you get by with just one copy? e.g. could everything that's currently being driven by uiDisplayData instead be driven by appliedColors?
  • It looks like both these state fields are related to props on SimulariumViewer (onUiDisplayDataChanged, selectionStateInfo). If the type of one now contains the other, could/should those paths be merged in the viewer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently no error will throw, but display will be incorrect if these two are out of sync after more than one color selection.

In the website we will be storing two possibilities for color state (so that we can toggle between default and user selections). The onUiDisplayDataChanged callback will set the default value, and the user selections will be stored separately. The UI components and the redux selector for the selectionStateInfo prop will grab the appropriate one without duplicating the state.

Here we could apply the suggestion from your second bullet, removing the uiDisplayData property from viewerState and driving everything with selectionStateInfo.appliedColors, because we only ever track the current state and never re-apply the default. Not duplicating state = good. But we will be slightly less in sync with naming/conventions of website.

In regards to merging the paths related to onUiDisplayDataChanged and selectionStateInfo:
I did this initially, but it created issues. Essentially now onUiDisplayDataChanged is for parsing new trajectories, whereas selectionStateInfo is about for responding to user changes. Unlikely to merge the paths although open to feedback and also maybe in need of renaming/documenting?

});
};
Expand Down
2 changes: 1 addition & 1 deletion src/simularium/SelectionInterface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export interface ColorChange {
export interface SelectionStateInfo {
highlightedAgents: SelectionEntry[];
hiddenAgents: SelectionEntry[];
colorChange: ColorChange | null;
appliedColors: UIDisplayData;
}

interface DisplayStateEntry {
Expand Down
16 changes: 8 additions & 8 deletions src/test/SelectionInterface.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const idMapping = {
};

const color = "";
const initialColorChanges = null;
const initialAppliedColors = [];

describe("SelectionInterface module", () => {
describe("decode", () => {
Expand Down Expand Up @@ -232,7 +232,7 @@ describe("SelectionInterface module", () => {
{ name: "D", tags: [] },
],
hiddenAgents: [],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't appear to matter to these tests currently, but be careful: these tests are now taking the same copy of initialAppliedColors by reference, and modifications made by one test could be visible to all the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use spread operator

};
const ids = si.getHighlightedIds(selectionStateHighlight);
const allAs = [0, 1, 2, 3];
Expand All @@ -252,7 +252,7 @@ describe("SelectionInterface module", () => {
{ name: "D", tags: [""] },
],
hiddenAgents: [],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
};
const ids = si.getHighlightedIds(selectionStateHighlight);

Expand All @@ -269,7 +269,7 @@ describe("SelectionInterface module", () => {
{ name: "E", tags: ["t1000"] },
],
hiddenAgents: [],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
};
const ids = si.getHighlightedIds(selectionStateHighlight);

Expand All @@ -282,7 +282,7 @@ describe("SelectionInterface module", () => {
const selectionStateHighlight = {
highlightedAgents: [{ name: "E", tags: [""] }],
hiddenAgents: [],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
};
const ids = si.getHighlightedIds(selectionStateHighlight);

Expand All @@ -300,7 +300,7 @@ describe("SelectionInterface module", () => {
{ name: "A", tags: [] },
{ name: "C", tags: [] },
],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
};
const ids = si.getHiddenIds(selectionStateHide);

Expand All @@ -316,7 +316,7 @@ describe("SelectionInterface module", () => {
{ name: "A", tags: ["t1", "t2"] },
{ name: "B", tags: ["t1"] },
],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
};
const ids = si.getHiddenIds(selectionStateHide);
expect(ids).toEqual([1, 2, 3, 5, 7]);
Expand All @@ -332,7 +332,7 @@ describe("SelectionInterface module", () => {
{ name: "A", tags: [""] },
{ name: "C", tags: ["", "t1", "t2"] },
],
colorChange: initialColorChanges,
appliedColors: initialAppliedColors,
};
const ids = si.getHiddenIds(selectionStateHide);

Expand Down
33 changes: 22 additions & 11 deletions src/viewport/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { AgentData, TrajectoryFileInfoAny } from "../simularium/types";
import { updateTrajectoryFileInfoFormat } from "../simularium/versionHandlers";
import { FrontEndError, ErrorLevel } from "../simularium/FrontEndError";
import { RenderStyle, VisGeometry, NO_AGENT } from "../visGeometry";
import { ColorChange } from "../simularium/SelectionInterface";
import FrameRecorder from "../simularium/FrameRecorder";
import { DEFAULT_FRAME_RATE } from "../constants";

Expand Down Expand Up @@ -342,12 +341,12 @@ class Viewport extends React.Component<
}
if (
!isEqual(
selectionStateInfo.colorChange,
prevProps.selectionStateInfo.colorChange
selectionStateInfo.appliedColors,
prevProps.selectionStateInfo.appliedColors
) &&
selectionStateInfo.colorChange !== null
selectionStateInfo.appliedColors.length > 0
) {
this.changeAgentsColor(selectionStateInfo.colorChange);
this.changeAgentsColor(selectionStateInfo.appliedColors);
}
}

Expand Down Expand Up @@ -596,12 +595,24 @@ class Viewport extends React.Component<
}
}

public changeAgentsColor(colorChange: ColorChange): void {
const { agent, color } = colorChange;
const agentIds = this.selectionInterface.getAgentIdsByNamesAndTags([
agent,
]);
this.visGeometry.applyColorToAgents(agentIds, color);
public changeAgentsColor(appliedColors: UIDisplayData): void {
appliedColors.forEach((agent) => {
const agentIds = this.selectionInterface.getAgentIdsByNamesAndTags([
{ name: agent.name, tags: [] },
]);
this.visGeometry.applyColorToAgents(
agentIds,
agent.color
);

agent.displayStates.forEach((state) => {
const stateIds =
this.selectionInterface.getAgentIdsByNamesAndTags([
{ name: agent.name, tags: [state.name] },
]);
this.visGeometry.applyColorToAgents(stateIds, state.color);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Under the hood, applyColorToAgents is calling VisGeometry.updateScene, which looks like it may be expensive. Now that this call is happening in a loop, is there a way to ensure that the update just happens once, at the end of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and why not, its cleaner! Also even if updateScene is relatively cheap (cheap enough to get called in every animate loop at least), it might get more expensive in some upcoming work on caching, so I like the idea of minimizing calls where we can.

});
}

public stopAnimate(): void {
Expand Down
Loading