feature: Rendering optimization for many labels #100
Conversation
| @@ -32,15 +51,4 @@ describe("Atom labels", () => { | |||
| wave.toggleLabels(); | |||
| expect(labels.every((label) => label.visible === !areLabelsInitiallyShown)).toBeTruthy(); | |||
| }); | |||
|
|
|||
| test("Label center is positioned on the atom surface", () => { | |||
There was a problem hiding this comment.
Seems like a good thing to test for - Label center is positioned on the atom surface - perhaps we should keep at least the explanation, if you are doing it in the above test
src/mixins/labels.js
Outdated
| * @returns {Object.<string, Array.<Three.InstancedMesh>>} atomsHashMap | ||
| */ | ||
| makeHashMapFromAtomNames() { | ||
| const atomsHashMap = {}; |
There was a problem hiding this comment.
This function might better fit in src/mixins/atoms.js
| createLabelSpritesAsPoints() { | ||
| if (this.#labels.length) { | ||
| this.#labels.forEach((label) => this.structureGroup.remove(label)); | ||
| } |
There was a problem hiding this comment.
Seems like should be isolated to this.removeLabels
src/mixins/labels.js
Outdated
| map: texture, | ||
| depthTest: true, | ||
| depthFunc: THREE.NotEqualDepth, | ||
| transparent: true, |
There was a problem hiding this comment.
Should we move the above config to settings?
There was a problem hiding this comment.
Do you mean PointsMaterial params?
src/mixins/labels.js
Outdated
| @@ -124,6 +190,8 @@ export const LabelsMixin = (superclass) => | |||
| */ | |||
| toggleLabels() { | |||
| this.areLabelsShown = !this.areLabelsShown; | |||
| this.#labels.forEach((label) => (label.visible = this.areLabelsShown)); | |||
| this.#labels.visible = this.areLabelsShown; | |||
There was a problem hiding this comment.
Our convention was to use is..., are... for booleans to make it clear - so maybe visible -> areVisible?
There was a problem hiding this comment.
Ohh... just found that the expression on line 194 is wrong, but the visible property is correct because it's a property of THREE.Object3d.
There was a problem hiding this comment.
tests/__tests__/mixins/labels.js
Outdated
| atoms = atomGroup.children.filter((object) => object.type === "Mesh"); | ||
| labels = atomGroup.children.filter((object) => object.type === "Sprite"); | ||
| }); | ||
|
|
||
| test("Labels are created for every atom", async () => { |
There was a problem hiding this comment.
Per https://github.com/Exabyte-io/wave.js/pull/100/files#r940467021 below - this should be called: "Labels are created for every atom and placed at their centers" probably
src/config.js
Outdated
| depthTest: true, | ||
| depthFunc: THREE.NotEqualDepth, | ||
| transparent: true, | ||
| }); |
There was a problem hiding this comment.
src/mixins/labels.js
Outdated
| } | ||
|
|
||
| /** | ||
| * this function is creates vertices hashMap where key is atom name, value is array of vertices where this atoms is situated |
There was a problem hiding this comment.
is creates -> creates
Why do we need this function? Let's explain in the docstring as well.
src/mixins/labels.js
Outdated
| @@ -12,6 +11,8 @@ export const LabelsMixin = (superclass) => | |||
| class extends superclass { | |||
| #texturesCache = {}; | |||
|
|
|||
| #labels = []; | |||
There was a problem hiding this comment.
I think we should create a labelsGroup that will contain the labels for all atoms in the corresponding order as the atoms themselves
src/enums.js
Outdated
| @@ -27,6 +27,7 @@ export const ATOM_CONNECTION_LINE_NAME = "Atom_Connection"; | |||
| export const MIN_ANGLE_POINTS_DISTANCE = 0.7; | |||
| export const MEASURE_LABELS_GROUP_NAME = "Measure_Labels"; | |||
| export const ANGLE = "ANGLE"; | |||
| export const LABELS_GROUP_NAME = "Labels Group"; | |||
There was a problem hiding this comment.
Probably safer without the space Labels_Group, no?
tests/__tests__/mixins/labels.js
Outdated
| ); | ||
| if (!labelPointsByAtomName) return false; | ||
| const positions = labelPointsByAtomName.geometry.getAttribute("position")?.array; | ||
| let isHaveSomeLabel = false; |
No description provided.