Skip to content

perf: avoid copying rvalue meshes when adding model volumes - #12116

Open
VailElla wants to merge 1 commit into
bambulab:masterfrom
VailElla:codex/pr-modelvolume-move
Open

perf: avoid copying rvalue meshes when adding model volumes#12116
VailElla wants to merge 1 commit into
bambulab:masterfrom
VailElla:codex/pr-modelvolume-move

Conversation

@VailElla

@VailElla VailElla commented Sep 3, 2026

Copy link
Copy Markdown

Summary

  • avoid deep-copying TriangleMesh when ModelObject::add_volume receives an rvalue
  • preserve convex-hull initialization after the mesh is moved into the volume
  • add a regression test that verifies the vertex and index buffers are transferred

Validation

  • cmake --build build/arm64 --target libslic3r --parallel 8
  • focused fff_print_tests regression: ModelObject moves rvalue mesh storage into a volume
  • git diff --check

The full fff_print_tests target is currently blocked by pre-existing test sources that use an older GCodeWriter API (set_extruder, lift, and retract_lift); those failures are unrelated to this change.

@Haidiye00 Haidiye00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, under what bug is this? Does it need to be fixed this way? Can you tell me what this bug is?

@VailElla

VailElla commented Sep 3, 2026

Copy link
Copy Markdown
Author

Hi, thank you for taking a look.

This PR is not intended to fix a user-visible functional bug. It addresses an unnecessary deep copy on an existing rvalue code path.

ModelObject::add_volume(TriangleMesh&&) already forwards the mesh as an rvalue to ModelVolume. Before this change, ModelVolume did not have a matching TriangleMesh&& constructor, so the call selected the const TriangleMesh& constructor. That constructor creates a new TriangleMesh(mesh), copying the vertex and index buffers even though the rvalue no longer needs to retain its original storage.

The added constructor keeps the existing std::shared_ptr storage model, but moves the mesh into that shared object. This preserves immutable mesh storage, leaves the lvalue overload and other ownership behavior unchanged, and keeps the existing convex-hull initialization.

The regression test checks that the vertex and index buffer addresses are transferred rather than copied, and that the convex hull is still initialized. On high-poly models, this path can involve millions of vertices and indices, so the goal is to reduce loading memory traffic and peak memory without changing geometry or slicing behavior.

I chose this implementation to keep the change local to ModelVolume and avoid changing the existing ModelObject::add_volume API. If you prefer this to be tracked under a performance issue, or would like a dedicated benchmark added to the PR, I can add that and adjust the patch accordingly.

@Haidiye00 Haidiye00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approve

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