Refactor MSB gen to consolidate interior/exterior code#97
Conversation
horfius
left a comment
There was a problem hiding this comment.
I haven't reviewed the logic deeply for Main.cs, but in general for C# you can make a lot of these more straightforward interface functions into arrow operator style, e.g.
public int GetMap() => map;
or into property style
public interface IMSBCompilableGroup
{
int Map { get; }
}
public class InteriorChunk : IMSBCompilableGroup
{
int Map { get; set; } // can get rid of map member and change references to this
}
Thanks, I forgot you could do that in C#. I've updated it to use the property style. |
Additional MSB gen cleanup
|
I've cleaned up some additional stuff, I'll leave it at that for now unless other changes are requested. |
|
|
||
| warps = new(); | ||
| positions = new(); | ||
| Paths = []; |
There was a problem hiding this comment.
You can do inline initialization of properties to clean this up a little,
public List<AssetContent> Assets { get; init; } = [] (this one might cause a compiler error if you are initializing in a constructor)
or
public List<AssetContent> Assets { get; } = [] if there are no constructors that do anything other than a simple init
| Lort.NewTask("Generating MSB", layout.tiles.Count); | ||
|
|
||
| foreach (BaseTile tile in layout.all) | ||
| void GenerateMSB(IMSBCompilableGroup group) |
There was a problem hiding this comment.
This whole section is well cleaned up, but I want @infernoplus to take a look and see if there are cases where the logic should be separated a bit.
| bed.TalkID = character.GetESD(group.IdList(), msb, bedContent); | ||
|
|
||
| if (group.IsInterior) { | ||
| bed.CollisionPartName = rootCollision.Name; |
There was a problem hiding this comment.
Why is this behind group.IsInterior now? I can't think of an exterior bed in all morrowind but we don't want to possibly exclude mods.
There was a problem hiding this comment.
All of the code blocks like this that reference rootCollision were previously only in the interior loop, so I had to add this check to maintain the same behaviour. It's possible that this is unnecessary and works fine for exteriors, but I don't know enough about it to make that call.
| asset.UnkPartNames[5] = rootCollision.Name; | ||
| } | ||
| /* Asset tileload config */ | ||
| else if (group.GetType() == typeof(HugeTile) || group.GetType() == typeof(BigTile)) |
There was a problem hiding this comment.
You should use this style instead of GetType and typeof.
else if (group is HugeTile || group is BigTile)
| asset.UnkPartNames[1] = rootCollision.Name; | ||
| asset.UnkPartNames[3] = rootCollision.Name; | ||
| asset.UnkPartNames[5] = rootCollision.Name; | ||
| if (group.IsInterior) |
There was a problem hiding this comment.
Why put this behind group.IsInterior? Exteriors need collision assigned.
There was a problem hiding this comment.
This was only done for interiors before so I kept that the same.
| int[] IdList(); | ||
|
|
||
| bool IsEmpty(); | ||
| bool IsInterior { get; } |
There was a problem hiding this comment.
You should make these Is_Condition_ consistent, either functions or property style.
Created interfaces for Tiles and InteriorGroups/Chunks to consolidate a lot of MSB generation code used by both. Related issue: #47
I tried to keep the behaviour identical to before and it seems to be working correctly.