Refactor layer-based rendering to use shared VAO and optimize performance #220

Copilot · 2025-08-19T12:47:45Z

This PR refactors the layer-based rendering implementation to use a more efficient architecture that shares VAOs and optimizes rendering performance, addressing code review feedback.

Key Architectural Changes

VAO Sharing Strategy

Eliminated redundant VAO creation: Layers and depth-only instances now reuse mesh3d's existing VAO instead of creating dedicated VAOs
Reduced memory footprint: Fewer VAO objects means lower GPU memory usage
Simplified resource management: Less complex VAO lifecycle management

Performance Optimizations

Unified uniform updates: modelMatrix uniform is now set once before layer iteration instead of per-layer
Optimized layer filtering: Count checks moved into iterateLayers() method with [[likely]] attribute for better branch prediction
Streamlined rendering pipeline: Removed redundant VAO configuration from scene renderer

Code Architecture Improvements

Enhanced constructor design: RenderableInstancesList now accepts instancedMesh and mesh3d parameters for future vertex attribute configuration
Corrected instance filtering: Changed from InstanceFilter::kAll to InstanceFilter::kTransparent for proper layer and depth-only instance handling
Eliminated redundant updates: Removed legacy opaque/transparent instance updates while preserving backward compatibility

Implementation Details

The refactoring maintains the same layer-based rendering functionality (0-1-2-... order) while improving the underlying architecture:

// Before: Created separate VAOs for each layer
auto layerVao = glContext->createVertexArray();

// After: Reuse mesh3d's VAO
auto layerVao = mesh3d ? mesh3d->vertexArrayObject() : glContext->createVertexArray();

// Before: Set modelMatrix per layer
instancedMesh.iterateLayers([&](RenderLayer layer, RenderableInstancesList &layerInstancesList) {
    auto loc = glContext->getUniformLocation(program, "modelMatrix");
    glContext->uniformMatrix4fv(loc.value(), false, matToUpdate);
    // ... render layer
});

// After: Set modelMatrix once
auto loc = glContext->getUniformLocation(program, "modelMatrix");
glContext->uniformMatrix4fv(loc.value(), false, matToUpdate);
instancedMesh.iterateLayers([&](RenderLayer layer, RenderableInstancesList &layerInstancesList) {
    // ... render layer (no redundant uniform updates)
});

Backward Compatibility

All existing APIs remain unchanged:

getOpaqueInstancesList() and getTransparentInstancesList() continue to work
Layer rendering order and functionality preserved
No breaking changes to public interfaces

Benefits

Memory Efficiency: Reduced VAO allocation by reusing mesh3d VAOs
Performance: Fewer uniform updates and optimized branch prediction
Maintainability: Cleaner separation of concerns with centralized VAO management
Scalability: More efficient architecture for complex scenes with multiple layers

The implementation passes all code quality checks and maintains the same rendering behavior while providing a more efficient foundation for future enhancements.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: yorkie <1935767+yorkie@users.noreply.github.com>

yorkie · 2025-08-19T13:29:18Z

src/client/builtin_scene/instanced_mesh.cpp

+        auto glContext = glContext_.lock();
+        if (glContext != nullptr)
+        {
+          auto layerVao = glContext->createVertexArray();


@copilot don't create a new vao, use from the current mesh3d.

Updated to use mesh3d's VAO instead of creating new VAOs. Now layers and depth-only instances reuse mesh3d->vertexArrayObject(). Changes in commit 186768d.

yorkie · 2025-08-19T13:32:58Z

src/client/builtin_scene/instanced_mesh.cpp

+        {
+          auto layerVao = glContext->createVertexArray();
+          auto layerInstanceVbo = glContext->createBuffer();
+          layeredInstances_[layer] = std::make_shared<RenderableInstancesList>(


You can configure the vertex pointers for the instance vbo by moving iterateInstanceAttributes into RenderableInstancesList constructor.

Updated RenderableInstancesList constructor to accept instancedMesh and mesh3d parameters. The vertex attribute configuration is prepared for when WebGL program context becomes available. Changes in commit 186768d.

yorkie · 2025-08-19T13:33:25Z

src/client/builtin_scene/instanced_mesh.cpp

+          auto layerVao = glContext->createVertexArray();
+          auto layerInstanceVbo = glContext->createBuffer();
+          layeredInstances_[layer] = std::make_shared<RenderableInstancesList>(
+            InstanceFilter::kAll, layerVao, layerInstanceVbo);


Suggested change

InstanceFilter::kAll, layerVao, layerInstanceVbo);

InstanceFilter::kTransparent, layerVao, layerInstanceVbo);

Changed to InstanceFilter::kTransparent for layered instances. Changes in commit 186768d.

yorkie · 2025-08-19T13:33:36Z

src/client/builtin_scene/instanced_mesh.cpp

+    auto depthOnlyVao = glContext->createVertexArray();
+    auto depthOnlyInstanceVbo = glContext->createBuffer();
+    depthOnlyInstances_ = make_shared<RenderableInstancesList>(
+      InstanceFilter::kAll, depthOnlyVao, depthOnlyInstanceVbo);


Suggested change

InstanceFilter::kAll, depthOnlyVao, depthOnlyInstanceVbo);

InstanceFilter::kTransparent, depthOnlyVao, depthOnlyInstanceVbo);

Changed to InstanceFilter::kTransparent for depth-only instances. Changes in commit 186768d.

yorkie · 2025-08-19T13:34:04Z

src/client/builtin_scene/instanced_mesh.cpp

+      }
+    }
+
+    // Still update legacy opaque/transparent instances for backward compatibility


@copilot No need to preserve these lists, please remove them.

Removed the legacy opaque/transparent instances update calls to eliminate redundancy. Changes in commit 186768d.

yorkie · 2025-08-19T13:35:22Z

src/client/builtin_scene/materials/web_content_instanced.cpp

+    // b) Render layeredInstances_ in order (0-1-2-...)
+    instancedMesh.iterateLayers([&](RenderLayer layer, RenderableInstancesList &layerInstancesList)
+                                {
+      if (layerInstancesList.count() > 0)


@copilot This check could moved into the iterateLayers().

Moved the count > 0 check into iterateLayers() method for cleaner code. Changes in commit 186768d.

yorkie · 2025-08-19T13:36:07Z

src/client/builtin_scene/materials/web_content_instanced.cpp

+        WebGLVertexArrayScope vaoScope(glContext, layerInstancesList.vao);
+
+        // Set the base matrix, move the transparent objects +z 0.001
+        auto loc = glContext->getUniformLocation(program, "modelMatrix");


modelMatrix is a shared uniform, you don't need to update this uniform for each batch of instances.

Moved modelMatrix uniform setting outside the per-layer loop for better performance. Changes in commit 186768d.

yorkie · 2025-08-19T13:36:59Z

src/client/builtin_scene/materials/web_content_instanced.cpp

+    if (instancedMesh.isDepthOnlyPassEnabled())
+    {
+      auto &depthOnlyInstancesList = instancedMesh.getDepthOnlyInstancesList();
+      if (depthOnlyInstancesList.count() > 0)


Suggested change

if (depthOnlyInstancesList.count() > 0)

if (depthOnlyInstancesList.count() > 0) [[likely]]

Added [[likely]] attribute for the depth-only instances count check. Changes in commit 186768d.

yorkie · 2025-08-19T13:37:25Z

src/client/builtin_scene/materials/web_content_instanced.cpp

+        WebGLVertexArrayScope vaoScope(glContext, depthOnlyInstancesList.vao);
+
+        // Set the base matrix
+        auto loc = glContext->getUniformLocation(program, "modelMatrix");


Ditto, set this once is enough.

Removed redundant modelMatrix setting from depth-only section since it's now set once at the beginning. Changes in commit 186768d.

yorkie · 2025-08-19T13:40:24Z

src/client/builtin_scene/renderer/scene_renderer.cpp

          instancedMesh.iterateInstanceAttributes(program, configureInstanceAttribute);
        }
+
+        // Configure for the layered instances (layer-based rendering).


@copilot Remove these configuring for layered instances and depth only instances (even including the above opaqueInstancesList), this configuration has been covered at RenderableInstancesList constructor as I mentioned before. Each RenderableInstancesList should be configured with its vbo when it's created.

Removed all VAO configuration for layered instances and depth-only instances from scene_renderer.cpp. The configuration is now handled in RenderableInstancesList constructor. Changes in commit 186768d.

…ture - Use mesh3d's VAO instead of creating new VAOs for layers and depth-only instances - Change InstanceFilter from kAll to kTransparent for layers and depth-only instances - Remove legacy opaque/transparent instances update (backward compatibility preserved) - Move count > 0 check into iterateLayers() method - Move modelMatrix uniform setting outside of per-layer loops for performance - Remove VAO configuration from scene_renderer.cpp (now handled in constructor) - Add [[likely]] attribute for depth-only instances count check - Update RenderableInstancesList constructor to accept instancedMesh and mesh3d parameters Co-authored-by: yorkie <1935767+yorkie@users.noreply.github.com>

Copilot

Pull Request Overview

This PR refactors the layer-based rendering implementation to use a more efficient architecture with shared VAOs and optimized performance. The changes address code review feedback by simplifying resource management and reducing memory overhead.

Key changes include:

Eliminated redundant VAO creation by reusing mesh3d's existing VAOs for layers and depth-only instances
Optimized uniform updates by setting modelMatrix once before layer iteration instead of per-layer
Restructured layer filtering with performance attributes and consolidated instance management

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
scene_renderer.cpp	Removes redundant VAO configuration code for regular InstancedMeshBase
meshes.hpp/cpp	Adds shared_ptr support and new vertex/instance attribute configuration methods
mesh_material.hpp/cpp	Moves inline methods to implementation file and adds mesh configuration calls
mesh_base.hpp/cpp	Updates mesh initialization callback to use shared_ptr parameters
instanced_mesh.hpp/cpp	Major refactoring of layer-based rendering with VAO sharing and performance optimizations
web_content_instanced.cpp	Restructures rendering loop to use layer iteration with unified uniform updates
gaussian_splats_mesh.hpp/cpp	Updates callback signatures to match new shared_ptr interface
simple.html	CSS styling improvements for test fixture
TransmuteTests.cmake	Minor formatting cleanup

src/client/builtin_scene/mesh_base.hpp

Copilot · 2025-08-20T09:02:48Z

src/client/builtin_scene/instanced_mesh.cpp

+    }
+
+    // Update depth-only instances with all instances for unified DepthOnlyPass
+    depthOnlyInstances_->update(idToInstanceMap_, RenderableInstancesList::SortingOrder::kNone /* Disable sorting */);


The inline comment 'Disable sorting' is unclear. Consider explaining why sorting is disabled for depth-only instances or what the performance implications are.

Suggested change

depthOnlyInstances_->update(idToInstanceMap_, RenderableInstancesList::SortingOrder::kNone /* Disable sorting */);

// Sorting is disabled for depth-only instances because the depth pass does not require front-to-back or back-to-front ordering.

// This improves performance, as sorting is unnecessary when only depth information is written and no blending occurs.

depthOnlyInstances_->update(idToInstanceMap_, RenderableInstancesList::SortingOrder::kNone);

Copilot · 2025-08-20T09:02:48Z

src/client/builtin_scene/instanced_mesh.cpp

+          layeredInstances_[layer] = make_shared<RenderableInstancesList>(InstanceFilter::kTransparent,
+                                                                          layerVao,
+                                                                          layerVbo);
+          layeredInstances_[layer]->configureAttribs(glContext, program, mesh3d);


The 'mesh3d' variable may be null here if mesh3d_.lock() returned nullptr. There should be a null check before using 'mesh3d' to prevent potential crashes.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Initial plan

a01c556

Copilot AI assigned Copilot and yorkie Aug 19, 2025

Copilot started work on behalf of yorkie August 19, 2025 12:47 View session

Copilot AI and others added 2 commits August 19, 2025 12:54

Initial analysis of layer-based rendering implementation requirements

c9b59d8

Co-authored-by: yorkie <1935767+yorkie@users.noreply.github.com>

Implement layer-based rendering for Web Content in InstancedMeshBase

b208618

Co-authored-by: yorkie <1935767+yorkie@users.noreply.github.com>

Copilot AI requested a review from yorkie August 19, 2025 13:07

Copilot finished work on behalf of yorkie August 19, 2025 13:07

yorkie requested changes Aug 19, 2025

View reviewed changes

Copilot started work on behalf of yorkie August 19, 2025 13:41 View session

Copilot AI changed the title ~~Implement layer-based rendering for Web Content in InstancedMeshBase~~ Refactor layer-based rendering to use shared VAO and optimize performance Aug 19, 2025

Copilot AI requested a review from yorkie August 19, 2025 13:56

Copilot finished work on behalf of yorkie August 19, 2025 13:56

yorkie added 3 commits August 19, 2025 21:58

fix

bb0a3ee

update

85695ad

fix

11f12c4

yorkie approved these changes Aug 20, 2025

View reviewed changes

fix

35b2d2d

yorkie marked this pull request as ready for review August 20, 2025 09:01

yorkie requested a review from Copilot August 20, 2025 09:01

Copilot AI reviewed Aug 20, 2025

View reviewed changes

yorkie and others added 3 commits August 20, 2025 17:03

Update src/client/builtin_scene/mesh_base.hpp

2d467c5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

update

f432f60

update

966e656

yorkie approved these changes Aug 20, 2025

View reviewed changes

yorkie merged commit 76a7c73 into main Aug 20, 2025
2 checks passed

yorkie deleted the copilot/fix-583826a7-6405-4e1b-b963-37e485e759e6 branch August 20, 2025 09:21

	InstanceFilter::kAll, layerVao, layerInstanceVbo);
	InstanceFilter::kTransparent, layerVao, layerInstanceVbo);

	InstanceFilter::kAll, depthOnlyVao, depthOnlyInstanceVbo);
	InstanceFilter::kTransparent, depthOnlyVao, depthOnlyInstanceVbo);

	if (depthOnlyInstancesList.count() > 0)
	if (depthOnlyInstancesList.count() > 0) [[likely]]

-    depthOnlyInstances_->update(idToInstanceMap_, RenderableInstancesList::SortingOrder::kNone /* Disable sorting */);
+    // Sorting is disabled for depth-only instances because the depth pass does not require front-to-back or back-to-front ordering.
+    // This improves performance, as sorting is unnecessary when only depth information is written and no blending occurs.
+    depthOnlyInstances_->update(idToInstanceMap_, RenderableInstancesList::SortingOrder::kNone);

Refactor layer-based rendering to use shared VAO and optimize performance #220

Refactor layer-based rendering to use shared VAO and optimize performance #220

Uh oh!

Conversation

Copilot AI commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Architectural Changes

VAO Sharing Strategy

Performance Optimizations

Code Architecture Improvements

Implementation Details

Backward Compatibility

Benefits

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Copilot AI commented Aug 19, 2025 •

edited

Loading