Conversation
…file endings in shader and source files
# Conflicts: # examples/render/808-test-lines/glfw/main.cpp
There was a problem hiding this comment.
Pull Request Overview
This PR introduces GPU-accelerated line rendering by implementing a GPU-generated lines feature using compute shaders. The implementation enables rendering of lines with vertex/index buffer generation performed on the GPU rather than the CPU.
- Adds GPU compute shader-based line rendering support to complement existing CPU-generated lines
- Implements GPU line generation through compute shaders and embedded shader programs
- Updates line rendering examples to include the new GPU implementation option
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vclib/render/src/vclib/bgfx/programs/embedded_c_programs/custom_gpu_lines.cpp | Implements compute shader loader for the CUSTOM_GPU_LINES program with platform-specific shader support |
| vclib/render/src/vclib/bgfx/primitives/lines/gpu_generated_lines.cpp | Core implementation of GPU-generated lines class with compute buffer management and line generation |
| vclib/render/shaders/vclib/bgfx/primitives/lines/gpu_generated_lines/cs_compute_buffers.sc | Compute shader for generating line vertex and index buffers on GPU |
| vclib/render/include/vclib/bgfx/programs/embedded_c_programs/custom_gpu_lines.h | Header declaring the compute shader loader specialization |
| vclib/render/include/vclib/bgfx/programs/embedded_c_programs.h | Includes the custom GPU lines header |
| vclib/render/include/vclib/bgfx/programs/compute_program.h | Adds CUSTOM_GPU_LINES to the compute program enumeration |
| vclib/render/include/vclib/bgfx/primitives/lines/gpu_generated_lines.h | Public header defining the GPUGeneratedLines class interface |
| vclib/render/include/vclib/bgfx/primitives/lines.h | Integrates GPU lines implementation into the main Lines class |
| examples/render/808-test-lines/qt/main.cpp | Updates UI to include GPU Generated option |
| examples/render/808-test-lines/glfw/main.cpp | Updates UI and increases line count to test GPU performance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
vclib/render/src/vclib/bgfx/programs/embedded_c_programs/custom_gpu_lines.cpp
Outdated
Show resolved
Hide resolved
vclib/render/src/vclib/bgfx/primitives/lines/gpu_generated_lines.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| const VertexBuffer& vertexCoords, | ||
| const VertexBuffer& vertexNormals = VertexBuffer(), | ||
| const VertexBuffer& vertexColors = VertexBuffer(), | ||
| const VertexBuffer& lineColors = VertexBuffer()); |
There was a problem hiding this comment.
lineColors should be an IndexBuffer. For consistency, everything that is not a vertex property should be in an IndexBuffer (see triangle colors and normals in the MeshRenderBuffers class)
| const IndexBuffer& lineIndices, | ||
| const VertexBuffer& vertexNormals = VertexBuffer(), | ||
| const VertexBuffer& vertexColors = VertexBuffer(), | ||
| const VertexBuffer& lineColors = VertexBuffer()); |
There was a problem hiding this comment.
lineColors should be an IndexBuffer. For consistency, everything that is not a vertex property should be in an IndexBuffer (see triangle colors and normals in the MeshRenderBuffers class)
| mLinesImplementation; | ||
| LinesImplementation mLinesImplementation; | ||
|
|
||
| public: |
There was a problem hiding this comment.
Missing:
- constructors and setPoints member with buffers
- the member function
defaultImplementationType(bool)should return the implementation to use by checking the caps. In the case of GPU supported, default should be GPU.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
the member function
defaultImplementationType(bool)should return the implementation to use by checking the caps. In the case of GPU supported, default should be GPU.
is this actually done?
| VertexBuffer mVertexCoords; | ||
| VertexBuffer mVertexNormals; | ||
| VertexBuffer mVertexColors; | ||
| VertexBuffer mLineColors; | ||
| IndexBuffer mLineIndices; |
There was a problem hiding this comment.
This should be carefully tested, but I think these buffers should not be members of this class, because they don't belong to the state of a GPUGeneratedLines object.
They are used temporarily to create the buffers from the data on CPU memory. They should be created only inside the setPoints method that takes std::vectors, used to generate the Vertex and Index buffers, and then destroyed.
The only issue that comes into my mind is related to any platform-dependent problem that requires that buffers need to be alive also after the bgfx::dispatch call, therefore we need to be sure that works on every platform.
I did a quick test and works fine on Linux and MacOS.
vclib/render/shaders/vclib/bgfx/primitives/lines/gpu_generated_lines/cs_compute_buffers.sc
Outdated
Show resolved
Hide resolved
# Conflicts: # vclib/render/include/vclib/bgfx/primitives/lines.h
…ces input vector is provided
# Conflicts: # vclib/render/shaders/vclib/bgfx/primitives/lines/cpu_generated_lines/varying.def.sc # vclib/render/src/vclib/bgfx/primitives/lines/cpu_generated_lines.cpp
No description provided.