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 #10259

Merged
merged 12 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:

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).

bremco and others added 4 commits July 25, 2021 22:26
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.
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.
Copy link
Collaborator

@Ghostkeeper Ghostkeeper left a 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?

cura/LayerDataBuilder.py Outdated Show resolved Hide resolved
cura/LayerPolygon.py Outdated Show resolved Hide resolved
@rburema
Copy link
Member Author

rburema commented Oct 31, 2021

@Ghostkeeper Thanks for taking this up btw!

..., 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?

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, but that would complicate Uranium with knowledge it, as a library, shouldn't have, so to speak. (Since the RenderBatch, and the newly introduced ranges, can be used for more than just the simulation-view.)

(There are .... ways around that, but I don't think it's pleasant, possibly less performant, and would require rewriting (not just) the shaders.)
(
* I understand it's not just OpenGL that does it this way.)

@rburema
Copy link
Member Author

rburema commented Oct 31, 2021

Also, I can't really reproduce the stutter here. That must've been something unique to my old Linux laptop then?

@rburema rburema requested a review from Ghostkeeper October 31, 2021 20:29
@rburema rburema dismissed Ghostkeeper’s stale review October 31, 2021 20:30

Requested changes made, questions answered (well, I hope).

@rburema
Copy link
Member Author

rburema commented Nov 7, 2021

Requested documentation added! :-)

Copy link
Collaborator

@Ghostkeeper Ghostkeeper left a 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 :)

@Ghostkeeper
Copy link
Collaborator

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:

  • Loading 1GB model in prepare mode, though I didn't really measure performance analytically. The frames were already quite smooth and I didn't notice any change. The noticeable change is definitely in layer view only.
  • Lay Flat On Face still works fine.
  • Overhang rendering still works fine.
  • X-ray view still works fine.
  • Selection still works fine.
  • Non-manifold rendering with polka dot pattern still works fine.
  • Layer view compatibility mode: A new bug arose here.

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:

Screenshot from 2021-11-08 15-36-33

However this is how Compatibility Mode for layer view now looks. The lines-rendering is not stopped at the layer slider:

Screenshot from 2021-11-08 15-35-53

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.
@rburema
Copy link
Member Author

rburema commented Nov 8, 2021

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.

@Ghostkeeper
Copy link
Collaborator

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.

QOpenGLShader::compile(Fragment): 0(23) : error C7011: implicit cast from "int" to "float"
0(23) : error C7011: implicit cast from "int" to "float"

*** Problematic Fragment shader source code ***
#ifdef GL_KHR_blend_equation_advanced
#extension GL_ARB_fragment_coord_conventions : enable
#extension GL_KHR_blend_equation_advanced : enable
#endif
#define lowp
#define mediump
#define highp
#line 1

#ifdef GL_ES
#ifdef GL_FRAGMENT_PRECISION_HIGH
precision highp float;
#else
precision mediump float;
#endif // GL_FRAGMENT_PRECISION_HIGH
#endif // GL_ES
varying lowp vec4 v_color;
varying float v_line_type;
varying highp float v_vertex_index;

uniform int u_show_travel_moves;
uniform int u_show_helpers;
uniform int u_show_skin;
uniform int u_show_infill;

uniform highp vec2 u_drawRange;

void main()
{
if (u_drawRange.x >= 0 && u_drawRange.y >= 0 && (v_vertex_index < u_drawRange.x || v_vertex_index > u_drawRange.y))
{
discard;
}
if ((u_show_travel_moves == 0) && (v_line_type >= 7.5) && (v_line_type <= 9.5))
{  // actually, 8 and 9
// discard movements
discard;
}
// support: 4, 5, 7, 10, 11
if ((u_show_helpers == 0) && (
((v_line_type >= 3.5) && (v_line_type <= 4.5)) ||
((v_line_type >= 6.5) && (v_line_type <= 7.5)) ||
((v_line_type >= 9.5) && (v_line_type <= 10.5)) ||
((v_line_type >= 4.5) && (v_line_type <= 5.5)) ||
((v_line_type >= 10.5) && (v_line_type <= 11.5))
))
{
discard;
}

// skin: 1, 2, 3
if ((u_show_skin == 0) && (
(v_line_type >= 0.5) && (v_line_type <= 3.5)
)) {
discard;
}
// infill:
if ((u_show_infill == 0) && (v_line_type >= 5.5) && (v_line_type <= 6.5))
{
// discard movements
discard;
}

gl_FragColor = v_color;
}
***
2021-11-15 14:49:25,861 - ERROR - [MainThread] UM.View.GL.ShaderProgram.setFragmentShader [147]: Fragment shader failed to compile: 0(23) : error C7011: implicit cast from "int" to "float"
0(23) : error C7011: implicit cast from "int" to "float"

2021-11-15 14:49:25,861 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: Exception: Could not set fragment shader from '/home/trin/Gedeeld/Projects/Cura/plugins/SimulationView/layers_shadow.shader'.
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: Traceback (most recent call last):
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:   File "/home/trin/Gedeeld/Projects/Uranium/UM/View/GL/OpenGL.py", line 197, in createShaderProgram
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:     shader.load(file_name, version = version_string)
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:   File "/home/trin/Gedeeld/Projects/Uranium/UM/View/GL/ShaderProgram.py", line 82, in load
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:     raise InvalidShaderProgramError("{0} is missing a shader [{1}, {2}]".format(file_name, vertex_key, fragment_key))
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: UM.View.GL.ShaderProgram.InvalidShaderProgramError: /home/trin/Gedeeld/Projects/Cura/plugins/SimulationView/layers_shadow.shader is missing a shader [vertex41core, fragment41core]
2021-11-15 14:49:25,862 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: 
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: During handling of the above exception, another exception occurred:
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: 
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: Traceback (most recent call last):
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:   File "/home/trin/Gedeeld/Projects/Uranium/UM/View/GL/OpenGL.py", line 203, in createShaderProgram
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:     shader.load(file_name, version = "")
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:   File "/home/trin/Gedeeld/Projects/Uranium/UM/View/GL/ShaderProgram.py", line 99, in load
2021-11-15 14:49:25,863 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]:     raise InvalidShaderProgramError(f"Could not set fragment shader from '{file_name}'.")
2021-11-15 14:49:25,864 - ERROR - [MainThread] UM.View.GL.OpenGL.createShaderProgram [205]: UM.View.GL.ShaderProgram.InvalidShaderProgramError: Could not set fragment shader from '/home/trin/Gedeeld/Projects/Cura/plugins/SimulationView/layers_shadow.shader'.
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [66]: An uncaught error has occurred!
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]: Traceback (most recent call last):
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]:   File "/home/trin/Gedeeld/Projects/Uranium/UM/Qt/Bindings/MainWindow.py", line 255, in _render
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]:     renderer.render()
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]:   File "/home/trin/Gedeeld/Projects/Uranium/UM/Qt/QtRenderer.py", line 164, in render
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]:     render_pass.render()
2021-11-15 14:49:25,864 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]:   File "/home/trin/Gedeeld/Projects/Cura/plugins/SimulationView/SimulationPass.py", line 65, in render
2021-11-15 14:49:25,865 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]:     self._layer_shader.setUniformValue("u_active_extruder", float(max(0, self._extruder_manager.activeExtruderIndex)))
2021-11-15 14:49:25,865 - CRITICAL - [MainThread] cura.CrashHandler.__init__ [69]: AttributeError: 'NoneType' object has no attribute 'setUniformValue'

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.
@Ghostkeeper Ghostkeeper merged commit 5e60cc6 into master Nov 15, 2021
@Ghostkeeper Ghostkeeper deleted the bremco-graphics_buffer_update branch November 15, 2021 14:24
@rburema
Copy link
Member Author

rburema commented Nov 16, 2021

Thanks for the fix & merge!

I shouldn't have ignored the feeling I made it a float for a reason then, it seems.

Not sure if varying was the right choice, but I don't want to change that.

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 varying as well. I know that works at least.

@Ghostkeeper Ghostkeeper mentioned this pull request Nov 18, 2021
2 tasks
@smartavionics
Copy link
Contributor

Please see the comments I left at 5e60cc6

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