-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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/color change bug #346
Fix/color change bug #346
Changes from 11 commits
cbb77a9
4b99f1e
4e99a3c
cc272ad
21e56cc
823eb5f
cb49395
81fe04f
5929e0b
cfa6c50
067cbdf
c715abe
d059892
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
/* 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 | ||
*/ | ||
|
||
import VisGeometry from "../visGeometry"; | ||
import jsLogger from "js-logger"; | ||
import { convertColorStringToNumber } from "../visGeometry/color-utils"; | ||
import { Color } from "three"; | ||
|
||
const initialColorData = ["#000000", "#000001", "#000002", "#000003"]; | ||
const visGeometry = new VisGeometry(jsLogger.DEBUG); | ||
|
||
describe("VisGeometry", () => { | ||
describe("agent color management", () => { | ||
beforeAll(() => { | ||
visGeometry.createMaterials(initialColorData); | ||
}); | ||
describe("convertDataColorIndexToId", () => { | ||
test("it returns the index into the shorter, color as string, array", () => { | ||
expect((visGeometry as any).convertDataColorIndexToId(0)).toBe( | ||
0 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(4)).toBe( | ||
1 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(8)).toBe( | ||
2 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(12)).toBe( | ||
3 | ||
); | ||
}); | ||
test("if the index is outside the the length of colorsData, it loops to the beginning", () => { | ||
expect((visGeometry as any).convertDataColorIndexToId(16)).toBe( | ||
0 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(20)).toBe( | ||
1 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(24)).toBe( | ||
2 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(28)).toBe( | ||
3 | ||
); | ||
}); | ||
test("if the colorDataIndex is not divisible by 4, returns -1", () => { | ||
expect((visGeometry as any).convertDataColorIndexToId(1)).toBe( | ||
-1 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(2)).toBe( | ||
-1 | ||
); | ||
expect((visGeometry as any).convertDataColorIndexToId(3)).toBe( | ||
-1 | ||
); | ||
}); | ||
}); | ||
describe("getColorDataIndex", () => { | ||
test("it returns the index into the colorData array of an existing color", () => { | ||
for (let i = 0; i < initialColorData.length; i++) { | ||
const colorNumber = convertColorStringToNumber( | ||
initialColorData[i] | ||
); | ||
const newColor = [ | ||
((colorNumber & 0x00ff0000) >> 16) / 255.0, | ||
((colorNumber & 0x0000ff00) >> 8) / 255.0, | ||
((colorNumber & 0x000000ff) >> 0) / 255.0, | ||
1.0, | ||
]; | ||
expect( | ||
(visGeometry as any).getColorDataIndex(newColor) | ||
).toBe(i * 4); | ||
} | ||
}); | ||
test("it returns -1 if the color is not found", () => { | ||
const colorNumber = convertColorStringToNumber("#000004"); | ||
const newColor = [ | ||
((colorNumber & 0x00ff0000) >> 16) / 255.0, | ||
((colorNumber & 0x0000ff00) >> 8) / 255.0, | ||
((colorNumber & 0x000000ff) >> 0) / 255.0, | ||
1.0, | ||
]; | ||
expect((visGeometry as any).getColorDataIndex(newColor)).toBe( | ||
-1 | ||
); | ||
}); | ||
}); | ||
test("createMaterials sets colorsData with 4 values for each color", () => { | ||
expect((visGeometry as any).colorsData).toHaveLength( | ||
initialColorData.length * 4 | ||
); | ||
expect((visGeometry as any).numberOfColors).toBe( | ||
initialColorData.length | ||
); | ||
}); | ||
describe("getColorForColorId", () => { | ||
test("it returns the color as a string for a valid colorId", () => { | ||
for (let i = 0; i < initialColorData.length; i++) { | ||
const expectedColor = new Color(initialColorData[i]); | ||
const actualColor = visGeometry.getColorForColorId(i); | ||
expect(actualColor.r).toBeCloseTo(expectedColor.r, 0.0001); | ||
expect(actualColor.g).toBeCloseTo(expectedColor.g, 0.0001); | ||
expect(actualColor.b).toBeCloseTo(expectedColor.b, 0.0001); | ||
} | ||
}); | ||
test("it returns the first color for an invalid colorId", () => { | ||
const actualColor = visGeometry.getColorForColorId(-1); | ||
|
||
expect(actualColor).toEqual(visGeometry.getColorForColorId(0)); | ||
}); | ||
test("it loops around if the id is out of range", () => { | ||
expect(visGeometry.getColorForColorId(4)).toEqual( | ||
visGeometry.getColorForColorId(0) | ||
); | ||
expect(visGeometry.getColorForColorId(5)).toEqual( | ||
visGeometry.getColorForColorId(1) | ||
); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ import { TrajectoryFileInfoAny } from "../simularium/types"; | |
import { updateTrajectoryFileInfoFormat } from "../simularium/versionHandlers"; | ||
import { FrontEndError, ErrorLevel } from "../simularium/FrontEndError"; | ||
import { RenderStyle, VisGeometry, NO_AGENT } from "../visGeometry"; | ||
import { ColorChanges } from "../simularium/SelectionInterface"; | ||
import { ColorChanges, SelectionEntry } from "../simularium/SelectionInterface"; | ||
|
||
export type PropColor = string | number | [number, number, number]; | ||
|
||
|
@@ -53,6 +53,14 @@ const defaultProps = { | |
hideAllAgents: false, | ||
showPaths: true, | ||
showBounds: true, | ||
selectionStateInfo: { | ||
colorChanges: [ | ||
{ | ||
agents: [] as SelectionEntry[], | ||
color: "", | ||
}, | ||
], | ||
}, | ||
agentColors: [ | ||
0x6ac1e5, 0xff2200, 0xee7967, 0xff6600, 0xd94d49, 0xffaa00, 0xffcc00, | ||
0x00ccff, 0x00aaff, 0x8048f3, 0x07f4ec, 0x79bd8f, 0x8800ff, 0xaa00ff, | ||
|
@@ -321,6 +329,10 @@ class Viewport extends React.Component< | |
!isEqual( | ||
selectionStateInfo.colorChanges, | ||
prevProps.selectionStateInfo.colorChanges | ||
) && | ||
!isEqual( | ||
selectionStateInfo.colorChanges, | ||
defaultProps.selectionStateInfo.colorChanges | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was the change that fixed the bug There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
) | ||
) { | ||
this.changeAgentsColor(selectionStateInfo.colorChanges); | ||
|
@@ -561,19 +573,21 @@ class Viewport extends React.Component< | |
} | ||
|
||
public changeAgentsColor(colorChanges: ColorChanges[]): void { | ||
const { | ||
onUIDisplayDataChanged, | ||
} = this.props; | ||
const { onUIDisplayDataChanged } = this.props; | ||
colorChanges.forEach((colorChange) => { | ||
const { agents, color } = colorChange; | ||
const uiDisplayData = this.selectionInterface.getUIDisplayData(); | ||
const agentIds = | ||
this.selectionInterface.getAgentIdsByNamesAndTags(agents); | ||
|
||
this.selectionInterface.updateAgentColors(agentIds, colorChange, uiDisplayData); | ||
this.selectionInterface.updateAgentColors( | ||
agentIds, | ||
colorChange, | ||
uiDisplayData | ||
); | ||
const colorId = this.getColorId(color); | ||
this.visGeometry.applyColorToAgents(agentIds, colorId); | ||
const updatedUiDisplayData = this.selectionInterface.getUIDisplayData(); | ||
const updatedUiDisplayData = | ||
this.selectionInterface.getUIDisplayData(); | ||
onUIDisplayDataChanged(updatedUiDisplayData); | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know you could do this! Is this the preferred way (in our team) to do testing of private members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a stack overflow suggestion, I'm not sure if we have a preferred way of testing private methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel like it. I am not worried about it for this code.