-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 #10259
Conversation
Instead of re-uploading the mesh each time the range changes, handle the range in the shaders with the new draw-range parameters. This does however, mean the range has to be in vertices, not in elements. This necessitates some changes to the simulation-view, and some added bits deeper in the base code. Mainly, since each time the type of a line changes, there is an extra vertex, there needs to be a type-change count available to get from 'paths' to range indices.
… into bremco-graphics_buffer_update
Polygons don't change when in layer-view. There's already an analogous elementCount property anyway, so a vertexCount property can't do much harm. Just keep in mind that when the polygons are altered, it should be either done via build, or the lineMeshXXXCount methods should be used instead.
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.
A few minor things, but also a major question:
I don't really understand the need for this type change count you're adding. It seems like it's added to the index where the current layer ends? But why does that matter?
@Ghostkeeper Thanks for taking this up btw!
So, in the shader(s), since properties are either set globally or on a per-vertex basis*, we duplicate one vertex whenever we switch line-type, because otherwise you'd get a line where the two vertices have different types (think a vertex with the extrusion line type on one end and a vertex with travel on the other end). However, as this is a workaround for a reality in rendering** (and at least somewhat specific to SimulationView), anywhere else it's just the vertices or line-segments you expect. In order to start with the proper line segment in the layer-view, we need to take the number of type-changes into account. Possibly, this could also have happened within the ranges itself, (There are .... ways around that, but I don't think it's pleasant, possibly less performant, and would require rewriting (not just) the shaders.) |
Also, I can't really reproduce the stutter here. That must've been something unique to my old Linux laptop then? |
Co-authored-by: Ghostkeeper <[email protected]>
Requested changes made, questions answered (well, I hope).
Requested documentation added! :-) |
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 for the fixes. Looks good now.
The documentation is good to add to the actual code, rather than just in this PR. In these comments it will get lost in time. Or lost in SaaS, whichever is first to go. So thanks :)
Loaded up a big 20cm cube with layer height 0.06mm and 100% density gyroid infill. This PR changed the frame rate from 1.3fps to 5.8fps. It did appear to vary a lot more than it did before. Some frames still took about a second to render (though I didn't measure this) but most frames are a lot faster, and the slow frames now are comparable to the normal frames before. Other tests I performed:
So I found 1 bug. This is how Compatibility Mode for layer view used to look. The top 5 layers are rendered with full line width, the rest are rendered with thin lines only: However this is how Compatibility Mode for layer view now looks. The lines-rendering is not stopped at the layer slider: This will need fixing before this change can be merged. It works with any model in layer view once compatibility mode is enabled in the preferences and Cura is restarted. |
These shader (files) are now only used for compatability mode. The 'modern' code is found in the '...3d.shader' versions of these files.
Should be fixed now! I thought I had already done this, but apparently not. I almost added the bottom slider into the compatibility-view, but I ran into the different rendering of the top layers. Let's not add more risk to this now that it's almost properly finished though. Thanks for the thoroughness, as always. |
I'm now getting an error when loading up layer view in compatibility mode. I'm guessing you didn't get this error since you probably ran your code to test the draw range in compatibility mode, so I'll see if I can quickly find what the issue is.
|
We can't change this index to an integer, because varying variables can't be integer, even though this variable will only really be tested on vertex level. Not sure if varying was the right choice, but I don't want to change that. This prevents a crash, probably only occurring on certain GPUs. Contributes to issue CURA-8657.
Thanks for the fix & merge! I shouldn't have ignored the feeling I made it a float for a reason then, it seems.
Same. I'm never 100% sure what legacy OpenGL (pre 3.x) does and doesn't accept (also since it's mostly untestable, since everyone has their own implementation that might or might not accept newer things seemingly arbitrarily), so I'm hesitant to change from |
Please see the comments I left at 5e60cc6 |
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:
Front-end implementation of
originally: Ultimaker/Uranium#714.new: Ultimaker/Uranium#721(See also this discussion Ultimaker/Uranium#664.)
Due to technical details (see the link above), meshes ranged by index are now rendered by specifying the range of vertex-indices in the shaders, rather than specifying the element range beforehand. (Manually, as gl_primitiveID couldn't be used because of the legacy support... also this gives more control, potentially.)
Potential bonus:
The compatibility mode, (for legacy OpenGL/shaders,) could be made compatible with Simulation-View relatively easy now. Perhaps I could even do that in this PR. Keeping this relatively small for now is better though!
Also not in this PR (yet?):
RenderBatch's are still recreated on each draw. As so much of its internals are cached, either directly or via the classes it uses this might not present a problem in practice.
In Simulation-View, the range to show is recalculated for each render, even when it makes a lot more sense to cache those calculations. An earlier attempt has been made at this, then reverted, however. Rather than complicate this PR by also attempting that, leave it alone for now.
The geometry shaders are still used. That has a little less to do with this particular update (though related), but for completeness sake, I'd like to say that we could maybe also pre-calculate/upload the 'tubes' to the graphics card instead of re-calculating them in the shaders every redraw (especially since the common wisdom is that 'geometry shaders are slow' ... not sure how true that actually is though).