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.

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/sof 6814 #131

Merged
merged 21 commits into from
Aug 23, 2023
Merged

Feature/sof 6814 #131

merged 21 commits into from
Aug 23, 2023

Conversation

VsevolodX
Copy link
Contributor

Made elements labels to be 3D aware and occluded by atoms, removed useless code.

@VsevolodX VsevolodX marked this pull request as ready for review August 14, 2023 17:25
map: texture,
transparent: true,
depthFunc: THREE.LessEqualDepth,
depthTest: true,
Copy link
Member

Choose a reason for hiding this comment

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

Move to settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

src/settings.js Outdated
@@ -45,7 +45,7 @@ export default {
labelPointsConfig: {
size: 1.5,
depthTest: true,
depthFunc: THREE.NotEqualDepth,
depthFunc: THREE.LessEqualDepth,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to modify this??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For points it should be as it was before, I'll revert

src/wave.js Outdated
@@ -322,12 +323,13 @@ export class Wave extends mix(WaveBase).with(
this.drawBoundaries();
if (this.isDrawBondsEnabled) this.drawBonds();
this.render();
this.createLabelSpritesAsPoints();
if (this.fastLabelsRender) this.createLabelsAsPoints();
Copy link
Member

Choose a reason for hiding this comment

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

Let's have this logic instead a function createLabels and abstract it from here

}
});
this.structureGroup.add(this.labelsGroup);
this.render();
Copy link
Member

Choose a reason for hiding this comment

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

Per my comment below this.render() should go to createLabels function

src/wave.js Outdated
this.refillChosenAtoms();
}

render() {
this.adjustLabelsToCameraPosition(this.scene, this.camera);
if (this.settings.labelsConfig.areSpritesUsed) this.adjustLabelsToCameraPosition();
Copy link
Member

Choose a reason for hiding this comment

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

Move this to create labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Looked at it again. Adjustment should happen on render() after the camera moved, not when the labels are created.

Copy link
Member

Choose a reason for hiding this comment

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

Let's move the if statement to the function itself

@VsevolodX VsevolodX merged commit 9f8ef23 into dev Aug 23, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants