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

Graphics Buffer Update -- New PR #721

Merged
merged 19 commits into from
Nov 15, 2021
Merged

Conversation

rburema
Copy link
Member

@rburema rburema commented Aug 9, 2021

The HD of my private laptop is fried. While I resolve that, it's easier to work on the laptop from work. Since switching accounts back and forth can be a big pain and lead to confusion, I'm developing these from my work-account.

I now return you the text of the original PR:

The highly interdependent Issues this fixes:

The Vertex Array Object (VAO) for a render wasn't actually being (re)used, but regenerated each time. Now it's cached.
While (otherwise) vertex- and index-buffers where already cached for normal rendering, the index-buffer for a mesh was recreated each time only a part of the mesh (a range) needed to be drawn (for example, Layer and Simulation view in Cura). (NB: This was due to a workaround on our side, caused by an only partial implementation of a specific bit of the OpenGL API by the Python interface we're using.) This is now fixed.
Complications:

In order to show 'ranged'/partial meshes, implementing programs (like Cura) must now have shaders that take a draw-range uniform, which can be compared against the newly introduced 'vertex-index' attribute. (As we can't use the OpenGL API due to partial implementation, and we can't use the gl_primitiveID built-in due to support of legacy shaders.) See also Graphic Buffers Update original: Ultimaker/Cura#10188 new: Ultimaker/Cura#10259, the front-end implementation of this PR.
See also this related discussion #664 earlier.

bremco and others added 11 commits March 7, 2021 11:51
I'm not sure why we're using VAO's when so much seems to be recreated each render, but this way at least the VAO itself wouldn't also be recreated even if the objects aren't.
Still TODO: Once the RenderBatch isn't recreated each time a slightly different render is made, VertexArrayObjects will actually be used, instead of just pretended to be used.
Also still TODO: any model will render as if the base color is matte dark grey. All the other rendering (like for 'lay-flat by face' or the blue model outline or the tool-arrows and such) and all the rendering _given that the cube is matte grey_ (like 'the error polka dot pattern' and the 'below the buildplate inversion') will still work properly. So everything works _except_ for the color and light-highlights. Why?
VAO includes shader uniforms, which where needed. So now, instead of cacheing per mesh alone, cache per mesh _per shader_. Currently the doesn't work 100% yet if different parts of a mesh are rendered with different settings, such as in the simulation view. Also need to check if the X-Ray view was supposed to be opaque or if that's another thing that doesn't work 100% yet (is usable though just not as pretty as remembered).
Instead of re-uploading the mesh each time the range changes, handle the range in the shaders with the new draw-range parameters.
Typing edition.

Co-authored-by: Jaime van Kessel <[email protected]>
Seems to have fixed the test as well. Not sure if it was ever a valid test if it can't create a VAO there.
Fixes type laziness, types should work harder!
@rburema
Copy link
Member Author

rburema commented Sep 16, 2021

So I finally properly ran the profile that @nallath suggested.

For a small case the answer is straightforward:

Current main branch is a bit slower:
old_code_small_example

Code in PR (I've merged main into it, will commit soon), is a bit faster:
new_code_small_example

For a larger example, I noticed something a bit strange; the profile shows a huge win for this PR, but ... the only difference I noticed as a user is that when in preview mode (where the speedup should be most noticeable, if at all), and I only selected one, or a few, layers, the 'new way' was still chugging a bit, when the 'old way' ran smoothly. When the entire model was shown on screen there wasn't much difference, I think.

This can partly be explained in that the new way of doing things is slightly more reliant on the graphics card rather than the normal compute cores, and that the whole model is loaded at all times, just not all parts are shown (early out in shader when a part shouldn't be shown of course). But it needs to be able to render the entire model anyway, so I don't consider this a huge loss.

Anyway, here is the chart for the current main:
old_code_big_example

... and for the apparently +-10x as fast new code:
new_code_big_example

Copy link
Member

@nallath nallath left a comment

Choose a reason for hiding this comment

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

I've got some tiny nitpicks, but it looks good otherwise.

UM/Mesh/MeshData.py Outdated Show resolved Hide resolved
UM/Mesh/MeshData.py Outdated Show resolved Hide resolved
rburema and others added 3 commits October 27, 2021 18:52
That's what it's for.

Contributes to issue CURA-8657.

Co-authored-by: Jaime van Kessel <[email protected]>
Contributes to issue CURA-8657.

Co-authored-by: Jaime van Kessel <[email protected]>
UM/View/RenderBatch.py Outdated Show resolved Hide resolved
UM/View/RenderBatch.py Outdated Show resolved Hide resolved
UM/View/RenderBatch.py Outdated Show resolved Hide resolved
UM/View/RenderBatch.py Outdated Show resolved Hide resolved
rburema and others added 3 commits October 31, 2021 19:00
Spelling

Co-authored-by: Ghostkeeper <[email protected]>
(Our own) bindings aren't used like this, there's an 'updateBindings' for that, setUniformValue sets the 'unbound' (at least w.r.t. our _own_ system, not to be confused by the fact that u_drawRange is of course a bound value w.r.t. the 'raw' OpenGL/shader) value.
Not that it's likely to happen.
@rburema rburema dismissed Ghostkeeper’s stale review October 31, 2021 20:29

Requested changes made.

@Ghostkeeper
Copy link
Collaborator

This PR is on hold for the moment since the companion PR in the Cura repository has encountered a bug while testing.

@Ghostkeeper Ghostkeeper merged commit 4869329 into master Nov 15, 2021
@Ghostkeeper Ghostkeeper deleted the bremco-graphics_buffer_update branch November 15, 2021 14:25
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.

4 participants