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

Dungeon Score improvements (on another branch) #490

Open
wants to merge 34 commits into
base: 2.x-legacyfabricexp
Choose a base branch
from

Conversation

Desco1
Copy link
Contributor

@Desco1 Desco1 commented Jul 15, 2024

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.

Desco1 and others added 25 commits June 11, 2024 12:34
* feat: Make all Dungeon solver colors customizable

Not ready, but likely not going to break thing

Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <157546848+Coccocoahelper@users.noreply.github.com>

* Maybe done

* maybe a proper fix

* Update TileEntityChestRendererHook.kt

Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <157546848+Coccocoahelper@users.noreply.github.com>

* revert

* fix import

* Fix missing package

???????????????????????????????????????????????????????????

---------

Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <157546848+Coccocoahelper@users.noreply.github.com>
Co-authored-by: sychic <47618543+Sychic@users.noreply.github.com>
# 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) <157546848+Coccocoahelper@users.noreply.github.com>

* Maybe done

* maybe a proper fix

* Update TileEntityChestRendererHook.kt

Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <157546848+Coccocoahelper@users.noreply.github.com>

* revert

* fix import

* Fix missing package

???????????????????????????????????????????????????????????

---------

Signed-off-by: thatonecoder (formerly Coccocoa's Helper) <157546848+Coccocoahelper@users.noreply.github.com>
Co-authored-by: sychic <47618543+Sychic@users.noreply.github.com>
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.",
Copy link
Member

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"

Copy link
Contributor Author

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?

Copy link
Member

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.

Comment on lines 351 to 398
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-"
}}")
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that looks better

@Sychic
Copy link
Member

Sychic commented Jul 18, 2024

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
@Desco1
Copy link
Contributor Author

Desco1 commented Jul 22, 2024

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

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.

3 participants