Potential issue with asset removal Problem

[GitNiko]

https://github.com/andriyDev/landmass/blob/0e3680e29ea0020966177955ede8a5e931918367/crates/landmass_rerecast/src/lib.rs#L219C73-L220C1

  for rerecast_id in changed_rerecast_ids {
    let Some(landmass_id) = mapping.get(rerecast_id) else {
      // The mapping has been cleaned up since we got the event OR this mesh
      // never had a mapping.
      continue;
    };

    let Some(rerecast_mesh) = rerecast_meshes.remove(rerecast_id) else { // <------here
      // We always send an event the first time the conversion is created in
      // case this asset exists. But it is also perfectly valid for this asset
      // to not exist and still be loading.
      continue;
    };
    let landmass_mesh =
      convert_rerecast_navmesh_to_landmass_navmesh(&rerecast_mesh);
    let landmass_mesh = match landmass_mesh.validate() {
      Ok(landmass_mesh) => landmass_me

I noticed a potential issue in the convert_changed_rerecast_meshes_to_landmass system where rerecast_meshes.remove(rerecast_id) is used to get the rerecast mesh for conversion. This seems problematic as it permanently removes the asset from the Assets storage.

Questions:

• Is this remove() call intentional, or should it be get()?
• Are there any specific reasons why the original rerecast mesh needs to be removed after conversion?
• Could this cause issues when multiple entities reference the same rerecast mesh?

[andriyDev]

Hi there! This definitely should have been documented here. This was intentional. The reasoning was that I didn't want to be keeping the rerecast mesh in RAM when practically users should only really care about the landmass version. No reason to keep 2 instances in memory that represent the same data!

We don't need an owned instance of the rerecast mesh for anything, but removing it lets us drop it earlier. In theory I could support a version that doesn't remove the duplicate instance, but I'd need a pretty compelling real-world case for that.

I don't think this should cause issues of multiple entities referencing the same rerecast mesh? I have a test for this. The RerecastToLandmassIds resource maps the asset ID of the rerecast mesh to the landmass asset ID. We only remove mappings when the rerecast mesh handle becomes unused.

Is there a particular situation you are concerned about? Do you need the rerecast mesh for something?

[andriyDev]

This line is certainly worth documenting about this perf decision either way!

[GitNiko]

I encountered this issue in my project where I:

1. Hold handles to bevy_rerecast::Navmesh assets as documented in bevy_rerecast
2. Extended the navmesh functionality using custom traits with methods like closest_point_on_navmesh() to prevent characters from getting stuck outside the navigation mesh
3. Use these handles and methods in my game systems

When the conversion system runs and calls rerecast_meshes.remove(rerecast_id), my handles suddenly point to removed assets, causing my custom methods to fail. This breaks the fundamental expectation that asset handles remain valid as long as they're held.

// My usage pattern that gets broken:
trait NavmeshExt {
    fn closest_point_on_navmesh(&self, point: Vec3) -> Option<Vec3>;
    // other utility methods...
}

impl NavmeshExt for bevy_rerecast::Navmesh {
    // implementation...
}

// Later in my systems:
fn prevent_character_getting_stuck(
    navmeshes: Res<Assets<Navmesh>>,
    navmesh_handle: Res<NavmeshHandle>,
    // other params...
) {
   let Some(navmesh) = navmeshes.get(&navmesh_handle.0) else {
            info!("Navmesh not found");
            return;
        };
  // ...
}

Of course, users could directly use pub struct NavMeshHandle3d(pub Handle<bevy_rerecast::Navmesh>); to avoid this issue, but this requires deep familiarity with the landmass plugin's asset removal mechanism. Otherwise, users are likely to encounter the same problem.

More fundamentally, I think we should carefully consider whether it's appropriate for one plugin to remove assets belonging to another plugin. This crosses plugin boundaries and can break assumptions that users make based on the documentation of the original plugin (bevy_rerecast in this case).

[andriyDev]

In regards to preventing agents from getting stuck outside the navigation mesh: you could use Archipelago::sample_point to snap the characters back to the nav mesh. That's just the one method of course, not sure if there are more things you're missing. Please share more if landmass is missing something else!

This "removing assets" trick isn't new. Render assets have RenderAssetUsage that determines whether regular meshes should be removed from the main world and only kept on the GPU - it's the same situation, you have live handles to assets that were removed. The primary difference here is "crossing crate boundaries".

There is an escape hatch of using convert_rerecast_navmesh_to_landmass_navmesh directly and managing the lifetimes of assets manually. I think that's a reasonable middle ground if you really care about keeping the rerecast navmesh around. I'd rather make it easy for users of landmass to have the "right thing" happen automatically (not keeping around useless memory).

[janhohenheim]

Didn't fully read this yet, but I believe removing the rerecast asset should be fine in theory. Even the original implementation doesn't keep that navmesh, but transforms it into a specialized Detour navmesh struct.