Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reworked the Badge system, added badge for RecipePower #537

Merged
merged 7 commits into from
May 19, 2022

Conversation

RaymondBlaze
Copy link
Contributor

@RaymondBlaze RaymondBlaze commented Apr 28, 2022

Badges were almost completely hardcoded, so I reworked the whole system so it can work dynamically and accept more functions:

  1. Badge now work as a supplier of a list of TooltipComponent on the client side, providing fully customizable tooltip render
  2. Added registerable BadgeFactory to define different type of Badge
  3. BadgeManager now use BadgeFactory.Instance (basically work as a supplier of Badge) to store and pass Badge between server side and client side
  4. OriginDisplayScreen now hold a time field, so animated badge tooltips become possible (currently used for showing compound ingredients in RecipeBadge's tooltip)

Current implemented badges:

  1. Badge: a sprite without any tooltips
  2. TooltipBadge: have tooltips
  3. KeyBindingBadge: have tooltips which may show key binding names
  4. CraftingRecipeBadge: have tooltips, may show a 3x3 crafting recipe

Further work:

  1. Make a callback for auto badges so addons can register(Done)
  2. Implement a badge for ModifyCraftingPower
  3. More useful builtin badge types for datapacks

@apace100
Copy link
Owner

I have a couple of remarks regarding this PR.

  1. In previous datapacks, one could override automatically assigned badges (the key binding badges), like this:
{
  "badges": [
    {
      "sprite": "origins:textures/gui/badge/active.png",
      "text": "Active ability, use with %s."
    },
    {
      "sprite": "origins:textures/gui/badge/info.png",
      "text": "Cooldown: 15 seconds."
    }
  ]
}

Now, since before there was no distinction in badge types, all badges had the key binding associated with the power inserted into the formatting and replacing the %s in the text. This is no longer the case, and since the default badge type is tooltip and not key_binding, this overriding that worked before will result in a faulty key binding badge.
Therefore, in favor of backwards compatibility with existing datapacks, I suggest either a) automatically detecting when a power has a keybind associated, and if so, switching to origins:key_binding as the default factory for that power, or if that's not possible b) switching to origins:key_binding as the default factory for all powers, if no type is specified.

  1. Overriding a key binding badge with a key binding badge is possible independently of whether the key binding originates from the power the badge is on, or one of its sub-powers if it's a multiple. This is handled by the PowerKeyManager which traverses the sub-powers to look for an active power to find a key binding.
    With the crafting_recipe power type, you moved the handling of multiple powers into the auto badge registration. This means that the auto badges for crafting recipes work for multiple powers with recipe sub-powers, however adding additional badges manually (which means replacing the existing badge but adding a manual recipe badge in the array, like with the key binding badge above) does not trigger this "sub-power search" for recipes, causing it to fail and no badges to appear at all.

  2. With the factories of the key_binding and crafting_recipe powers theoretically supporting arbitrary keys and crafting recipes, it should be possible to define which key or recipe to display in the JSON. Right now, you're just filling those parameters from code.
    a) key_binding should allow specifying which key binding will be localized and fill the %s slot:

{
  "type": "origins:key_binding",
  "sprite": "origins:textures/gui/badge/active.png",
  "text": "Interact with an entity using %s to suck blood.",
  "key": "key.use"
}

b) recipe should allow specifying which recipe to display, preferably by providing either a full recipe as defined by SerializableDataTypes.RECIPE, or by providing an ID like in this example:

{
  "type": "origins:crafting_recipe",
  "sprite": "origins:textures/gui/badge/recipe.png",
  "text": "You probably know this, but this is how you craft ladders:",
  "recipe": "minecraft:ladder"
}
  1. If possible, instead of taking STRINGs as arguments for the text fields, try SerializableDataType.TEXT to allow datapackers to provide arbitrary text components. This would actually also solve the mentioned 3a, since a text component for keybinds (KeybindText class) exists.

  2. Rename Badge#getTooltipComponent to Badge#getTooltipComponents, since it returns a list.

  3. Rename the badge factory ID origins:key_binding to origins:keybind to be consistent with Minecraft's ID for the text component that translates keybinds. We should probably consider changing the class name to KeybindBadge then as well.

  4. This is a bigger suggestion than the previous few. This whole system of factories and objects seems like it would fit well into Calio's system of DataObjectRegistrys. Would you be willing to take a look into porting this to use that system instead? One benefit would be that Calio would automatically enable datapacks to have a badges folder where datapackers could define stand-alone badges in separate files and have a badge type which just refers to one of those badges. Would be useful if e.g. someone makes a badge for information on a specific status effect, and wants to attach it to all powers which mention that status effect in their description. Also, I plan to eventually port all "data registries" (origins, layers, but also powers, actions, and conditions in Apoli) to that DataObjectRegistry system. Feel free to contact me if you need any more information or help with this.

Thanks for your work on this PR, it's looking really good already and is a huge step towards making Origins both more extensible and user-friendly. :)

@RaymondBlaze
Copy link
Contributor Author

RaymondBlaze commented Apr 30, 2022

  1. I have changed the Badge system to use DataObjectRegistry instead, badges now can be referred using identifiers or defined by JSON objects in the power file, however, custom badges is no longer connected to the power and can't take default fields from the power(like recipe). Before powers are restricted to load after badges, it should be considered unstable to use badges from badges folder as the order is completely random now.
  2. TooltipBadge now accepts Text instead of String, so I removed the whole KeyBindingBadge class as Text can specify translation objects including keybinds. I didn't keep the backwards compatibility with the old text field which accepts String as there is no way I can tell whether a text field is legacy or not ;(
  3. Subpowers are now allowed to have own badges, including auto and custom ones, these badges will be automatically merge to the multiple power when the badge loading is finished.
  4. CraftingRecipeBadge now accpets recipe field for a SerializableDataTypes.RECIPE, but no support for SerializableDataTypes.IDENTIFIER to reference a existing recipe, as I didn't found a neat way to interact with the RecipeManager for this. It also optionally accpets a prefix field and a suffix field, which are both Texts, one showing before the recipe, the other showing after the recipe, the text field is removed though. What's more, if the player is using advance tooltip, then the badge will show the recipe's id at the last line of the tooltip.

@apace100 apace100 merged commit 36d44b3 into apace100:1.18 May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants