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.

Sign up for GitHub

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

Jump to bottom

[WIP] Update highlighted points when time changes #96

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

[WIP] Update highlighted points when time changes #96

wants to merge 15 commits into from
+34 −4

Conversation

andy-sweet
Copy link
Collaborator

@andy-sweet andy-sweet commented Jun 5, 2024 •
edited
Loading

Closes #53
Closes #80

This updates the highlighted points when the time point changes. It also distinguishes between the number of loaded tracks and highlighted points at the current time point, presenting those to the user.

It does this by storing the point IDs in the tracks with the start and end time (which are all readily available), then using that to convert point IDs to time-specific indices.

This is probably the smallest change that gives us the behavior described in #53. However, this does against some of the ideas in #53 and #68. I agree that mixing ID conversion logic and geometry information is undesirable, so very open to other approaches here.

The approach here also has performance issues when a large number of tracks have been loaded because the conversion takes a little time.

zebrahub-update-selected-points.mp4

@vercel Vercel
Copy link

vercel bot commented Jun 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
points-web-viewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2024 8:40pm

@andy-sweet
Copy link
Collaborator Author

I don't think this reviewable/mergable yet. The behavior might be good enough, but there are some performance issues. I'd also want to wait to merge #95 before marking this ready for review.

@aganders3
Copy link
Collaborator

aganders3 commented Jun 6, 2024 •
edited
Loading

This is probably the smallest change that gives us the behavior described in #53. However, this does against some of the ideas in #53 and #68. I agree that mixing ID conversion logic and geometry information is undesirable, so very open to other approaches here.

My idea here would be to instead store a(nother) map on PointCanvas that maps curTime -> pointIndices (number -> Set<number>). This could be filled in during canvas.addTrack. Memory usage should be similar but it should be a bit faster to update, as there'd be no calculations and no loop over tracks when changing the timepoint.

I'm also trying to think about doing it in the shader (create a new PointsMaterial to support highlights), but I think it would be more complicated, less general-purpose, and it might not even be faster in the end.

(apologies - I know you said this is not ready for review)

@aganders3 aganders3 mentioned this pull request Sep 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@andy-sweet @aganders3