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

Lines pr v2 #4

Open
wants to merge 117 commits into
base: main
Choose a base branch
from
Open

Lines pr v2 #4

wants to merge 117 commits into from

Conversation

bigmat18
Copy link
Contributor

@bigmat18 bigmat18 commented Feb 1, 2025

[render] Adding custom lines system

ToDo:

Major:

  • Lines should be indexed: a buffer for the vertices, and an index buffer with two indices per line.
  • Review the usage of the LinesVertex class.
    • Right now, the Lines and Polylines classes allow to set points exclusively with std::vector<LinesVertex> as input.
    • What I expect is that each class has the following members (data should be passed directly to shaders, not copied at all (with the exception of cpu and instancing)):
      • void setPoints(const std::vector<float>& vertCoords, const std::vector<uint>& vertColors = std::vector<uint>(), const std::vector<float>& vertNormals = std::vector<float>()); (this call computes implicitly the line indices)
      • void setPoints(const std::vector<float>& vertCoords, const std::vector<uint>& lineIndices, const std::vector<uint>& vertColors = std::vector<uint>(), const std::vector<float>& vertNormals = std::vector<float>(), std::vector<uint>& lineColors = std::vector<uint>());
    • Additionally, for GPU, Indirect and Texture based, the following members should be available:
      • void setPoints(uint nVertices, const VertexBuffer& vertCoords, const VertexBuffer& vertColors = VertexBuffer(), const VertexBuffer& vertNormals =VertexBuffer());
      • void setPoints(uint nVertices, uint nLines, const VertexBuffer& vertCoords, const IndexBuffer& lineIndices, const VertexBuffer& vertColors = VertexBuffer(), const VertexBuffer& vertNormals = VertexBuffer(), const IndexBuffer& lineColors = IndexBuffer());
  • Lines and polylines (mostly in shaders) use sometimes rgba, sometimes abgr. This is confusing. bgfx uses by default abgr (in little endian), and that format should be used always by all the lines and polylines shaders.
    • utils.sh contained a function called uintToVec4FloatColor(), (now renamed to uintRGBAToVec4FloatColor() in shaders_common.sh); use the one already provided in shaders_common.sh: uintABGRToVec4Color (which is inverted)
  • Use lines also for mesh edges (to do this, indices and per-line colors are necessary)
  • MeshRenderData shouldn't be modified. All the settings for the lines (wireframe in this case) should be moved in MeshRenderBuffer, which is a class that depends on bgfx. MeshRenderBuffers is not.
  • Review all the usages of push_back(). Most of the times it is used without making a reserve() before (and it is possible to do it).
  • Each <method>Lines and <method>PolyLines class is a DrawableObject, but this is not necessary (no need to implement copy and clone for each of them). Only the effective DrawableLines class must be a DrawableObject! (doing this simplifies the next point, since a DrawableObject MUST have available the data necessary to create the buffers always... If each <method>Lines class is not a DrawableObject, it can avoid to have always the points available. Only the DrawableLines class must have it).
    • Do the same for PolyLines
  • bgfx::makeRef expects that the data is available for two frames (and apparently this constraint becomes more strict when using DynamicBuffers). All the buffers of Lines do not take into account this. This causes crashes on windows-dx11 (always) and sometimes on linux-vulkan.
    • Do the same for PolyLines
  • All the Lines classes create objects in an inconsistent state when constructed with their empty constructor: they cannot be updated later:
    lines::CPUGeneratedLines l;
    // .. some computations...
    l.update(points); // <- runtime error. 
    
    • Do the same for polylines
  • Instancing: Too easy to get runtime errors like:
    Failed to allocate instance data buffer (requested 138660, available 98304). Use bgfx::getAvailTransient* functions to ensure availability.
    
    These cases should be managed.
  • Lines/Polylines do not work on dx11 when GPU, indirect and texture based;
  • manage limit of 65535 thread groups in dx compute shaders
  • fix w error on polylines
  • there is too much copy/paste in Lines/Polylines classes and shaders. And therefore they sometime have the same bugs. Reorganize the code in such way that the shared code is available for all the implementations.

Minor:

  • An example that allows to test all the lines should be provided.
    • Add also an example for polylines
    • fix color bug on GPU generated lines/polylines - why only gpu have different colors? Is this due to discrepancies between rgba and abgr formats?
  • Fix windows build
  • Fix opengl build (related with major MeshRenderData)
  • Lines and PolyLines implementations have a member function getSettings() marked const that returns a non-const pointer...
  • GPUGeneratedLines, IndirectBasedLines and TexturedBasedLines (and their polyline implementation) store useless mPoints vector
  • Resolve reviews

Style:

  • A proper documentation explaining the differences between the methods used to draw lines should be provided
  • polish all new files - add copyright and header guards (remove #pragma once)
  • check all new enums - they should be converted to enum classes
  • remove namespace vcl::lines
  • indexes -> indices
  • joines -> joins
  • Instancing: use always reinterpret_cast instead of c-like cast

Refactor:

  • use whenever is possible the classes VertexBuffer and IndexBuffer: less code to write and less error-prone resources management (e.g. destructor and move)
    • do the same for polylines
  • create() static function pattern in DrawableLines and DrawablePolylines should be revisited
  • shader utils function defined in bgfx/drawable/drawable_lines/utils.sh should be moved to bgfx/shaders_common.sh
  • LinesSettings class should be splitted in LinesSettings and PolylineSettings

bigmat18 and others added 30 commits January 6, 2025 01:29
Merge remote origin/main
# Conflicts:
#	examples/render/910-viewer-imgui/glfw/main.cpp
#	examples/render/CMakeLists.txt
#	vclib/render/include/vclib/bgfx/drawable/drawable_mesh.h
#	vclib/render/include/vclib/bgfx/drawable/mesh/mesh_render_buffers.h
# Conflicts:
#	tests/render/000-static-asserts/drawers.h
#	vclib/render/include/vclib/imgui/imgui_drawer.h
# Conflicts:
#	examples/render/CMakeLists.txt
#	vclib/render/include/vclib/bgfx/drawable/drawable_mesh.h
#	vclib/render/include/vclib/bgfx/drawable/mesh/mesh_render_buffers.h
Comment on lines 161 to 168
// now, bind uniform for draw
// TODO - this should be the same of before..........
float indirectData[] = {
static_cast<float>(pointSize - 1),
static_cast<float>(mMaxTextureSize),
0,
0};
mIndirectData.bind(indirectData);
Copy link
Member

Choose a reason for hiding this comment

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

TODO: modify the shaders in order to have this uniform consistent (also with lines implementation).
Choose an order and use the same in all the places.

# Conflicts:
#	vclib/render/include/vclib/bgfx/drawable/drawable_mesh.h
# Conflicts:
#	vclib/render/include/vclib/bgfx/drawable/drawable_mesh.h
#	vclib/render/src/vclib/bgfx/context/embedded_shaders/lines_cpu_generated_vsfs.cpp
#	vclib/render/src/vclib/bgfx/context/program_manager.cpp
# Conflicts:
#	vclib/render/include/vclib/bgfx/context/embedded_shaders/drawable_mesh_wireframe.h
#	vclib/render/include/vclib/bgfx/drawable/drawable_mesh.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/drawable_mesh_wireframe.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_cpu_generated_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_gpu_generated_cs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_indirect_based_cs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_indirect_based_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_instancing_based_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_texture_based_cs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/lines_texture_based_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_cpu_generated_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_gpu_generated_cs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_indirect_based_cs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_indirect_based_joints_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_indirect_based_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_instancing_based_joints_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_instancing_based_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_texture_based_cs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_texture_based_joints_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/embedded_vf_programs/polylines_texture_based_vsfs.h
#	vclib/render/include/vclib/bgfx/programs/vert_frag_loader.h
#	vclib/render/include/vclib/bgfx/programs/vert_frag_program.h
#	vclib/render/src/vclib/bgfx/context/embedded_shaders/drawable_mesh_wireframe.cpp
#	vclib/render/src/vclib/bgfx/context/program_manager.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/drawable_mesh_wireframe.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_cpu_generated_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_gpu_generated_cs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_indirect_based_cs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_indirect_based_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_instancing_based_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_texture_based_cs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines_texture_based_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_cpu_generated_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_gpu_generated_cs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_indirect_based_cs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_indirect_based_joints_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_indirect_based_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_instancing_based_joints_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_instancing_based_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_texture_based_cs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_texture_based_joints_vsfs.cpp
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/polylines_texture_based_vsfs.cpp
# Conflicts:
#	vclib/render/include/vclib/bgfx/programs/compute_program.h
#	vclib/render/src/vclib/bgfx/programs/embedded_vf_programs/lines.cpp
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.

2 participants