-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add OpenGL Renderer #1075
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
Add OpenGL Renderer #1075
Conversation
It seems like most of the OpenGL mobjects are identical except for the parent class. Is it possible to detect which renderer is used, and dynamically switch the top level class (or wherever the classes differ)? |
@Darylgolden That would be doable, but it'd introduce more complexity to an already complex change, so I'd rather not. |
@naveen521kk In general your comments are looking for a level of polish that this PR isn't intended for. Changing the window title and logging with rich is fine, but documenting each of the mobjects is too much and making formatting changes without CI/automation won't last anyway. |
I think someone needs to do that right? Essentially, it should be fine at least having typing (as in what type of arguments is accepted either in the docstring or using typing module should be fine.), because the Users (including myself ) aren't familiar with though new Objects, and have no docs seems to be worse. Maybe it can be done later in another PR but it needs to be done before the release.
We haven't implemented that, but locally you can use isort for sorting import, at least for the files you created and for line ending something using pre-commit can be used, but sadly requires extra configuration though. Maybe if you want I can push them for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @naveen521kk for the review! Obviously, your comments should be addressed -- but I agree with @eulertour that this PR probably isn't the right place for this level of detail. I'd also rather have this merged before further work (refactoring classes, adding documentation, general polishing ...) is added to it; right now it is very difficult to keep track of changes because this PR is already so big.
I've just tested the latest version of this PR on MacOS, the example scenes now work as advertised, and the embed
feature is really great as well. I'd like to propose to merge this now as-is, and document open TODOs in new issues which we can collect in a new "OpenGL rendering" project board.
My two main concerns are documentation, and the fact that this duplicates lots of code. I don't know yet whether it is possible, but I would really like to have just one Mobject class for both Cairo and OpenGL rendering (and even if it is not possible / turns out to be a bad idea, we can certainly still find ways to reduce duplication).
I will at least do the formatting and the two comments about using rich and having a nice window title, before merging this PR. |
@eulertour I don't have permissions to push to this branch. Anyway, I attach the patch git am 0001-Use-rich-for-printing-exceptions.patch and then push Or simply merge https://github.com/naveen521kk/manim/tree/opengl-format git pull https://github.com/naveen521kk/manim opengl-format |
I think that should be a different task/PR. Just need to clean up the window situation (not hardcoding pyglet!). moderngl-window do have a window backend for headless. If the window type was configurable this is easy enough to sort out. |
points, du_points, dv_points = uv_surface.get_surface_points_and_nudged_points() | ||
normals = uv_surface.get_unit_normals() | ||
nudge = 1e-2 | ||
nudged_points = points + nudge * normals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ignores the normal_nudge variable which is set in the init and sets it to a constant value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some may call it a bug, I call it faithful to the original. Anyway I fixed it.
jupyterlab = { version = "^3.0", optional = true } | ||
moderngl = "^5.6.3" | ||
moderngl-window = "^2.3.0" | ||
mapbox-earcut = "^0.12.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please bump manimpango
to latest version 0.2.4
, because it will cause SegFaults or will not work with moderngl with wheels in place.
REF:
.github/workflows/ci.yml
Outdated
sudo apt update | ||
sudo apt install -y ffmpeg | ||
sudo apt-get -y install texlive texlive-latex-extra texlive-fonts-extra texlive-latex-recommended texlive-science texlive-fonts-extra tipa | ||
sudo apt-get -y install texlive texlive-latex-extra texlive-fonts-extra texlive-latex-recommended texlive-science texlive-fonts-extra tipa freeglut3-dev python-opengl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sudo apt-get -y install texlive texlive-latex-extra texlive-fonts-extra texlive-latex-recommended texlive-science texlive-fonts-extra tipa freeglut3-dev python-opengl | |
sudo apt-get -y install libpango1.0-dev texlive texlive-latex-extra texlive-fonts-extra texlive-latex-recommended texlive-science texlive-fonts-extra tipa freeglut3-dev python-opengl |
None of the text mobjects are ported in this PR so there's no need to worry about pango. I added it to the CI anyway since there were some dependencies I only added for testing. The way the flags work is detailed in the commit description. Using |
I expected to render the video and then open in the media player, anyway that can be fixed later :).
Locally run
I wasn't following this initially. I expected just running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm,
the flags had some problems but that can fixed in a later PR.
The
Pango continues to cause bugs despite not being central to the library's use and even when it isn't used. Code like this should be delegated to a plugin, even if it's still included in the pip package.
This is mentioned in the commit description, it's intentional. |
I would like to have an option to see the video after rendering, and we were using
👍
Wheels were removed in the latest release, so that shouldn't be a problem, that's why I asked you to have it as the minimum version. Also, there were no reports of installation failure or any other bugs, that caused them to not able to use Manim, other than that of SegFaults in Manimgl, see ManimCommunity/ManimPango#27. Also, does it bothers you anywhere else? Also, Manim should have at least one Text rendering either latex or pango's, and we are encouraging in our docs that |
I'll start a new issue about it. |
Motivation
Allows rendering with OpenGL
Overview / Explanation for Changes
Adds an OpenGLRenderer, OpenGLCamera, OpenGL-enabled Mobjects, and a
--use_opengl_renderer
flag. When this flag is passed, you can pass the-p
flag to preview animations, the-w
flag to generate video, and the-q
flag to specify render quality. If you don't pass either the-p
or the-w
flag, nothing will happen. Scenes rendered with the OpenGL renderer must only use OpenGL-enabled Mobjects.Oneline Summary of Changes
Testing Status
None :/
Further Comments
Temp files are currently written to media/temp_x.mp4. This should be changed, but I wanted to focus on getting the rendering done properly. Latex and text also aren't implemented.
Acknowledgements
Reviewer Checklist
SurfaceExample.mp4