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

More Makie integration #6

Merged
merged 29 commits into from
May 29, 2024
Merged

More Makie integration #6

merged 29 commits into from
May 29, 2024

Conversation

asinghvi17
Copy link
Collaborator

  • Adds a plotcamerapath recipe which has a colored line (representing time) and an arrow (representing the camera).
    • TODO: add viewing frustum
  • Runs the documentation build under a virtual framebuffer (xvfb) so we can use GLMakie on CI
  • Fixes the Makie camera integration so that everything is updated correctly
  • Adds a demo using rotation and zoom in GLMakie
  • Defines duration(path) for paths as well

@asinghvi17 asinghvi17 requested a review from timholy May 20, 2024 04:44
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

This looks great. Other than reducing the duplication between the README and Documenter docs (which I think is important), the rest of this I'd leave up to your excellent judgement!

src/path.jl Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
ext/FlyThroughPathsMakieExt.jl Outdated Show resolved Hide resolved
arrows!(
plot,
@lift([$eyeposition_obs]),
@lift([$viewdir_obs]);
Copy link
Member

Choose a reason for hiding this comment

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

If we go with your frustum idea, should we instead use an arrow for upvector? With a polar rotation I can imagine some confusion arising from a failure to change this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two arrows might get confusing if we don't have the frustum active, and it's currently pretty finicky - you need to have a second Scene active with the camera being set, otherwise the data source doesn't exist. It's possible to compute the frustum manually but it would be pretty annoying.

Instead of that, we could use a 3D camera mesh as an arrow marker - that would show directionality as well. Currently the arrow doesn't indicate upvector but presumably this would if it's sufficiently large.

Copy link
Member

Choose a reason for hiding this comment

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

Good points. I don't have a clear idea about how to do this, so let's just do whatever seems reasonable.

docs/src/makie.md Show resolved Hide resolved
oldstate = set_view!(camera, path, t)
```

This updates the current `camera` settings from `path` at time `t`.

### Displaying the path

```
```julia
Copy link
Member

Choose a reason for hiding this comment

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

With the move to Documenter docs, I think we could also slim the README a lot. Otherwise we have two places we have to keep updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can definitely slim the README down (and translate the examples to Documenter syntax)!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Example translation should be done, but I still have to slim down the README.

Co-authored-by: Tim Holy <tim.holy@gmail.com>
Copy link

codecov bot commented May 24, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@timholy
Copy link
Member

timholy commented May 24, 2024

This looks awesome, thanks for sticking with it!

@timholy
Copy link
Member

timholy commented May 24, 2024

Any idea what causes xvfb-run: error: Xvfb failed to start?

@asinghvi17
Copy link
Collaborator Author

Nope, will look into it though. I based this on GeoMakie, which works, so will have to make sure nothing is different...

@timholy
Copy link
Member

timholy commented May 26, 2024

Awesome, great to see it fixed. If it's just slimming the README, I can help with that, so feel free to merge whenever you're comfortable.

@asinghvi17
Copy link
Collaborator Author

asinghvi17 commented May 27, 2024

Thanks! I've got a couple more changes I wanted to make, then should be good to go.

You can see the docs at https://holylab.github.io/FlyThroughPaths.jl/previews/PR6

@asinghvi17 asinghvi17 merged commit 86e6140 into main May 29, 2024
4 of 6 checks passed
@timholy timholy deleted the as/docs_with_glmakie branch May 30, 2024 15:23
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