Add support for assigning multiple item classifications

[Seatori]

Adds support for users assigning multiple item classifications to a single item.

This is achieved by taking the base filler flag and appending each of the assigned classifications to it, making use of how Manual already 'supported' assigning multiple classifications to an item, but this adjusts the behavior so that they're no longer mutually exclusive.

Tested by assigning all three of the "Progression", "Useful", and "Trap" flags to a single item and then viewing that it displayed with all three in the client. Also verified that Useful + Trap items were correctly removed from the pool when the number of items exceeded the number of locations.

[Seatori]

As of recent, assigning multiple classifications to items has been added to a few of Archipelago's supported games, and it's considered to be supported behavior. Manual technically already allows multiple classifications to be assigned to an item in its .json files, but it overwrites them in sequence. This attempts to adjust the behavior so that it treats them as additive instead, in the interest of greater user freedom.

[FuzzyGamesOn]

As of recent, assigning multiple classifications to items has been added to a few of Archipelago's supported games, and it's considered to be supported behavior. Manual technically already allows multiple classifications to be assigned to an item in its .json files, but it overwrites them in sequence. This attempts to adjust the behavior so that it treats them as additive instead, in the interest of greater user freedom.

This is correct and makes sense, but is not an automatic inclusion for us. Going to mention something that we often consider for the direction of Manual:

AP supporting new behavior doesn't necessarily indicate new behavior that we want to support, because Manual prioritizes being easy to use and simple more than it does supporting AP's capabilities fully. This often comes down to weighing the value that the new behavior provides versus the complexity or confusion that it may introduce. It's fine for us to say that we don't support something if it doesn't provide enough value to our project or our users, and also fine to point the users that want it to hooks.

That being said, I can see this being useful for users. I also think keeping it out of Builder for now would avoid any confusion that newer users might have. This feels like more of a power user capability, anyways.

(Oh, and you'll want to make sure the changes account for existing uses of ItemClassification or an item's classification. DataValidation.py has an instance that would need updating, not sure if any others.)

[Seatori]

AP supporting new behavior doesn't necessarily indicate new behavior that we want to support, because Manual prioritizes being easy to use and simple more than it does supporting AP's capabilities fully. This often comes down to weighing the value that the new behavior provides versus the complexity or confusion that it may introduce. It's fine for us to say that we don't support something if it doesn't provide enough value to our project or our users, and also fine to point the users that want it to hooks.

That being said, I can see this being useful for users. I also think keeping it out of Builder for now would avoid any confusion that newer users might have. This feels like more of a power user capability, anyways.

I can respect not wanting to add every little thing. I just think that in this case, since Manual already seems so close to accepting this capability from a user standpoint, it would be a fairly harmless addition to allow users to do it. The corresponding hook method is also very simple, but that requires users to be told "you can do it using hooks" which may be an additional barrier of entry for them; one that I personally don't think is warranted given the simplicity of it.

That said, I do agree with not putting it on the Builder for now. A completely new user entering and seeing they can mix and match classifications may be an additional barrier of entry for Manual itself, especially if they're not already familiar with the concept. I believe that, as it becomes more widespread, there might eventually be a point where its inclusion would be warranted; but for now I think it makes sense as something where users can ask if it's possible and be told "yes, here's the extremely simple method of it".

(Oh, and you'll want to make sure the changes account for existing uses of ItemClassification or an item's classification. DataValidation.py has an instance that would need updating, not sure if any others.)

Yes, I had thought of that too but I wasn't able to look into it at the time. I'll look through it while I'm looking into your other requested change!

[Seatori]

I did a little more testing and found that filler_traps adds any item flagged as a trap to the pool, and... apparently this behavior isn't a result of my change? It seems like it's based on what the tags are in the .json files before they're filtered out when the items are created, meaning you can already flag items as both trap and useful or progression and they'll always be added to the pool multiple times if filler traps are enabled.

Is this the intended behavior? If not, would this be the desired behavior in the case of this change being added?

[FuzzyGamesOn]

I did a little more testing and found that filler_traps adds any item flagged as a trap to the pool, and... apparently this behavior isn't a result of my change? It seems like it's based on what the tags are in the .json files before they're filtered out when the items are created, meaning you can already flag items as both trap and useful or progression and they'll always be added to the pool multiple times if filler traps are enabled.

Is this the intended behavior? If not, would this be the desired behavior in the case of this change being added?

This is a bug, yeah. The only thing that should use the raw JSON classification is create_item(), everything else should use the classification of the items in the pool by that time. In fact, that traps list and traps.append in create_items isn't needed at all; adjust_filler_items() gets a list of traps from the item pool in the else, but not in the if for some reason. So it should just do that for both (i.e., above the if else).

[Seatori]

[...] In fact, that traps list and traps.append in create_items isn't needed at all; adjust_filler_items() gets a list of traps from the item pool in the else, but not in the if for some reason. So it should just do that for both (i.e., above the if else).

It looks to me like the if in adjust_filler_items() is supposed to be adding traps to the pool, hence the create_item call, while the else is for removing the traps that already exist in the pool, which is why that one searches for the items in the pool. Calling the list of items already in the pool doesn't seem like it would do much at all when it comes to adding them, and if the function was switched to only adding duplicates of the items that already exist in the pool, that would remove the current functionality of allowing certain items to only exist if filler traps are enabled.

[FuzzyGamesOn]

It looks to me like the if in adjust_filler_items() is supposed to be adding traps to the pool, hence the create_item call, while the else is for removing the traps that already exist in the pool...

That's a good point. Only need to make sure that your changes continue to work with what's there, then. Not super concerned about people adding prog traps or something, since they're still traps and should be treated like traps. For our purposes, it probably makes more sense for us to treat traps (and other classifications) uniformly regardless of any additional classifications on the trap. At least to start.

(Also, whenever your PR is complete, remember to hit the "Ready for review" button. Then we'll get other folks to review as well. 😃 )

[nicopop]

this is really good!

[silasary]

Code looks good to me. I cannot test it right now, so I'll trust someone else to actually execute it :)

[nicopop]

Code looks good to me. I cannot test it right now, so I'll trust someone else to actually execute it :)

I tried it, it does work

the datavalidation modification will almost definetly cause a merge conflict with #104
we'll see which get merged first the other will need a conflict fix

[FuzzyGamesOn]

Thanks! Looks good 😃