Closed gpucce closed 2 years ago
I know it's only a draft sorry for my short review. Do we need the
embed
of Liveview? I mean either you want to embed it or you want to open it with the liveviewer, right?
So you mean I should use a different function smth like JavisNB.liveview
complementary to JavisNB.embed
or should I keep the liveview entirely inside Javis
? I would like to move it to JavisNB
so it all works the same
I'm unsure whether we want an extra repository for Liveviewer as it's not actually a notebook so I wouldn't put it in this repo. It uses Gtk however and can cause problems on a headless machine so I would go for JavisViewer.jl
There I would call the render
function liveview.
In JavisNB
I like the embed(render
as you suggested.
I'm unsure whether we want an extra repository for Liveviewer as it's not actually a notebook so I wouldn't put it in this repo. It uses Gtk however and can cause problems on a headless machine so I would go for
JavisViewer.jl
There I would call therender
function liveview. InJavisNB
I like theembed(render
as you suggested.
Perhaps I later understood what you mean, the thing is that right now setting liveview=true
has different functionalities inside or outside notebooks. Inside a notebook it makes render
output the gif as a vector of images, in the REPL it opens the liveviewer
Yeah that's true. I think I would change that behavior as it's not that intuitive for me. Maybe the embed function itself can have a keyword which outputs it as array. Not sure whether that's feasible codewise though.
Just from the name of functions the best I can think of is the following:
render
renders the files to gif or MP4
embed
shows the result of a render in the notebook
filmstrip
or something similar shows the array of frames in the notebook. We could have a function embed
which takes a filmstrip as an argument.
What do you think @gpucce @TheCedarPrince
I like the naming scheme!
As you said I also don't have many ideas to move the liveview/filmstrip
kwarg from render
to embed
without making too many changes to Javis
.
One way could be to make embed
take all render
arguments and then add to it the filmstrip
argument that is then passed to render
through liveview
this way we would also not have to write embed(render(...))
though it may be a little confusing having two functions that are nearly identical.
Hey @gpucce !
Haha, I saw you open this issue and am super thrilled! Thanks for working on this with us! Didn't want to comment yet until you felt ready to convert this from a draft to a review PR.
I will say that I really like not having to write embed(render(...))
.
That syntax does feel a bit clunky to me personally so I am very happy with the direction this PR is going.
To @Wikunia 's points, I really like the naming convention!
Keeping render
and limiting it to just generating files is great.
filmstrip
is a very interesting idea as I think this would be better suited to actually be housed inside of Javis.jl itself as opposed to JavisNB
.
My rationale for why I think it should be in Javis.jl is that I could see this providing a useful functionality for users who may want to do further image processing with a given animation before it either gets rendered or embedded somewhere - it enables a sort of post-processing if you will.
As a result, it makes sense to have embed
not only ingest a FilmStrip
type but to also have render
ingest a FilmStrip
type.
What I imagine the FilmStrip
type could look like is something like this:
struct FilmStrip
film::Array,
framerate::Real,
end
I like the syntax of embed
as well being available within notebooks.
As a final thought, I think it is also necessary at this time to move the Javis Viewer out of Javis (with its associated dependencies) as I think worrying about how to best support liveview
may be distracting from what we are trying to solve in this PR.
I would say it doesn't matter if we break liveview
in the process of these PRs as I think it best to move Javis Viewer into its own package JavisViewer.jl
as @Wikunia suggested.
Just some thoughts off hand! Great work so far @gpucce !
Hi @TheCedarPrince thanks for the review and no worries about commenting.
Just to make sure, you say you would like the FilmStrip
struct to be part of Javis instead of JavisNB right?
Also just in case you did not see them there are the postprocess_frame
and postprocess_frames_flow
arguments for render
that might allow to do the postprocessing you mentioned.
For now I will move one with making embed
a fully capable render
for notebooks and test it.
That is correct @gpucce - what do you think @Wikunia ?
Would FilmStrip
make a nice addition?
Also, I completely forgot about the postprocess
functions.
But that sounds like a good plan of action! Feel free to ask more questions!
Yes Filmstrip should be part of Javis and having a render function for a filmstrip would be cool to change the frame rate later on as an example or create several film strips using the filmstrip function and combine them to a single video with the render function. That would have made my life easier when creating my YouTube video.
Which means I wouldn't store the framerate in the filmstrip itself by the way.
Ok nice, though I guess that will go in a different PR, so for now embed
will be the notebook equivalent of render
once Javis
has FilmStrip
I will add support in JavisNB
I think the main things are there as we said, I would like to add a couple tests but it is impossible (I can't manage) to test having a specific branch as a dependency so I can't use the Javis branch where I am doing the changes to work with JavisNB. Thus, for those tests I believe I have to wait until that is merged in Javis.
Let me know what you think @Wikunia @TheCedarPrince
Thanks a lot. You can locally test it though by calling dev ../Javis.jl
for example and then checkout the right branch there. Not sure about on GitHub actions though.
The thing is that I would need to do that in both JavisNB and the test environment and if I did that it throws this strange error that says 'can not merge projects' Googling it it looks like it is not possible. Don't know if you ever had the same issue, however I can try in the next days.
Hmm that's weird thought it would be simple but I can also check during the weekend
Hey @gpucce ,
I just tried this out in a Pluto notebook and am getting this strange error:
Any idea as to why this is happening?
EDIT: I can confirm, this error appears to only occur within Pluto notebooks as it works fine within Jupyter notebooks:
Hi @TheCedarPrince yeah the error is there because you should use Javis as it is in the PR to move the notebook functionality to JavisNB,
https://github.com/JuliaAnimators/Javis.jl/pull/451
Sorry I forgot to mention this.
I mean I think that is why, if you are already doing it then I have to check!
Hi @TheCedarPrince I did the changes you suggested and went on and added also the Documenter support and the proper documentation let me know what you think and if it works for you!
Hi @TheCedarPrince grammar changes should be done. I can't merge it myself so waiting for you on that.
For the grammar fixes in general if you tell me I use it to refresh english a bit but if you want to make them and go on it is totally fine!!
Hi @TheCedarPrince
I changed the docs adding more info, for sure there will be more grammar checking to do sorry for my english :)
But I think it works better now I took some more of the things that were there in the Javis workflows documentation and added them here.
Oh and I also uncommented the CI docs workflow.
PR Checklist
If you are contributing to
JavisNB.jl
, please make sure you are able to check off each item on this list:CHANGELOG.md
with whatever changes/features I added with this PR?JavisNB
dependencies to theProject.toml
+ set an upper bound of the dependency (if applicable)?test
directory (if applicable)?Link to relevant issue(s)
Closes #1, #2, #3
How did you address these issues with this PR? What methods did you use? I started moving the functionality to render the videos within the notebooks.
The idea is to use the
embed
function that, if called in place of theJavis.render
one will replicate the current functionalities ofJavis.render
within a notebook. That means, what used to benow becomes
At the same time I created a branch in my fork called
gp-move-viewers-to-separate-package
where I started changingJavis.render
to accomodate for this, in particularJavis.render
only returns the pathname without calling the viewer functions even if called within a notebook, same goes for the case thatliveview=true
.