Implement MaterialLibrary #631
Conversation
LeeTwentyThree
left a comment
LeeTwentyThree
left a comment
There was a problem hiding this comment.
Very nice system, but with some concerning parts.
|
|
||
| <ItemGroup> | ||
| <None Remove="Resources\MatFilePathMapBZ.resources" /> | ||
| <EmbeddedResource Include="Resources\MatFilePathMapBZ.resources" /> | ||
| <None Remove="Resources\MatFilePathMapSN.resources" /> | ||
| <EmbeddedResource Include="Resources\MatFilePathMapSN.resources" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
This includes both embedded resources in both DLLs, right? The size may be insignificant, but since this is loaded into memory, and because the other game's materials would be completely irrelevant, they should be included/excluded based on the build target.
| public static int Size | ||
| { | ||
| get | ||
| { | ||
| if (_resourceManager == null) | ||
| return 0; | ||
|
|
||
| var resourceSet = _resourceManager.GetResourceSet(CultureInfo.InvariantCulture, true, true); | ||
|
|
||
| if (resourceSet == null) | ||
| { | ||
| InternalLogger.Error("Failed to get the ResourceSet of the material library."); | ||
| return 0; | ||
| } | ||
|
|
||
| //Sadly, this is actually the simplest way to get the total number of entries in a .resources file. | ||
| int materialEntries = 0; | ||
| foreach (var _ in resourceSet) | ||
| materialEntries++; | ||
|
|
||
| return materialEntries; | ||
| } | ||
| } |
There was a problem hiding this comment.
I would suggest converting Size from a property to a method, e.g. GetMaterialEntriesCount or GetEntryCount.
I know those names are a bit long, but it's often best (and even a C# convention) to be more clear. Size is a bit unclear to me. But also, I'm not certain if this would be necessary to expose in the first place.
| #if SUBNAUTICA | ||
| string resourcePath = "Nautilus.Resources.MatFilePathMapSN"; | ||
| #elif BELOWZERO | ||
| string resourcePath = "Nautilus.Resources.MatFilePathMapBZ"; | ||
| #endif | ||
|
|
||
| _resourceManager = new ResourceManager(resourcePath, Assembly.GetExecutingAssembly()); |
There was a problem hiding this comment.
Couldn't Patch be removed entirely with this being done with the field's definition?
| public static IEnumerator ReplaceVanillaMats(GameObject customPrefab) | ||
| { | ||
| if (customPrefab == null) | ||
| { | ||
| InternalLogger.Error("Attempted to apply vanilla materials to a null prefab."); | ||
| yield break; | ||
| } | ||
|
|
||
| var loadedVanillaMats = new List<Material>(); | ||
| var customMatNames = new List<string>(); | ||
| foreach (var renderer in customPrefab.GetAllComponentsInChildren<Renderer>()) | ||
| { | ||
| var newMatList = renderer.materials; | ||
|
|
||
| for (int i = 0; i < newMatList.Length; i++) | ||
| { | ||
| if (newMatList[i] == null) | ||
| continue; | ||
|
|
||
| var currentMatName = MaterialUtils.RemoveInstanceFromMatName(newMatList[i].name); | ||
|
|
||
| bool skipMat = customMatNames.Contains(currentMatName); | ||
| if (!skipMat) | ||
| { | ||
| foreach (var material in loadedVanillaMats) | ||
| { | ||
| if (MaterialUtils.RemoveInstanceFromMatName(material.name).Equals(currentMatName)) | ||
| { | ||
| newMatList[i] = material; | ||
| skipMat = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (skipMat) | ||
| continue; | ||
|
|
||
| var taskResult = new TaskResult<Material>(); | ||
| yield return FetchMaterial(currentMatName, taskResult); | ||
|
|
||
| var foundMaterial = taskResult.value; | ||
|
|
||
| if (foundMaterial == null) | ||
| { | ||
| customMatNames.Add(currentMatName); | ||
| continue; | ||
| } | ||
|
|
||
| newMatList[i] = foundMaterial; | ||
| loadedVanillaMats.Add(foundMaterial); | ||
| } | ||
|
|
||
| renderer.materials = newMatList; | ||
| } | ||
| } |
There was a problem hiding this comment.
I know I'm nitpicking here, I hope it's not too annoying. That being said, I very much prefer the name ReplaceVanillaMaterials, it's much clearer and C# naming conventions almost always include avoiding abbreviations. Also, I feel that prefab makes more sense than customPrefab, but that point hardly matters at all.
| private static IEnumerator GetMaterialFromScene(string matName, string sceneName, IOut<Material> matResult) | ||
| { | ||
| matResult.Set(null); | ||
|
|
||
| if (!AddressablesUtility.IsAddressableScene(sceneName)) | ||
| { | ||
| InternalLogger.Error($"Attempted to get a material from invalid scene: {sceneName}"); | ||
| yield break; | ||
| } | ||
|
|
||
| yield return new WaitUntil(() => LightmappedPrefabs.main); | ||
|
|
||
| bool materialSet = false; | ||
| bool matCheckFailed = false; | ||
| LightmappedPrefabs.main.RequestScenePrefab(sceneName, scenePrefab => | ||
| { | ||
| foreach (var renderer in scenePrefab.GetAllComponentsInChildren<Renderer>()) | ||
| { | ||
| foreach (var material in renderer.materials) | ||
| { | ||
| if (material == null) | ||
| continue; | ||
|
|
||
| if (MaterialUtils.RemoveInstanceFromMatName(material.name).Equals(matName)) | ||
| { | ||
| matResult.Set(material); | ||
| materialSet = true; | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| matCheckFailed = true; | ||
| }); | ||
|
|
||
| yield return new WaitUntil(() => materialSet || matCheckFailed); | ||
| } |
There was a problem hiding this comment.
This is the only part that I would request an entire revision of. Unless I am entirely misunderstanding or missing something, the current code means that for every individual material that needs it, an entire Unity scene is loaded and iterated across in its entirety, with all of its renderers and materials loaded into memory and compared individually. The implications of that for even just one prefab with 10 materials from a scene is at least a bit detrimental, in my opinion. Also, I have a feeling that the WaitUntil check could run indefinitely if initialized at the wrong point in time.
| /// <summary> | ||
| /// Removes the (Instance) text from the end of a material's name, recursively. Primarily helpful for programmatic | ||
| /// material name comparisons. | ||
| /// </summary> | ||
| /// <param name="matName">The material name from which to remove the Instance string.</param> | ||
| /// <returns>The altered material name if any trailing (Instance) strings were found, otherwise the uneffected | ||
| /// matName.</returns> | ||
| public static string RemoveInstanceFromMatName(string matName) | ||
| { | ||
| string returnValue = matName; | ||
| string instanceString = " (Instance)"; | ||
|
|
||
| // We avoid using .Replace() here to account for users possibly including the instance string at the beginning | ||
| // Or in the middle of their material names. Not likely to happen, but better safe than sorry. | ||
| if (matName.EndsWith(instanceString)) | ||
| { | ||
| returnValue = matName.Substring(0, matName.Length - instanceString.Length); | ||
| return RemoveInstanceFromMatName(returnValue); | ||
| } | ||
|
|
||
| return returnValue; | ||
| } | ||
|
|
There was a problem hiding this comment.
This could probably be moved to GeneralExtensions.cs, just like GeneralExtensions.TrimClone.
2 participants
Changes made in this pull request
RemoveInstanceFromMatName(), making it easier to compare material names programmatically.Breaking changes