Fix: More tweaks to spawns/loot with regards to subgroups.

[RyanYappert]

The arguments for various InstanceAssetManager functions were ambiguous as to whether they were using subgroupId or positionId. Some of this has to do with differing data structures; for enemies, each spot (combination of stage layout ID and index) represents a single enemy. For items (both drops and gathering spots), each spot represents a list of potential items. Untangling this is a headache, so for now this fix should let things drop as they should, regardless of what subgroup they fall on, provided that subgroups never overlap in positionId.

Checklist:

• The project compiles
• The PR targets develop branch

[pacampbell]

If there are arguments which are always 0, should we just remove those arguments?

[RyanYappert]

Maybe I'm confused over the structure of the assets here. I'll probably have to go back to the drawing board and fix this, but these are my issues/confusions.

• Enemies and items are different in dimensionality and the asset managers don't address this. InstanceEnemyManager is and should be a InstanceAssetManager<byte, Enemy, InstancedEnemy>, but InstanceItemManager really should be a InstanceAssetManager<uint, List<GatheringItem>, List<InstancedGatheringItem>> because items come in collections.
  • Alternatively, these could be some wrapper class of the List<GatheringItem> like a GatheringItemCollection.
• I should be able to reference a unique object with a (StageId, foo) pair. For GatheringItems (wood, plants, treasure, etc.), this is (StageId, PosId) according to InstanceGetGatheringItemListHandler. In this case, the object itself has no representation, and is represented solely by the list of items that it contains. Thus, a mapping from (StageId, PosId) => List<GatheringItem>
  • The natural corollary for enemies is (StageId, PositionIndex) => Enemy, because enemies have their own complicated representation.
  • (StageId, Subgroup) does not return a unique object, it returns a set of objects.
  • If you want a (sub)set of enemies, you should get the whole list and filter it yourself. This means that InstanceAssetManager needs a public T3 GetAssets(StageId stageId) function that enemies would use, and a public List<T3> GetAssets(StageId stageId, T1 setId) that items would use.

[RyanYappert]

I also realize now that I was pushing to the wrong repository, but I don't want to remove the conversation here by closing this PR prematurely.

[alborrajo]

I'm not sure this fixes issues, or makes things clearer, but i'll let others be the judge

[alborrajo]

In the future the position index will be something we could set explicitly, i don't think it should be removed from here (#463)

[pacampbell]

Why did you rename subgroup to sub?

[RyanYappert]

Because it need not be a sub-group? The second argument denotes one TAssetItem, which may be a group of smaller objects, but not necessarily. I suppose we can argue the semantics of "ID within/sub the group that is identified by the stageLayoutId", but I specifically don't want this to be confused with spawn subgroups as the client requests them, because that's not how InstanceAssetManager should be structured.

It shouldn't be structured around spawn subgroups because even though the client requests enemy lists in that fashion, that's not how any other enemy-related data structure thinks about enemies (or anything else that might be instanced, and these all share an interface so they should think about similar data structures). In every other context, it thinks about them as a stageLayoutId and an index, which we variously call setId, PositionIndex, and PosId.

Once the enemy is dead and we need its loot, we only have access to its stageLayoutId and that index, not what subgroup it came from. Gathering items are also not structured in subgroups. So it seems silly to me to organize the instanceAssetManager around subgroups when you can just do the filtering at the one point that actually does care about subgroups:

var instancedEnemyList = client.Party.InstanceEnemyManager.GetAssets(stageId).Where(x => x.Subgroup == subGroupId);

[alborrajo]

I see your point, yeah, we should write down this reasoning somewhere, could you add an explanation to the comments?

[pacampbell]

I imagine we will revisit again but I can't find anything that sticks out at the moment.