Fix/color change bug #346

meganrm · 2023-11-22T23:19:49Z

Problem

in working on the color picker @interim17 and @ShrimpCryptid noticed a bug with wrong color being applied

Solution

The fix was to check if the color changes coming into the app had been reset to defaults. But in debugging we found that the color handling was confusing. There are two different color arrays: an array where each item is a single color, and colorsData where each color is represented by 4 numbers, so it's 4 times as long as the actual number of colors. We identify colors by their index in the first array, but sometimes need to switch between the two different indexes, and in each case they were labeled something like colorIndex or just index. Here I renamed everything as either a colorId :index into the shorter array where each item is a color and colorDataIndex: index into the 4 timex longer array. Hopefully this will reduce future confusion.

So the majority of this changeset is documentation and tests.

with @interim17

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
This change requires a documentation update
This change requires updated or new tests

Steps to Verify:

npm start
select a model
change colors, add new colors, etc, should be stable.

meganrm · 2023-11-22T23:20:18Z

src/viewport/index.tsx

+                ) &&
+                !isEqual(
+                    selectionStateInfo.colorChanges,
+                    defaultProps.selectionStateInfo.colorChanges


this was the change that fixed the bug

I wonder if this function would be more performant if we updated to newer react and use useEffect. I always wonder if the number of checks here could bog things down.

it would be clearer to read, but I imagine at some level hooks are doing all these checks anyway to know if they should run. My understanding is class components are equally performant to function/hook based components in a one to one comparison. you can get increases in performance if you manage to make your overall tree structure less nested which is one of the benefits

github-actions · 2023-11-22T23:21:53Z

jest coverage report 🧪

Total coverage

Status	Category	Percentage	Covered / Total
🔴	Statements	45.86% (+5.72% 🔼)	2251/4908
🔴	Branches	44.75%	840/1877
🔴	Functions	43.48% (+6.26% 🔼)	474/1090
🔴	Lines	45.85% (+5.46% 🔼)	2143/4673

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

Show files with reduced coverage 🔻

Reduced coverage

Status	Filename	Statements	Branches	Functions	Lines
🔴	src/viewport	14.76%	12.12%	15.38%	14.28%
🔴	index.tsx	14.76%	12.12%	15.38%	14.28%

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

meganrm · 2023-11-22T23:30:04Z

src/visGeometry/index.ts

        const colorArray = this.colorsData;
        const colorToCheck = map(color, (num) => round(num, 6));
        for (let i = 0; i < colorArray.length - 3; i += 4) {
-            const index = i / 4;
+            const index = i;


this was also a bug

meganrm · 2023-11-22T23:31:28Z

src/visGeometry/index.ts

-        if (!this.visGeomMap.has(id)) {
-            this.visGeomMap.set(id, DEFAULT_MESH_NAME);
-        }
-    }


these are just moved up, I grouped all the private methods for handling color together, followed by all the public methods

meganrm · 2023-11-22T23:32:19Z

src/visGeometry/index.ts

        this.setColorForIds(agentIds, colorId);
        this.updateScene(this.currentSceneAgents);
    }

-    public getColorForIndex(index: number): Color {
+    public getColorForColorId(colorId: number): Color {
+        if (colorId < 0) {


I think these cases will be impossible to hit. I wrote them as I was thinking of edge cases writing tests.

toloudis · 2023-11-27T18:27:53Z

src/visGeometry/index.ts

+     * Agent color handling
+     * @property this.colorsData is an array of floats, each 4 floats is a color
+     * @param dataColorIndex is an index into the colorsData array, so it is a multiple of 4
+     * @param colorId is an index into the numberOfColors, so it is a number between 0


what is the context of these @params ? this comment seems like it's not attached to anything. Also it might be a nice improvement to shift all of the color functions out to their own module.

yeah this comment is for the whole section of "agent color handling". I do agree it should be its own module (in addition to a lot of other things in visGeometry). I was consistent in all the following functions when using these two different names for parameters instead of generic things like "index" or "colorIndex" that we had before. I thought it would be clearer here than having the same comments for every function that used one of these

It's the @param tags that are not helping. They look like jsdoc tags for autogenerated documentation and you can't tell what they are parameters for. I think it would be clearer to have each function get its own jsdoc if you want to leave the code here in the middle of this module.

src/visGeometry/index.ts

toloudis · 2023-11-27T21:59:22Z

src/visGeometry/index.ts

    }

    public applyColorToAgents(agentIds: number[], colorId: number): void {
+        /**
+         * Sets one color for a set of ids
+         */


jsdoc style comments should go above the function name and not below it.

toloudis · 2023-11-27T22:07:07Z

src/visGeometry/index.ts

+     * Agent color handling: private methods
+     */
+
+    private get numberOfColors(): number {


Nit: I feel this would be more readable when it is used later in the code, if it were a simple function. It always messes me up when I think it's a simple variable and then wonder, who is updating its value when we add a new color??

ShrimpCryptid · 2023-11-28T00:40:25Z

src/test/VisGeometry.test.ts

+/* eslint-disable @typescript-eslint/no-explicit-any */
+/* NOTE: this file is testing many private
+    methods and properties of VisGeometry.ts
+    which we do by using (visGeometry as any) 
+    so we need to disable the eslint rule here
+*/


I didn't know you could do this! Is this the preferred way (in our team) to do testing of private members?

This was just a stack overflow suggestion, I'm not sure if we have a preferred way of testing private methods.

I don't know if we have standardized on a way to do this across many projects. IMO this is very acceptable but a tradeoff:
(1) It's fair game to do this to simply unit test functions that are supposed to be private/internal. It lets you get at those functions without having to modify the actual code. Idk if typescript has other clean ways to do it (that would be more lintable).
(2) if you are trying to test the public api then this could just confuse things about what is a class's intended contract to the outside world

regarding #2: should I separate out a file that is only the private methods? I think there is only one public method I test here

If you feel like it. I am not worried about it for this code.

src/visGeometry/index.ts

ShrimpCryptid · 2023-11-28T01:06:16Z

src/visGeometry/index.ts

+        const colorDataIndex = this.getColorDataIndex(newColor);
+        if (colorDataIndex !== -1) {
+            // found the color, need to return the colorId to the
+            // external caller, with no other changes needed
+            return this.convertDataColorIndexToId(colorDataIndex);
        }
-
-        const newIndex = this.colorsData.length;
+        // the color isn't in colorsData, so add it and return the colorId
+        const newColorDataIndex = this.colorsData.length;
        const newArray = [...this.colorsData, ...newColor];
        const newColorData = new Float32Array(newArray.length);
        newColorData.set(newArray);
        this.colorsData = newColorData;
-        this.renderer.updateColors(this.colorsData.length / 4, this.colorsData);
-        return newIndex;
+        this.renderer.updateColors(this.numberOfColors, this.colorsData);
+        return this.convertDataColorIndexToId(newColorDataIndex);


Are we worried about performance issues here?

Like, if I keep setting different colors for agents and then changing them, all of the intermediate colors would get added to this.colorsData. I don't think the array itself would cause issues (since it would take up like, 67 MB max if ignoring alpha values), but searching through it to find color indices later might be slower since it could potentially have up to 16 million entries.

Edit: I take back the 67 MB max figure actually, since it looks like colors components are stored as floats and not as 1-byte integers. So, all combinations of RGB colors, each stored as a 4-byte float in arrays of length four, would be...

256^3 colors * 4 floats/color * 4 bytes/float = 134,217,728 bytes = 134.2 MB

yeah it's a good question. I don't think there is any real issue with just adding a new color every time we change color even if it already exists. My thinking was around keeping the data consistent ie, what if we wanted to select all agents that have the same color? But we don't currently have a use case for that. I could go either way on this

I would expect the array of color data that gets passed to the shader, to be the same length as the number of agent types...keeping that small and efficient is good for the shader that uses it. But it will only be "bad" in the case Peyton describes where people start putting a lot of unused colors in the array and it gets to like more than 2-3x the number of agent types and we get extra cache misses on lookups and just have dead data lying around for no reason.

ShrimpCryptid · 2023-11-28T01:07:40Z

src/visGeometry/index.ts

        return new Color(
-            this.colorsData[index * 4],
-            this.colorsData[index * 4 + 1],
-            this.colorsData[index * 4 + 2]
+            this.colorsData[colorId * 4],
+            this.colorsData[colorId * 4 + 1],
+            this.colorsData[colorId * 4 + 2]


Should this return the alpha channel, since we're already storing it anyways?

this Color from THREE js doesn't accept 4th term. but it is a question for when we're actually supporting transparency

Co-authored-by: Peyton Lee <[email protected]>

toloudis

The code changes look good.

There are still a bunch of jsdoc function comments in here which are below the function declaration, which is more python-style than js style and not what jsdoc recommends.

We need a new ticket to move this color handling code out to its own module.

…simularium-viewer into fix/color-change-bug

meganrm · 2023-11-28T19:11:30Z

The code changes look good.

There are still a bunch of jsdoc function comments in here which are below the function declaration, which is more python-style than js style and not what jsdoc recommends.

We need a new ticket to move this color handling code out to its own module.

I made one: #347

ShrimpCryptid · 2023-11-28T20:18:59Z

src/visGeometry/index.ts

    public setColorForIds(ids: number[], colorId: number): void {
        /**
         * Sets one color for a set of ids, using an index into a color array
         * @param ids agent ids that should all have the same color
-         * @param colorId index into the color array
+         * @param colorId index into the numberOfColors


Suggested change

* @param colorId index into the numberOfColors

* @param colorId index in terms of numberOfColors

ShrimpCryptid

Looks good! Thanks for the discussion on the points I brought up.

interim17 and others added 7 commits November 9, 2023 09:54

bug fix branch

cbb77a9

remove log

4b99f1e

successfully change colors

4e99a3c

remove log

cc272ad

fix typing

21e56cc

restore initial agent colors array

823eb5f

clean up, comments and tests

cb49395

meganrm requested a review from a team as a code owner November 22, 2023 23:19

meganrm requested review from toloudis, frasercl and ascibisz and removed request for a team November 22, 2023 23:19

meganrm commented Nov 22, 2023

View reviewed changes

more confident comment

81fe04f

meganrm commented Nov 22, 2023

View reviewed changes

meganrm requested a review from ShrimpCryptid November 22, 2023 23:33

toloudis reviewed Nov 27, 2023

View reviewed changes

src/visGeometry/index.ts Outdated Show resolved Hide resolved

toloudis reviewed Nov 27, 2023

View reviewed changes

edits to comments

5929e0b

ShrimpCryptid reviewed Nov 28, 2023

View reviewed changes

src/visGeometry/index.ts Outdated Show resolved Hide resolved

ShrimpCryptid reviewed Nov 28, 2023

View reviewed changes

src/visGeometry/index.ts Outdated Show resolved Hide resolved

ShrimpCryptid reviewed Nov 28, 2023

View reviewed changes

src/visGeometry/index.ts Outdated Show resolved Hide resolved

ShrimpCryptid reviewed Nov 28, 2023

View reviewed changes

meganrm and others added 3 commits November 28, 2023 10:30

Update src/visGeometry/index.ts

cfa6c50

Co-authored-by: Peyton Lee <[email protected]>

clarify comment

067cbdf

Update src/visGeometry/index.ts

c715abe

Co-authored-by: Peyton Lee <[email protected]>

toloudis approved these changes Nov 28, 2023

View reviewed changes

Merge branch 'fix/color-change-bug' of https://github.com/simularium/…

d059892

…simularium-viewer into fix/color-change-bug

ShrimpCryptid reviewed Nov 28, 2023

View reviewed changes

ShrimpCryptid approved these changes Nov 28, 2023

View reviewed changes

meganrm merged commit cf277d0 into main Nov 28, 2023
6 checks passed

meganrm deleted the fix/color-change-bug branch November 28, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/color change bug #346

Fix/color change bug #346

meganrm commented Nov 22, 2023 •

edited

Loading

meganrm Nov 22, 2023

toloudis Nov 27, 2023

meganrm Nov 27, 2023

github-actions bot commented Nov 22, 2023 •

edited

Loading

Reduced coverage

meganrm Nov 22, 2023

meganrm Nov 22, 2023

meganrm Nov 22, 2023

toloudis Nov 27, 2023

meganrm Nov 27, 2023

toloudis Nov 27, 2023

toloudis Nov 27, 2023

toloudis Nov 27, 2023

ShrimpCryptid Nov 28, 2023 •

edited

Loading

meganrm Nov 28, 2023

toloudis Nov 28, 2023

meganrm Nov 28, 2023

toloudis Nov 28, 2023

ShrimpCryptid Nov 28, 2023 •

edited

Loading

meganrm Nov 28, 2023

toloudis Nov 28, 2023 •

edited

Loading

ShrimpCryptid Nov 28, 2023

meganrm Nov 28, 2023

toloudis left a comment

meganrm commented Nov 28, 2023

ShrimpCryptid Nov 28, 2023

ShrimpCryptid left a comment

	* @param colorId index into the numberOfColors
	* @param colorId index in terms of numberOfColors

Fix/color change bug #346

Fix/color change bug #346

Conversation

meganrm commented Nov 22, 2023 • edited Loading

Problem

Solution

Type of change

Steps to Verify:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2023 • 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

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

ShrimpCryptid Nov 28, 2023 • edited Loading

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

ShrimpCryptid Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toloudis Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toloudis left a comment

Choose a reason for hiding this comment

meganrm commented Nov 28, 2023

Choose a reason for hiding this comment

ShrimpCryptid left a comment

Choose a reason for hiding this comment

meganrm commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

ShrimpCryptid Nov 28, 2023 •

edited

Loading

ShrimpCryptid Nov 28, 2023 •

edited

Loading

toloudis Nov 28, 2023 •

edited

Loading