Feature/session colors #400

interim17 · 2024-06-28T23:17:23Z

Time Estimate or Size

medium

related website branchs:
fix/update-ssi -> a patch to bring website into alignment with this changeset
feature/color-session -> the end goal for the feature in development

Advances #510 and #511

Problem

On the front end we need to be storing and applying sets of color changes (browser sessions, or re-applying default). Current code assumes one-color-change-at-a-time.

Solution

UIDisplayData is a good existing data structure that conveys a snapshot of agent tree and its colors.

In SelectionStateInfo colorChanges have been replaced by appliedColors of type UIDisplayData

This is more consistent with the other parts of the selection object which serves as a snapshot of the selection state, rather than as a pass-through for a series of changes.

Keeping this change set small. Possible further changes would keep the SelectionInterface entries object in sync with "rendered state"... Might be useful for writing changes out to new files, not needed right now for MVP.

Type of change

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

…nges from that data

…into feature/session-colors

github-actions · 2024-06-28T23:19:14Z

jest coverage report 🧪

Total coverage

Status	Category	Percentage	Covered / Total
🔴	Statements	40.95%	2043/4989
🔴	Branches	43.21%	838/1939
🔴	Functions	36.92%	418/1132
🔴	Lines	41.17%	1956/4751

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show files with reduced coverage 🔻

Reduced coverage

Status	Filename	Statements	Branches	Functions	Lines
🔴	index.ts	7.69%	7.25%	5.64%	7.65%
🔴	src/viewport	14.09%	10.52%	13.95%	13.65%
🔴	index.tsx	14.09%	10.52%	13.95%	13.65%
🔴	src/visGeometry	21.76%	22.79%	27.5%	21.63%

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

…ettings from front end

…ting type from setInitialAgentColors

… viewer

interim17 · 2024-07-19T05:08:50Z

src/index.ts

@@ -28,6 +28,6 @@ export {
 } from "./simularium";
 export { compareTimes, loadSimulariumFile } from "./util";
 export { DEFAULT_CAMERA_SPEC } from "./constants";
-export { AgentData } from "./simularium/types";
+export type { AgentData } from "./simularium/types";


small bug fix folded in here

this should be included in the types imports up above and should be exported directly from "./simularium" like the rest of them are. probably should be it's own PR because currently it breaks running the viewer on main

thank you, PR here.

interim17 · 2024-07-19T05:15:10Z

examples/src/ColorPicker.tsx

+            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;


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

interim17 · 2024-07-19T05:26:40Z

src/viewport/index.tsx

+    public changeAgentsColor(appliedColors: UIDisplayData): void {
+        const colorSettings =
+            this.selectionInterface.getIdsAndColorsFromUIData(appliedColors);
+        colorSettings.forEach((setting) => {
+            this.visGeometry.applyColorToAgents(
+                setting.agentIds,
+                setting.color
+            );
+        });


This function now takes in a snapshot of all the colors currently applied, rather than processing individual changes one at a time.

…Color

meganrm

looks good, I would pull out the AgentData fix into it's own PR and make sure the exports are consistent with how we're exporting all the other types from the system

…into feature/session-colors

interim17 · 2024-08-13T16:15:26Z

looks good, I would pull out the AgentData fix into it's own PR and make sure the exports are consistent with how we're exporting all the other types from the system

For context for other reviewers, this is done, the fix has been merged into both main and this branch.

frasercl

Couple of concerns, none of which I have enough context on to be sure how concerned I should be. Approving to let you merge at your discretion.

frasercl · 2024-08-13T18:03:28Z

examples/src/Viewer.tsx

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


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?

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?

frasercl · 2024-08-13T18:06:52Z

src/test/SelectionInterface.test.ts

@@ -232,7 +232,7 @@ describe("SelectionInterface module", () => {
                    { name: "D", tags: [] },
                ],
                hiddenAgents: [],
-                colorChange: initialColorChanges,
+                appliedColors: initialAppliedColors,


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.

updated to use spread operator

frasercl · 2024-08-13T18:16:16Z

src/viewport/index.tsx

+        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);
+            });


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?

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.

…duce calls of updateScene

interim17 · 2024-08-13T21:25:21Z

src/visGeometry/index.ts

+        colorAssignments.forEach((color) => {
+            const newColorData = this.colorHandler.setColorForAgentTypes(
+                color.agentIds,
+                color.color
+            );
+            this.renderer.updateColors(
+                newColorData.numberOfColors,
+                newColorData.colorArray
+            );
+        });


@meganrm pointing out the most significant change in response to @frasercl feedback since your approval.

Changed applyColorToAgents to receive array of arguments and back changes so there is only one call to updateScene when applying color changes.

interim17 added 9 commits May 23, 2024 09:27

add util for comparing new and old ui data to get color changes

e5bc443

take sessionUIData prop in viewport and add method to apply color cha…

65985e3

…nges from that data

throw error if ui data don't match

1b04d38

Merge branch 'main' of https://github.com/simularium/simularium-viewer …

f5935c6

…into feature/session-colors

don't throw errors when ui data doesn't match session data

130044b

call receiveSessionUIData in onTrajectoryFileInfo

687e4f3

use controller methods to handle color changes

40d9425

remove ColorChange from SelectionStateInfo

4ca2123

Merge branch 'main' of https://github.com/simularium/simularium-viewer …

5f27d77

…into feature/session-colors

interim17 added 14 commits July 8, 2024 09:14

initial consolidation of updateAgentColors and updateUiDataColor

fc3f96f

remove controller methods for applying color changes

f3e0816

go back to using SelectionStateInfo prop to receive and apply color s…

d015892

…ettings from front end

csonsolidate changeAgentsColor and handler, and remove named ColorSet…

627c414

…ting type from setInitialAgentColors

remove unused types, imports, and comments

dfb53d9

fix whitespace

6f9bfa8

remove unused agnet name from color update arguments

9678d19

rename getIdsAndColorsFromUIData

1bab4fe

remove duplicate length check

d6a6a76

rename colorSettings to appliedColors in SelectionStateInfo

a355255

revert updateUiDataColor to not update entries object

e3f8f82

rename argument for changeAgentsColor

cc52ba7

rename settings to colorAssignments

6cae2d5

use new UIDisplayData structure in applying color changes in test bed…

a158e4e

… viewer

interim17 commented Jul 19, 2024

View reviewed changes

interim17 added 3 commits July 19, 2024 09:30

consolidate helper function into changeAgentsColor

8e54623

declare ids before applying to agents for readability in changeAgents…

98662fb

…Color

typo fix

2337d40

interim17 marked this pull request as ready for review July 19, 2024 18:11

interim17 requested a review from a team as a code owner July 19, 2024 18:11

interim17 requested review from meganrm and frasercl and removed request for a team July 19, 2024 18:11

interim17 mentioned this pull request Jul 23, 2024

website patch for SelectionStateInfo changes simularium/simularium-website#545

Merged

meganrm approved these changes Aug 9, 2024

View reviewed changes

Merge branch 'main' of https://github.com/simularium/simularium-viewer …

a9b8fed

…into feature/session-colors

frasercl approved these changes Aug 13, 2024

View reviewed changes

interim17 added 3 commits August 13, 2024 13:06

use appliedColors in place of uiDisplayData in test bed state

311175c

process batches of assigments in when applying colors to agents to re…

f2de0a1

…duce calls of updateScene

overwrite subagent colors when color changes are applied to parent

80bb750

interim17 commented Aug 13, 2024

View reviewed changes

interim17 merged commit d3fa1a9 into main Aug 20, 2024
6 checks passed

interim17 deleted the feature/session-colors branch August 20, 2024 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/session colors #400

Feature/session colors #400

interim17 commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

Reduced coverage

interim17 Jul 19, 2024

meganrm Aug 9, 2024

interim17 Aug 12, 2024

interim17 Jul 19, 2024 •

edited

Loading

interim17 Jul 19, 2024

meganrm left a comment

interim17 commented Aug 13, 2024

frasercl left a comment

frasercl Aug 13, 2024

interim17 Aug 13, 2024

frasercl Aug 13, 2024

interim17 Aug 13, 2024

frasercl Aug 13, 2024

interim17 Aug 13, 2024

interim17 Aug 13, 2024

Feature/session colors #400

Feature/session colors #400

Conversation

interim17 commented Jun 28, 2024 • edited Loading

Time Estimate or Size

Problem

Solution

Type of change

github-actions bot commented Jun 28, 2024 • edited Loading

jest coverage report 🧪

Total coverage

Reduced coverage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interim17 Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meganrm left a comment

Choose a reason for hiding this comment

interim17 commented Aug 13, 2024

frasercl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interim17 commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

interim17 Jul 19, 2024 •

edited

Loading