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.

Sign up for GitHub

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

Jump to bottom

Feature: Armor Stack Decay Timer #2538

Open

Ovi-111 wants to merge 7 commits into hannibal002:beta from Ovi-111:armor-stack

+184 −33

Contributor

Ovi-111 commented Sep 18, 2024 •

edited by CalMWolfs

Loading

What

This adds a decay timer for armor stacks. When a stack reaches max stacks (10), text color changes for better experience. Decay timer can be toggled to show only for the 10th stack.

Images

Changelog New Features

Added a decay timer for armor stacks. - Ovi_1
- The timer can be set to show only for the 10th stack.
- Shows stack type when you gain stack (toggleable).

Changelog Improvements

The armor stack counter is hidden from the action bar when Armor Stack Display is enabled. - Ovi_1

Changelog Technical Details

Renamed StackDisplayConfig to ArmorStackDisplayConfig. - Ovi_1

Ovi added 6 commits

September 17, 2024 17:11


          Armor stack timer testing


          Armor Stack Timer & Hide Stacks from ActionBar

ed1baf4


          Reset when world change and config change

245e8f6


          Stack Type Display


          One line string render && config info change

ee6ad2c


          one line or multiline display

507670c

hannibal002 added this to the Version 0.28 milestone

jani270 added the Soon label


          Merge branch 'beta' into armor-stack

cdae1a9

CalMWolfs added Waiting on Dependency PR and removed Soon labels

Collaborator

jani270 commented Oct 10, 2024

I dont think the dependency is good here, the other PR has been dead for months, would be better to just review/merge this one with good changes.

Collaborator

CalMWolfs commented Oct 10, 2024

I dont think the dependency is good here, the other PR has been dead for months, would be better to just review/merge this one with good changes.

ill close the other one then

CalMWolfs mentioned this pull request

Feature: Armor Stack Timer #2200

Closed

CalMWolfs removed the Waiting on Dependency PR label

jani270 added the Soon label

hannibal002 requested changes

View reviewed changes

Owner

hannibal002 left a comment

Code looks mostly fine.
Feature works also cool in game.

But i could not get it to work with 3/4 aurora armor.
and with 3/4 crimson the timer was 1-2 seconds off: the display showed "0s" for 1-2 seconds until it finally removed one stack.

Also suggested some code cleanup and consistnecy changes

src/main/java/at/hannibal2/skyhanni/config/features/combat/CombatConfig.java

                   @Accordion
-                  // TODO rename to armor stack display
-                  public StackDisplayConfig stackDisplayConfig = new StackDisplayConfig();
+                  public ArmorStackDisplayConfig armorStackDisplayConfig = new ArmorStackDisplayConfig();

Owner

hannibal002 Oct 11, 2024

please remove the suffix "Config" from the variable name (not the class name)

src/main/java/at/hannibal2/skyhanni/config/features/combat/ArmorStackDisplayConfig.java

+              public class ArmorStackDisplayConfig {
+                  @Expose
+                  @ConfigOption(name = "Enable", desc = "Enable the armor stack display feature.")

Owner

hannibal002 Oct 11, 2024

Please explain a bit more what this feature does. (include crimson armor names maybe, or their ability names)

src/main/java/at/hannibal2/skyhanni/config/features/combat/ArmorStackDisplayConfig.java

+                  @ConfigOption(name = "Enable", desc = "Enable the armor stack display feature.")
+                  @ConfigEditorBoolean
+                  @FeatureToggle
+                  public boolean enabled = true;

Owner

hannibal002 Oct 11, 2024

please make this default diabled

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

+                  private fun drawDisplay(): List<String> {
+                      if (stackCount == 0 || armorPieceCount < 3) return emptyList()
+                      val displaySingleLine = config.showInSingleLine

Owner

hannibal002 Oct 11, 2024

the config variables here can all be removed/inlined (not the isMaxStack one)

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

                       if (!isEnabled()) return
-                      config.position.renderString(display, posLabel = "Armor Stack Display")
+                      display = drawDisplay()

Owner

hannibal002 Oct 11, 2024

please add a check so that this only happens every second tick (10 times a second, not 20 times a second). Can be done via event.isMod(2)

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

+                          else -> "§b"
+                      }
+                      if (displaySingleLine) {

Owner

hannibal002 Oct 11, 2024

you can move the two returns below to this if block directly

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

+                  @SubscribeEvent
+                  fun onWorldChange(event: LorenzWorldChangeEvent) {
+                      if (!isEnabled()) return

Owner

hannibal002 Oct 11, 2024

the check here can go

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

+                  @SubscribeEvent
+                  fun onPlaySound(event: PlaySoundEvent) {
+                      if (!isEnabled() && !config.armorStackDecayTimer) return

Owner

hannibal002 Oct 11, 2024

i think the AND here is wrong, a OR makes more sense to me

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

+                  @SubscribeEvent
+                  fun onPlaySound(event: PlaySoundEvent) {
+                      if (!isEnabled() && !config.armorStackDecayTimer) return
+                      if (event.soundName in listOf("tile.piston.out", "tile.piston.in") &&

Owner

hannibal002 Oct 11, 2024

please move the list into a member of the class so that we dont need to recreate it each time

Contributor

ItsEmpa Oct 11, 2024

additionally, a set is probably better than a list here?

src/main/java/at/hannibal2/skyhanni/features/combat/ArmorStackDisplay.kt

+                  private fun resetDecayTime() {
+                      armorPieceCount = InventoryUtils.getArmor().firstNotNullOfOrNull { armor ->
+                          val lore = armor?.getLore().toString()

Owner

hannibal002 Oct 11, 2024

I dont like the getLore().toString() workaround here. i think we have another matcher utils function that works with a list and uses the first/last directly

github-actions bot added the Wrong Title/Changelog label

github-actions bot commented Oct 11, 2024

I have detected some issues with your pull request:

Body issues:
No changes detected in this pull request

Please fix these issues. For the correct format, refer to the pull request template.

github-actions bot removed the Wrong Title/Changelog label

hannibal002 removed the Soon label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet