-
Notifications
You must be signed in to change notification settings - Fork 442
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
Dungeon Score improvements (on another branch) #490
base: 2.x-legacyfabricexp
Are you sure you want to change the base?
Conversation
* feat: Make all Dungeon solver colors customizable Not ready, but likely not going to break thing Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <[email protected]> * Maybe done * maybe a proper fix * Update TileEntityChestRendererHook.kt Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <[email protected]> * revert * fix import * Fix missing package ??????????????????????????????????????????????????????????? --------- Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <[email protected]> Co-authored-by: sychic <[email protected]>
# Conflicts: # mod/src/main/kotlin/gg/skytils/skytilsmod/features/impl/dungeons/DungeonChestProfit.kt # mod/src/main/kotlin/gg/skytils/skytilsmod/features/impl/dungeons/solvers/BoulderSolver.kt
* feat: Make all Dungeon solver colors customizable Not ready, but likely not going to break thing Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <[email protected]> * Maybe done * maybe a proper fix * Update TileEntityChestRendererHook.kt Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <[email protected]> * revert * fix import * Fix missing package ??????????????????????????????????????????????????????????? --------- Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <[email protected]> Co-authored-by: sychic <[email protected]>
eb88d6c
to
42207b2
Compare
i18nCategory = "skytils.config.dungeons", | ||
i18nSubcategory = "skytils.config.dungeons.score_calculation" | ||
type = PropertyType.SWITCH, name = "Show Dungeon Status", | ||
description = "Shows information about the current dungeon run that affects its score.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR removes "minimized score estimate", it might be useful to mention that this toggle is effectively the opposite of it. Maybe something along the lines of "Disabling this will minimize the dungeon score"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about specifying for a deprecated setting, but yeah the settings are the exact opposite, could maybe be convenient to flip them as "Hide Dungeon Status/Score Breakdown" instead. Also, the previous setting was disabled by default, maybe the new ones (if not changed from "Show") could be enabled to keep default functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I meant mostly for users who are used to the previous settings/names.
ScoreCalculationElement.text.add("• §eDeaths: ${if (deaths.get() == 0) { | ||
"§a" | ||
} else { | ||
"§c" | ||
}}${deaths.get()} ${if (firstDeathHadSpirit.get()) "§7(§6Spirit§7)" else ""}") | ||
ScoreCalculationElement.text.add("• §ePuzzles: ${if (completedPuzzleCount == puzzleCount) { | ||
"§a" | ||
} else if (failedPuzzles.get() > 0) { | ||
"§c" | ||
} else { | ||
"§e" | ||
}}$completedPuzzleCount§7/§a$puzzleCount") | ||
ScoreCalculationElement.text.add("• §eSecrets: " + if (totalSecrets.get() == 0) { | ||
"§7?" | ||
} else { | ||
val found = foundSecrets.get() | ||
(if (found == totalSecrets.get()) { | ||
"§a" | ||
} else if (found >= minSecrets.get()) { | ||
"§e" | ||
} else { | ||
"§c" | ||
}) + found + "§7/§a" + totalSecrets.get() + " §e(min: " + when (val sec = minSecrets.get()) { | ||
-1 -> "§7?" | ||
-2 -> "§c✘" | ||
-3 -> "§a✔" | ||
else -> "§a${(sec - found).takeUnless { it <= 0 } ?: "✔"}" | ||
} + "§e, could skip: " + when (val skip = skippableSecrets.get()) { | ||
-1 -> "§7?" | ||
-2 -> "§c✘" | ||
-3 -> "§a✔" | ||
else -> "§a${totalSecrets.get() - skip}" | ||
} + "§e)" | ||
}) | ||
ScoreCalculationElement.text.add("• §eCrypts: ${if (crypts.get() >= 5) { | ||
"§a" | ||
} else { | ||
"§c" | ||
}}${crypts.get().coerceAtMost(5)}") | ||
ScoreCalculationElement.text.add("• §eMimic: §l${if ((dungeonFloorNumber ?: 0) >= 6) { | ||
if (mimicKilled.get()) { | ||
"§a✔" | ||
} else { | ||
"§c✘" | ||
} | ||
} else { | ||
"§7-" | ||
}}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would make more sense to expand everything out into multiline string templates to avoid the awkward formatting here. In particular, the if/else statements being expanded while the rest of the string template is not feels weird and also the prevalence of using the same colors for a simple boolean which could maybe be replaced with a helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda find it awkward to have only half of the lines on the template, and moving puzzles and secrets outside of it, but it did improve readability a lot, probably worth settling with it either way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks better
mod/src/main/kotlin/gg/skytils/skytilsmod/features/impl/dungeons/ScoreCalculation.kt
Outdated
Show resolved
Hide resolved
oh huh i meant to request changes, not approve |
…gacyfabricexp # Conflicts: # mod/src/main/kotlin/gg/skytils/skytilsmod/Skytils.kt # mod/src/main/kotlin/gg/skytils/skytilsmod/features/impl/dungeons/DungeonChestProfit.kt # mod/src/main/kotlin/gg/skytils/skytilsmod/features/impl/dungeons/ScoreCalculation.kt
I'm not sure exactly what I just did but the commit amount doubled? Not sure if it was because of having approved it, but there's a bunch of duplicate commits now, hopefully not causing (a lot of) issues |
Signed-off-by: Desco1 <[email protected]>
c474e6b
to
d4e2616
Compare
Same thing as #489, except the score calc class was also migrated to the new event system.
Wanted to add a feature to mark Blaze Puzzle as complete for the score calc (and potentially catlas aswell) when the solver detected 10 blaze deaths (or when read from party chat) but figured that that may aswell wait until most players aren't on a version with a bugged "Blaze Done" party chat message.