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

Create AssetBundleTemplate.cs #348

Merged
merged 6 commits into from
Jun 28, 2023

Conversation

EldritchCarMaker
Copy link
Contributor

Added new PrefabTemplate, used to load asset bundles and prefabs from them

@LeeTwentyThree
Copy link
Member

LeeTwentyThree commented May 18, 2023

AssetBundleTemplate(Assembly modAssembly, string assetBundleFileName, string prefabName, PrefabInfo info) seems dangerous for memory and could be slow. Otherwise looks good.

Copy link
Member

@LeeTwentyThree LeeTwentyThree left a comment

Choose a reason for hiding this comment

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

Option 1: Consider removing the overload that loads an Asset Bundle, or at least give a warning. This would only be safe in a mod that is very small or has very few assets.

Option 2: Cache loaded bundles in an internal dictionary somewhere (filename: bundle).

@EldritchCarMaker
Copy link
Contributor Author

There's still a few problems with it, but I think its good enough for use

The constructor overload that loads the bundle won't work well with mods that have multiple bundles. This is because it caches the bundles based on Assembly. Unfortunately we can't go by file name, because then multiple mods could not have bundles named, for instance, assets, or any other common and non-descript name. I think it's more important that other mods don't mess with each other with commonly named bundles than it is for the rare time when a mod uses more than one bundle

It doesn't return the bundle once it's loaded, so the user would have to load it themselves if they wanted to use the bundle for their own purposes (like loading a sprite, or other non-GameObject asset). This could be solved with an out argument, but I think that would be annoying to deal with for mods with a larger number of assets, as there would be an extra argument in there that they wouldn't need, but would still have to supply.

Unfortunately I can't think of a good way to handle these

@LeeTwentyThree
Copy link
Member

Asset Bundles already require unique names, so that can be safely used as a unique identififer.

"... the runtime asset bundle cache system uses Name & Hash pair to uniquely identify each asset bundle on disk. So if you rename a bundle, it is considered a new bundle when requested even if the hash is one you used previously." (Unity forum post)

Look into the Path.GetFileName(string) method for finding the name for identification purposes.

@LeeTwentyThree
Copy link
Member

Unfortunately we can't go by file name, because then multiple mods could not have bundles named, for instance, assets, or any other common and non-descript name. I think it's more important that other mods don't mess with each other with commonly named bundles than it is for the rare time when a mod uses more than one bundle

Correct me if I'm wrong but I believe this is already the case. In the past I have had to rename my asset bundles because of conflicting mods.

@LeeTwentyThree
Copy link
Member

LeeTwentyThree commented May 19, 2023

How about this change/overload using Assembly.GetCallingAssembly()?

// private static Dictionary<Assembly, AssetBundle> _loadedBundles = new Dictionary<Assembly, AssetBundle>();
// use asset bundle file name for identifier
private static Dictionary<string, AssetBundle> _loadedBundles = new Dictionary<string, AssetBundle>();

/// ....
public AssetBundleTemplate(string assetBundleFileName, string prefabName, PrefabInfo info) : base(info)
    {
        AssetBundle bundle;

        if(!_loadedBundles.TryGetValue(assetBundleFileName, out bundle))
        {
            bundle = Utility.AssetBundleLoadingUtils.LoadFromAssetsFolder(Assembly.GetCallingAssembly(), assetBundleFileName);
            _loadedBundles.Add(assetBundleFileName, bundle); // iirc you forgot to add it to the cache
        }
        
        _prefab = bundle.LoadAsset<GameObject>(prefabName);
    }

@LeeTwentyThree
Copy link
Member

I would like to see this in Nautilus soon but currently I think as it is implemented it could be improved, or do you think it's fine as is?

@EldritchCarMaker
Copy link
Contributor Author

Look into the Path.GetFileName(string) method for finding the name for identification purposes.

This just returns the file name of the bundle, which is already included in the method arguments anyway, so this won't help much

@EldritchCarMaker
Copy link
Contributor Author

Unfortunately we can't go by file name, because then multiple mods could not have bundles named, for instance, assets, or any other common and non-descript name. I think it's more important that other mods don't mess with each other with commonly named bundles than it is for the rare time when a mod uses more than one bundle

Correct me if I'm wrong but I believe this is already the case. In the past I have had to rename my asset bundles because of conflicting mods.

The bundle names themselves have to be different, yes, but the file names can be the same. I just tested this in game using two separate bundles, where I renamed one to the same as the other. Both bundles had the same file name, and could be loaded together just fine. Once they were loaded, you could use bundle.name to get the actual unique name of the bundle, but that doesn't help much when trying to load/cache them as you'd only be able to access that bundle.name field once it's been loaded

It's now gotten automatically via Assembly.GetExecutingAssembly

Also I did forget to add the bundle to the cache
@EldritchCarMaker
Copy link
Contributor Author

There's three routes we can use to cache the bundles

We can go by assembly, which will work perfectly for the vast majority of mods as very few have multiple asset bundles for the same assembly, but won't work 100% of the time because sometimes, very rarely, a mod may use two separated asset bundles.

We can also go by file name, which will allow mods to have multiple bundles but also means that any two mods that have the same file names as each other would be unable to use the template, because the caches would interfere (note: you can still use two bundles with the same file name, as long as those two bundles do not share the same internal name. It's unlikely for people to rename their bundles, but it is still a possibility.)

Or we can go by both, and use a tuple or some other struct to store both the assembly and the file name. This would, I believe, solve both issues. But I'm also lazy and don't think it's worth doing

I think the best option is to stick with the assembly caching, as the vast majority of mods don't use multiple bundles so they wouldn't be affected in any major way, and there is only one mod that I know of that uses two bundles and it wouldn't be using this template anyway.

@LeeTwentyThree
Copy link
Member

LeeTwentyThree commented Jun 16, 2023

Not sure why assembly caching would be a good idea, when we already have bundle names to go by which should be a lot safer. It is not unusual to use multiple asset bundles. For example I have a mod project that uses an ancient asset bundle alongside a new one because we lost the source unity project and can't merge all the assets into one bundle. Any large mod could use it.

On the other hand, renaming an asset bundle seems quite unusual to me.

@EldritchCarMaker
Copy link
Contributor Author

recheck

@github-actions
Copy link
Contributor

github-actions bot commented Jun 27, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@EldritchCarMaker
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Jun 27, 2023
@LeeTwentyThree LeeTwentyThree merged commit dad7ddd into SubnauticaModding:master Jun 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants