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

Valis 8 prevent collision btw annot txts #39

Merged
merged 15 commits into from
Feb 4, 2022

Conversation

leepc12
Copy link

@leepc12 leepc12 commented Dec 14, 2021

  • Prevent collision between annotation names
  • Increase the threshold of LoD (level of detail) for blending of annotation names to prevent fading-out too fast at higher LoD (zoomed-out).

Fixes:

@leepc12 leepc12 requested a review from emro December 15, 2021 21:11
Copy link

@emro emro left a comment

Choose a reason for hiding this comment

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

Overall looks awesome! Most of my comments have to do with style conventions. Do you have a JS linting program installed? It can be really helpful.

The collision detection looks like it works really well. When I looked at your demo yesterday I saw a couple of overlaps but when I looked at it today I didn't see any.

One thing I was thinking could be another good change would be to not have the labels faded at the first zoom level at which they appear. They could just be black and then they'd be a lot more readable. I'm not sure what the use of having them faded is since they're not overlapping.

src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
Copy link

@zoldello zoldello left a comment

Choose a reason for hiding this comment

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

@leepc12 I have a comment marked as - Let's talk about. Let's talk about it. While I think the solution is clever, it does not solve the problem. I want to check with you to see if I am wrong. The other comments are minor Javascript rules.

src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
src/track/annotation/AnnotationTrack.tsx Show resolved Hide resolved
Copy link

@emro emro left a comment

Choose a reason for hiding this comment

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

One minor typo that I'd rather fix.

Also, can we make a release with this update?

src/track/annotation/AnnotationTrack.tsx Outdated Show resolved Hide resolved
@emro emro self-requested a review January 14, 2022 01:53
@leepc12 leepc12 requested a review from emro January 28, 2022 17:33
Copy link

@emro emro left a comment

Choose a reason for hiding this comment

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

Looks good

@emro emro merged commit 2d77999 into dev Feb 4, 2022
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.

3 participants