Some issues and code review
Opened this issue · 1 comments
While testing Sandcastle together with the stock inventory mod I'm putting together, I noticed a few things, and I figured that would be useful for you :
InventoryUtils.AddItem()
The InventoryUtils.AddItem() method is doing something very wrong, it calls StoreCargoPartAtSlot() with a reference to the part prefab. You simply can't do that. First, this mean you're calling Save()/OnSave() on the prefab, which mean you're executing some code that might alter the pristine prefab state, potentially having wide side effects on the entire game. Second, this will fail to produce a valid persisted state, as saving a non-initialized part is an unsupported scenario. There are multiple stock modules that simply throw an exception while doing so, and the situation will likely be even worse with modules from various plugins. You need to rework that method to first instantiate the part, then call StoreCargoPartAtSlot() with that instance. For the same reason, the StoreCargoPartAtSlot() overload that takes a part name shouldn't be used (and is actually unused by the stock code).
Mass & volume checks
In various InventoryUtils methods, you're relying on the part prefab mass / resource mass to check against the inventory available mass capacity. There is no guarantee that these values will be in line with the part instance real mass due to modules dynamically altering resources and IPartMassModifier modules. I guess this isn't much of an issue in practice since only Kerbal inventories have mass limits, and I guess you're not allowing storing parts there.
In the context of the stock inventory "extension" mod I'm putting together, the same issue would happen with cargo volume, as I'm implementing support for variable volume for mesh switchable / procedural parts.
In both cases, this is a tricky issue to fix, as there is no way to reliably get mass/volume without an actual part instance. My suggestion would be to rework how you're handling things, by creating the cargo part when its construction is requested instead of creating it when it is completed. Then you could have some special handling that "lock" the cargo part in the inventory (preventing it from being removed or interacted with) until its construction is done. But this would be a major rewrite and quite some work to implement.
An easier workaround would be to instantiate a dummy part every time you need to check mass volume, but this would be quite inefficient.
Sounds like I misunderstood the API. Was there documentation that I missed on how to use StoreCargoPartAtSlot?