Mentioned problem of staled cache is here.
Due to Server and GUI is a different processes, it's impossible(AFAIU) to share in-memory created mesh between 2 of them. (For example it could be for soil shape or something what we want to deform)

So if Server plugin creates Model through ECM, it can specify file as an input (plugin could create and update mesh file periodically). But we have to pick a new name every time and the new meshes will accumulate in server and GUI processes (yes, we can remove from server side, but GUI is detached). We can workaround by adding GUI plugin which should remove old meshes, but it's quite dirty hack.

So. It could be convenient if we could update a mesh if file changed. But I need some clarification/approval to avoid breaking whole design.
Problems which I potentially see: user (of MeshManger) saved pointer to mesh which can be destroyed. Maybe we can replace only guts of mesh and keep pointer intact (but should we?).

In general, it's hard to track lifetime with such API, but redesigning/rewriting API is not very fast procedure. (also seems this class could have a good cleanup for API, but it's out of context)

Thanks for tidying up the code. It was ported from old gazebo-classic and it's overdue for cleanup / redesign.

So. It could be convenient if we could update a mesh if file changed. But I need some clarification/approval to avoid breaking whole design.

To clarify, is this the use case you're describing? e.g. MeshManager loads a common::Mesh called my_mesh_1 from my_mesh.glb. Sometime later, my_mesh.glb gets updated on disk, and it would be nice to be able load the new my_mesh.glb into my_mesh_1 again instead of adding another new mesh to the map?

I think that's fine. We could add an extra arg to Load, e.g. Load(const std::string &_filename, bool _forceReload) so that it forces the mesh manager to reload the file and updates the mesh stored in the map. Note this extra arg however break ABI so would need a slightly different approach for gz-common6.

On the other hand, I see that you modified AddMesh to support replacing a mesh. I would lean towards having a separate API to explicitly do this, e.g. UpdateMesh.

To clarify, is this the use case you're describing?

My case: we have plugin which calculates new mesh for a soil shape with some frequency. And to be visible in GUI part and visible for sensors on server part we have to re-instantiate it both processes.
From a plugin we can do it only by removing model and adding new model (maybe we can limit to visual but it's the same idea). And changes in ECM will be propagated to GUI process. BUT due to MeshManager on other side already loaded mesh with same name, it won't be reloaded. As I said, we can use other filename, but it will lead to memory bloat in MeshManager.

Interactive use case: User making model of some object in Blender. Exports it in file. Loads it as a mesh to simulator and decide to update it in Blender one more time, saves it as a file with same name and trying to load it: and instead of new model they can see old version. That's very strange user experience
And here if we check file modification time in load call, we can spot this case and do not show old mesh to user

_forceReload doesn't make sense. You can just access singleton to remove old and load new one.

On the other hand, I see that you modified AddMesh to support replacing a mesh. I would lean towards having a separate API to explicitly do this, e.g. UpdateMesh.

As I said, previous implementation had a potential flaw: memory leak if model exist. And due to comment in header file clearly explaining that we are taking ownership of object we have only 2 options: replace or destroy new. And if user somehow want to load new, maybe it will be convenient to replace.

So, let me rephrase my problem in a different way: from user(real human or plugin) we don't have too much ways how to control some aspects of simulator. If MeshManager could automatically sync from Server to GUI process I could just create mesh from memory, add it to ECM and be happy (and delete it from ECM, add it back for update or something).

And out of context comment, you can have 2 API calls which won't break an ABI.

I suppose it could be much cleaner to redesign api around or shared_ptr, or very abstract ID type (like entity_id). But it will take 1 major release to get rid of current version and I want to aligned vision with maintainers on such step (not in this PR)

(sorry, work account of ntfshard)

or remove if the debug statement if it is not intended to be left here?

It's intended, as I said earlier before we had a memory leak here in this case. Right now maybe it could be convenient to notify user that model was replaced, maybe they have some logic which is loading each time instead on once. And for persons who expected that model will be reloaded, I expect to see a ticket what this function not worked(before)

In other words, leaving (even potential) memory leak is not a great solution in my opinion. We can destroy new model and write message about it. Just want to have some agreement about system behaviour

I'll apply your proposed changes slightly later today

Thanks for tidying up the code. It was ported from old gazebo-classic and it's overdue for cleanup / redesign.

So. It could be convenient if we could update a mesh if file changed. But I need some clarification/approval to avoid breaking whole design.

To clarify, is this the use case you're describing? e.g. MeshManager loads a common::Mesh called my_mesh_1 from my_mesh.glb. Sometime later, my_mesh.glb gets updated on disk, and it would be nice to be able load the new my_mesh.glb into my_mesh_1 again instead of adding another new mesh to the map?

I think that's fine. We could add an extra arg to Load, e.g. Load(const std::string &_filename, bool _forceReload) so that it forces the mesh manager to reload the file and updates the mesh stored in the map. Note this extra arg however break ABI so would need a slightly different approach for gz-common6.

On the other hand, I see that you modified AddMesh to support replacing a mesh. I would lean towards having a separate API to explicitly do this, e.g. UpdateMesh.

same line twice
(re-pushed commit due to CI on Windows complained about temp dir)