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

Make OpenSim::Mesh refcounted on copy #4010

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

Conversation

adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Feb 3, 2025

This fixes a practical issue that I encounter downstream in opensim-creator when loading/editing models that contain a large amount of expensive-to-load mesh data.


In OSC, OpenSim::Models are copied around quite a bit. E.g. whenever a user performs any model-editing action, it takes a copy of the model, so that if any user edits result in an error/exception there's an easy way to roll it back. Undo/redo works similarly, simulating a model on a separate thread works similarly, and so on - it's computationally expensive, but very robust to user-enacted edits, which OSC has to manage a lot of.

My problem is that, whenever a system wants to use a model copy, it usually has to re-call buildSystem, finalizeFromProperties, etc. on the copy. If the model has a lot of mesh data, it can take a very long time (e.g. multiple seconds) for the implementation to re-load and re-parse the mesh data from disk. It also increases memory usage in the case where multiple fully-finalized-and-built models are used concurrently (e.g. one model in the UI, one in a simulator).

If you want a concrete example, load https://simtk.org/projects/dogmodel in OpenSim Creator, edit it a bit (probably fast) and then Undo it. The undo will take forever because the model copy (in the undo buffer) will be re-finalized and reload all the mesh data.


The solution is to reference-count the mesh data once it's loaded, so that all copies of OpenSim::Mesh share a pointer to an immutable copy of the mesh data. This, however, has some additional drawbacks/considerations:

  • OpenSim::Mesh loading needs to happen more proactively, during extendFinalizeFromProperties, because copies may be made before generateDecorations is called (which is what's currently lazy-loading mesh data). **This might affect how @aymanhab is loading mesh data in OpenSim GUI (i.e. is the lazy loading still necessary)?
  • extendFinalizeFromProperties The implementation now has to track whether the underlying (resolved) filesystem path has changed, or whether the timestamp of the underlying mesh data is newer than what's loaded, because the existing implementation always reloaded, but we're now caching (so we need a cache invalidation mechanism). Therefore, the implementation now has to store+check paths, file timestamps, etc.

Brief summary of changes

  • Rewrite OpenSim::Mesh::extendFinalizeFromProperties to perform property checking, mesh loading, and mesh caching

Testing I've completed

  • Ensured it worked in OpenSim Creator. I imagine the existing unit test suite is going to flag the issue with eagerly loading mesh data 😉

Looking for feedback on...

  • Refactor of OpenSim::Mesh::extendFinalizeFromProperties. I didn't just whack the caching in. I also ended up rewriting most of the existing code in order to make it easier to understand what each step is doing.

CHANGELOG.md

  • updated.

This change is Reviewable

@adamkewley adamkewley changed the title Make OpenSim::Mesh refcounted on-copy WIP: Make OpenSim::Mesh refcounted on-copy Feb 3, 2025
@adamkewley
Copy link
Contributor Author

WIP until tests etc. are passing: this change fundamentally is going to mean that test suites that only called finalizeFromProperties but not generateDecorations might now have issues due to missing geometry.

@adamkewley adamkewley changed the title WIP: Make OpenSim::Mesh refcounted on-copy Make OpenSim::Mesh refcounted on-copy Feb 5, 2025
@adamkewley adamkewley changed the title Make OpenSim::Mesh refcounted on-copy Make OpenSim::Mesh refcounted on copy Feb 5, 2025
Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable change; I had one minor suggestion.

Moco internally makes copies of models to enable parallel computations in CasADi. I can't see how these change could affect Moco optimizations, since we never interact with mesh files, but figured I would mention it.

Tagging @aymanhab for a review to answer GUI-related questions.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @adamkewley)


OpenSim/Simulation/Model/Geometry.cpp line 348 at r2 (raw file):

    const auto* model = findFirstOwnerOfType<OpenSim::Model>(*this);
    if (!model) {
        log_error("Mesh {} not connected to model...ignoring", get_mesh_file());

Minor:

Suggestion:

        log_error("Mesh {} not connected to a model...ignoring", get_mesh_file());

@nickbianco nickbianco requested a review from aymanhab February 6, 2025 19:28
@adamkewley
Copy link
Contributor Author

👍

Moco internally makes copies of models to enable parallel computations in CasADi. I can't see how these change could affect Moco optimizations, since we never interact with mesh files, but figured I would mention it.

If you're copying the models and then re-finalizing them in order to do something useful with them in the background thread, I imagine you'll see a quite large performance uplift if the model contains non-trivial meshes

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