Skip to content

Commit

Permalink
Another batch of green VBO cleanup (openscad#5005)
Browse files Browse the repository at this point in the history 
* Fix VBO memory leak
* Remove unnecessary casting and simplify constructor args
* Simplify combine vertex cache
* Remove unused function parameters
* Remove unused append methods
* Add support for indexed rendering to CGALRenderer
* Add FIXME to remember why the VBO allocation is so complex
* Set isTriangular when triangulating PolySet
  • Loading branch information
@kintel
kintel authored Feb 22, 2024
1 parent dc2a0d1 commit 2eb528b
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 148 deletions.
1 change: 1 addition & 0 deletions src/geometry/cgal/cgalutils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ std::unique_ptr<PolySet> createPolySetFromNefPolyhedron3(const CGAL::Nef_polyhed
for (const auto& tri : allTriangles) {
polyset->indices.push_back({tri[0], tri[1], tri[2]});
}
polyset->isTriangular = true;

#if 0 // For debugging
std::cerr.precision(20);
Expand Down
76 changes: 28 additions & 48 deletions src/glview/VBORenderer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,19 +130,14 @@ size_t VBORenderer::getEdgeBufferSize(const Polygon2d& polygon) const
}

void VBORenderer::add_shader_attributes(VertexArray& vertex_array,
const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& /*normals*/,
const Color4f& /*color*/,
size_t active_point_index, size_t primitive_index,
double /*z_offset*/, size_t shape_size,
size_t /*shape_dimensions*/, bool outlines,
bool /*mirror*/) const
size_t shape_size, bool outlines) const
{
if (!shader_attributes_index) return;

std::shared_ptr<VertexData> vertex_data = vertex_array.data();

if (points.size() == 3 && getShader().data.csg_rendering.barycentric) {
if (getShader().data.csg_rendering.barycentric) {
// Get edge states
std::array<GLubyte, 3> barycentric_flags;

Expand Down Expand Up @@ -172,45 +167,30 @@ void VBORenderer::add_shader_attributes(VertexArray& vertex_array,
barycentric_flags[active_point_index] = 1;

addAttributeValues(*(vertex_data->attributes()[shader_attributes_index + BARYCENTRIC_ATTRIB]), barycentric_flags[0], barycentric_flags[1], barycentric_flags[2], 0);
} else {
if (OpenSCAD::debug != "") PRINTDB("add_shader_attributes bad points size = %d", points.size());
}
}

void VBORenderer::create_vertex(VertexArray& vertex_array, const Color4f& color,
const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& normals,
size_t active_point_index, size_t primitive_index,
double z_offset, size_t shape_size,
size_t shape_dimensions, bool outlines,
bool mirror) const
size_t shape_size,
bool outlines, bool mirror) const
{
vertex_array.createVertex(points, normals, color, active_point_index,
primitive_index, z_offset, shape_size,
shape_dimensions, outlines, mirror,
primitive_index, shape_size, outlines, mirror,
[this](VertexArray& vertex_array,
const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& normals,
const Color4f& color,
size_t active_point_index, size_t primitive_index,
double z_offset, size_t shape_size,
size_t shape_dimensions, bool outlines,
bool mirror) -> void {
this->add_shader_attributes(vertex_array, points, normals, color,
active_point_index, primitive_index,
z_offset, shape_size, shape_dimensions,
outlines, mirror);
}
);

size_t shape_size, bool outlines) -> void {
this->add_shader_attributes(vertex_array, active_point_index, primitive_index,
shape_size, outlines);
});
}

void VBORenderer::create_triangle(VertexArray& vertex_array, const Color4f& color,
const Vector3d& p0, const Vector3d& p1, const Vector3d& p2,
size_t primitive_index,
double z_offset, size_t shape_size,
size_t shape_dimensions, bool outlines,
bool mirror) const
size_t primitive_index, size_t shape_size,
bool outlines, bool mirror) const
{
double ax = p1[0] - p0[0], bx = p1[0] - p2[0];
double ay = p1[1] - p0[1], by = p1[1] - p2[1];
Expand All @@ -224,20 +204,20 @@ void VBORenderer::create_triangle(VertexArray& vertex_array, const Color4f& colo
if (!vertex_array.data()) return;

create_vertex(vertex_array, color, {p0, p1, p2}, {n, n, n},
0, primitive_index, z_offset, shape_size,
shape_dimensions, outlines, mirror);
0, primitive_index, shape_size,
outlines, mirror);
if (!mirror) {
create_vertex(vertex_array, color, {p0, p1, p2}, {n, n, n},
1, primitive_index, z_offset, shape_size,
shape_dimensions, outlines, mirror);
1, primitive_index, shape_size,
outlines, mirror);
}
create_vertex(vertex_array, color, {p0, p1, p2}, {n, n, n},
2, primitive_index, z_offset, shape_size,
shape_dimensions, outlines, mirror);
2, primitive_index, shape_size,
outlines, mirror);
if (mirror) {
create_vertex(vertex_array, color, {p0, p1, p2}, {n, n, n},
1, primitive_index, z_offset, shape_size,
shape_dimensions, outlines, mirror);
1, primitive_index, shape_size,
outlines, mirror);
}
}

Expand Down Expand Up @@ -287,7 +267,7 @@ void VBORenderer::create_surface(const PolySet& ps, VertexArray& vertex_array,
Vector3d p2 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(2)], m);

create_triangle(vertex_array, color, p0, p1, p2,
0, 0, poly.size(), 3, false, mirrored);
0, poly.size(), false, mirrored);
triangle_count++;
} else if (poly.size() == 4) {
Vector3d p0 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(0)], m);
Expand All @@ -296,9 +276,9 @@ void VBORenderer::create_surface(const PolySet& ps, VertexArray& vertex_array,
Vector3d p3 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(3)], m);

create_triangle(vertex_array, color, p0, p1, p3,
0, 0, poly.size(), 3, false, mirrored);
0, poly.size(), false, mirrored);
create_triangle(vertex_array, color, p2, p3, p1,
1, 0, poly.size(), 3, false, mirrored);
1, poly.size(), false, mirrored);
triangle_count += 2;
} else {
Vector3d center = Vector3d::Zero();
Expand All @@ -312,7 +292,7 @@ void VBORenderer::create_surface(const PolySet& ps, VertexArray& vertex_array,
Vector3d p2 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(i - 1)], m);

create_triangle(vertex_array, color, p0, p2, p1,
i - 1, 0, poly.size(), 3, false, mirrored);
i - 1, poly.size(), false, mirrored);
triangle_count++;
}
}
Expand Down Expand Up @@ -350,7 +330,7 @@ void VBORenderer::create_edges(const Polygon2d& polygon,
for (const Vector2d& v : o.vertices) {
Vector3d p0 = uniqueMultiply(vert_mult_map, Vector3d(v[0], v[1], 0.0), m);

create_vertex(vertex_array, color, {p0}, {}, 0, 0, 0.0, o.vertices.size(), 2, true, false);
create_vertex(vertex_array, color, {p0}, {}, 0, 0, o.vertices.size(), true, false);
}

GLenum elements_type = 0;
Expand Down Expand Up @@ -391,7 +371,7 @@ void VBORenderer::create_polygons(const PolySet& ps, VertexArray& vertex_array,
Vector3d p2 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(2)], m);

create_triangle(vertex_array, color, p0, p1, p2,
0, 0, poly.size(), 2, false, mirrored);
0, poly.size(), false, mirrored);
triangle_count++;
} else if (poly.size() == 4) {
Vector3d p0 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(0)], m);
Expand All @@ -400,9 +380,9 @@ void VBORenderer::create_polygons(const PolySet& ps, VertexArray& vertex_array,
Vector3d p3 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(3)], m);

create_triangle(vertex_array, color, p0, p1, p3,
0, 0, poly.size(), 2, false, mirrored);
0, poly.size(), false, mirrored);
create_triangle(vertex_array, color, p2, p3, p1,
1, 0, poly.size(), 2, false, mirrored);
1, poly.size(), false, mirrored);
triangle_count += 2;
} else {
Vector3d center = Vector3d::Zero();
Expand All @@ -419,7 +399,7 @@ void VBORenderer::create_polygons(const PolySet& ps, VertexArray& vertex_array,
Vector3d p2 = uniqueMultiply(vert_mult_map, ps.vertices[poly.at(i - 1)], m);

create_triangle(vertex_array, color, p0, p2, p1,
i - 1, 0, poly.size(), 2, false, mirrored);
i - 1, poly.size(), false, mirrored);
triangle_count++;
}
}
Expand Down
17 changes: 4 additions & 13 deletions src/glview/VBORenderer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,14 @@ class VBORenderer : public Renderer

virtual void create_triangle(VertexArray& vertex_array, const Color4f& color,
const Vector3d& p0, const Vector3d& p1, const Vector3d& p2,
size_t primitive_index = 0,
double z_offset = 0, size_t shape_size = 0,
size_t shape_dimensions = 0, bool outlines = false,
bool mirror = false) const;
size_t primitive_index = 0, size_t shape_size = 0,
bool outlines = false, bool mirror = false) const;

virtual void create_vertex(VertexArray& vertex_array, const Color4f& color,
const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& normals,
size_t active_point_index = 0, size_t primitive_index = 0,
double z_offset = 0, size_t shape_size = 0,
size_t shape_dimensions = 0, bool outlines = false,
bool mirror = false) const;
size_t shape_size = 0, bool outlines = false, bool mirror = false) const;
void add_shader_pointers(VertexArray& vertex_array); // This could stay protected, were it not for VertexStateManager
void add_color(VertexArray& vertex_array, const Color4f& color);

Expand All @@ -66,13 +62,8 @@ class VBORenderer : public Renderer

private:
void add_shader_attributes(VertexArray& vertex_array,
const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& normals,
const Color4f& color,
size_t active_point_index = 0, size_t primitive_index = 0,
double z_offset = 0, size_t shape_size = 0,
size_t shape_dimensions = 0, bool outlines = false,
bool mirror = false) const;
size_t shape_size = 0, bool outlines = false) const;

size_t shader_attributes_index{0};
enum ShaderAttribIndex {
Expand Down
36 changes: 11 additions & 25 deletions src/glview/VertexArray.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,6 @@ void VertexData::remove(size_t count)
}
}

void VertexData::append(const VertexData& data) {
size_t i = 0;
for (auto& a : attributes_) {
a->append(*(data.attributes_[i]));
i++;
}
}

void VertexArray::addSurfaceData()
{
std::shared_ptr<VertexData> vertex_data = std::make_shared<VertexData>();
Expand All @@ -57,28 +49,18 @@ void VertexArray::addEdgeData()
addVertexData(vertex_data);
}

void VertexArray::append(const VertexArray& vertex_array)
{
size_t i = 0;
for (auto& v : vertices_) {
v->append(*(vertex_array.vertices_[i]));
i++;
}
}

void VertexArray::createVertex(const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& normals,
const Color4f& color,
size_t active_point_index, size_t primitive_index,
double z_offset, size_t shape_size,
size_t shape_dimensions, bool outlines,
bool mirror, const CreateVertexCallback& vertex_callback)
size_t shape_size, bool outlines, bool mirror,
const CreateVertexCallback& vertex_callback)
{
if (vertex_callback)
vertex_callback(*this, points, normals, color, active_point_index,
primitive_index, z_offset, shape_size,
shape_dimensions, outlines, mirror);

if (vertex_callback) {
vertex_callback(*this, active_point_index,
primitive_index, shape_size, outlines);
}
addAttributeValues(*(data()->positionData()), points[active_point_index][0], points[active_point_index][1], points[active_point_index][2]);
if (data()->hasNormalData()) {
addAttributeValues(*(data()->normalData()), normals[active_point_index][0], normals[active_point_index][1], normals[active_point_index][2]);
Expand Down Expand Up @@ -213,6 +195,10 @@ void VertexArray::createInterleavedVBOs()
if (useElements()) {
GL_TRACE("glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, %d)", elements_vbo_);
GL_CHECKD(glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, elements_vbo_));
if (elements_size_ == 0) {
GL_TRACE("glBufferData(GL_ELEMENT_ARRAY_BUFFER, %d, %p, GL_STATIC_DRAW)", elements_.sizeInBytes() % (void *)nullptr);
GL_CHECKD(glBufferData(GL_ELEMENT_ARRAY_BUFFER, elements_.sizeInBytes(), nullptr, GL_STATIC_DRAW));
}
size_t last_size = 0;
for (const auto& e : elements_.attributes()) {
GL_TRACE("glBufferSubData(GL_ELEMENT_ARRAY_BUFFER, %d, %d, %p)", last_size % e->sizeInBytes() % (void *)e->toBytes());
Expand Down
28 changes: 5 additions & 23 deletions src/glview/VertexArray.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ class IAttributeData
[[nodiscard]] virtual GLenum glType() const = 0;
// Return pointer to the raw bytes of the element vector
[[nodiscard]] virtual const GLbyte *toBytes() const = 0;
// Append data to the end of the attribute
virtual void append(const IAttributeData& data) = 0;
// Clear the entire attribute
virtual void clear() = 0;
// Remove data from the end of the attribute
Expand Down Expand Up @@ -92,14 +90,6 @@ class AttributeData : public IAttributeData
[[nodiscard]] inline size_t sizeofAttribute() const override { return sizeof(T) * C; }
[[nodiscard]] inline size_t sizeInBytes() const override { return data_.size() * sizeof(T); }
[[nodiscard]] inline GLenum glType() const override { return E; }
void append(const IAttributeData& data) override {
const auto *from = dynamic_cast<const AttributeData<T, C, E> *>(&data);
if (from != nullptr) {
data_.insert(data_.end(), from->data_.begin(), from->data_.end());
} else {
assert(false && "AttributeData append type mismatch!!!");
}
}
void clear() override { data_.clear(); }
void remove(size_t count) override { data_.erase(data_.end() - (count * C), data_.end()); }
[[nodiscard]] inline const GLbyte *toBytes() const override { return (GLbyte *)(data_.data()); }
Expand Down Expand Up @@ -161,14 +151,13 @@ class VertexData
color_data_ = std::shared_ptr<IAttributeData>(attributes_.back());
}

void append(const VertexData& data);
void clear() { for (auto& a : attributes_) a->clear(); }
// Remove the last n interleaved vertices
void remove(size_t count = 1);

// Return reference to internal IAttributeData vector
[[nodiscard]] inline const std::vector<std::shared_ptr<IAttributeData>>& attributes() const { return attributes_; }
// Return reference to IAttributeData
// Return reference to the last added IAttributeData. This is typically where elements data is stored.
[[nodiscard]] inline const std::shared_ptr<IAttributeData> attributeData() const { if (attributes_.size()) return attributes_.back(); else return nullptr; }
// Return reference to position attribute data
[[nodiscard]] inline const std::shared_ptr<IAttributeData>& positionData() const { return position_data_; }
Expand Down Expand Up @@ -236,13 +225,8 @@ class VertexArray
{
public:
using CreateVertexCallback = std::function<void (VertexArray& vertex_array,
const std::array<Vector3d, 3>& points,
const std::array<Vector3d, 3>& normals,
const Color4f& color,
size_t active_point_index, size_t primitive_index,
double z_offset, size_t shape_size,
size_t shape_dimensions, bool outlines,
bool mirror)>;
size_t shape_size, bool outlines)>;


VertexArray(std::unique_ptr<VertexStateFactory> factory, std::vector<std::shared_ptr<VertexState>>& states,
Expand All @@ -264,8 +248,6 @@ class VertexArray
elements_.addAttributeData(std::move(data));
}

// Append VertexArray data to this VertexArray
void append(const VertexArray& vertex_array);
// Clear all data from the VertexArray
void clear() { for (auto& v : vertices_) v->clear(); }

Expand All @@ -276,9 +258,9 @@ class VertexArray
const std::array<Vector3d, 3>& normals,
const Color4f& color,
size_t active_point_index = 0, size_t primitive_index = 0,
double z_offset = 0, size_t shape_size = 0,
size_t shape_dimensions = 0, bool outlines = false,
bool mirror = false, const CreateVertexCallback& vertex_callback = nullptr);
size_t shape_size = 0,
bool outlines = false, bool mirror = false,
const CreateVertexCallback& vertex_callback = nullptr);

// Return reference to the VertexStates
inline std::vector<std::shared_ptr<VertexState>>& states() { return states_; }
Expand Down
Loading

0 comments on commit 2eb528b

Please sign in to comment.