Add Support For Multiple Banners

[HassanAbouelela]

This PR adds support for having multiple banners which rotate in a single event, much like icons currently do. See python-discord/branding#179 for the relevant changes there. They should be merged around the same time (preferably branding first).

There are almost no code changes here as most of the required logic was already implemented, and was standardized to work for both icons and banners. A majority of the diff is renaming "icon" to "asset" and adding an asset type argument to the functions.

For testing purposes, make sure to create an API token as you'll hit the unauthorized request cap almost instantly. You can use my branding fork for testing as well, it has 2 banners under the fallback event:

https://api.github.com/repos/HassanAbouelela/pydis-branding/contents

[HassanAbouelela]

Kwzrd suggested we remove support for standalone banners to simplify the code here, thanks for the idea!

[wookie184]

Is it important that we are able to link a banner and icon so they always appear at the same time? I'd guess they probably would end up in sync if they are in the same order in the banners/ and icons/ directories in the branding repo, but is there a way of making this more explicit?

[HassanAbouelela]

@wookie184 I considered it, but after deliberating on the matter for a while, I didn't think the added complexity of trying to coordinate both was worth the payoff, especially considering the tight deadline we're trying to finish this in.

It can be added in the future, I imagine we can link them based off their name, but more logic would be necessary to properly handle rotating them at the same time (and certain edge cases such as having 2 banners, and 1 icon).

[wookie184]

Tested and all seems good to go.

Was trying to think of a good way of handling matching banners and icons. Had one idea for a layout:

icons
  - icon_green.png
  - icon_red.png
banners
  - banner_green.png
  - banner_red.png

To get the assets to use, we'd select a random icon, and then select a random banner in the same group (group is what's after the _, or something like that). This would be pretty flexible as you can easily have one icon with many banners or vice versa.

icons
  - icon1_green.png
  - icon2_green.png
banners
  - banner_green.png

And files with no "group" would just be considered part of the default group, so e.g. icon.png would match to banner.png.

[HassanAbouelela]

@wookie184 That's a pretty good system provided you don't try to deal with combinations of groups, and no groups, such as:

icons:
icon_green.png
icon_red.png

banners:
banner_green.png
banner_red.png
banner_other.png

In this case, you have the following problem:

Day 1: Green icon, green banner
Day 2: Red icon, red banner
Day 3: Other banner, ?? icon.

You end up either having to repeat an icon two days in a row, cause a mismatch in the banners and icons, or smartly pick an icon that is not in the upcoming group. This complication is why I chose not to bother with support for this in this PR.

[wookie184]

I don't think there would be any need to support groups with only banners or only icons, that's something that could be validated in the branding repo to make sure it doesn't happen,

[HassanAbouelela]

I don’t agree. We already have events that don’t have banners, because not every contributor will make one. That would artificially limit us. Additionally, with your proposed system where you can have multiple assets in each “group”, you run into the same problems.

[wookie184]

We already have events that don’t have banners

Do we? I couldn't find any, and it looks like we already enforce events having to have a banner https://github.com/python-discord/branding/blob/3173b230d8e2b0382a0cb19f0966a7100ad88910/events/validation.py#L68-L73

[HassanAbouelela]

We have events that don’t have as many banners as icons rather. This is the case with all events that have more than one icon currently, because nothing else is supported yet.

There were also considerations for not making a banner to go along with an event icon in dev-branding recently.

[wookie184]

We have events that don’t have as many banners as icons rather. This is the case with all events that have more than one icon currently, because nothing else is supported yet.

That wouldn't be an issue with what I am proposing. For example, if you had 2 icons and 1 banner, you'd just put them all in a single group. A random icon would be chosen, and a random banner from the corresponding group is chosen (in this case always the same one).

[HassanAbouelela]

No the problem is with what I mentioned about having unbalanced groups.

You end up either having to repeat an icon two days in a row, cause a mismatch in the banners and icons, or smartly pick an icon that is not in the upcoming group.

Ultimately it is solvable. Repeating an asset is one such solution. Which solution is deemed best is probably best left to the actual PR implementing that.

[MarkKoz]

The code looks fine but I'm not sure how to test it with an actual event. Works fine with the default event though.