Port Herblore plugin to Kotlin #396

ghost · 2018-04-06T15:16:50Z

review :)

garyttierney

Have some feedback but nothing major. This looks pretty good.

garyttierney · 2018-04-06T17:55:46Z

game/plugin/skills/herblore/src/potion.kt

+            stop()
+        }
+
+        if (player.inventory.remove(finished.unf_potion, 1) > 0) {


player.inventory.remove(id, amount) returns the number of items that was removed. If you only want to remove one, you can write player.inventory.remove(id) intead, which returns a boolean indicating if one value was removed/.

garyttierney · 2018-04-06T17:56:54Z

game/plugin/skills/herblore/src/potion.kt

+    MAGIC(id = 3042, unf_potion = UnfinishedPotion.LANTADYME, ingredient = NormalIngredient.POTATO_CACTUS.id, level = 76, experience = 172.5),
+    ZAMORAK_BREW(id = 189, unf_potion = UnfinishedPotion.TORSTOL, ingredient = NormalIngredient.JANGERBERRIES.id, level = 78, experience = 175.0);
+
+    val unf_potion = unf_potion.id


Only used internally. May as well inline this where it's used.

What do you exactly mean with this? All the other things have been fixed now!

Just means use the UnfinishedPotion as the value inside FinishedPotion, and call unf_potion.id (or unfinished.id, whatever it's called now). If you press Ctrl+Shift+N in IntelliJ it can do this for you.

garyttierney · 2018-04-06T17:57:14Z

game/plugin/skills/herblore/src/potion.kt

+
+    val unf_potion = unf_potion.id
+    companion object {
+        private val FINISHED_POTION = FinishedPotion.values().associateBy  { Pair(it.unf_potion, it.ingredient) }


i.e.

Pair(it.unfinishedPotion.id, it.ingredient)

What do you exactly mean with this? All the other things have been fixed now!

garyttierney · 2018-04-06T17:57:38Z

game/plugin/skills/herblore/src/potion.kt

+
+enum class FinishedPotion (
+        val id: Int,
+        unf_potion: UnfinishedPotion,


should be camelCase

garyttierney · 2018-04-06T18:00:13Z

game/plugin/skills/herblore/src/herblore.plugin.kts

+
+on { ItemOnItemMessage::class }
+        .where { id == PESTLE_MORTAR || id == KNIFE }
+        .where { targetId == PESTLE_MORTAR || targetId == KNIFE }


You can only apply where once, so you'll need to fold this into a single expression.

.where { (id == PESTLE_MORTAR || id == KNIFE) && (targetId == PESTLE_MORTAR || targetId == KNIFE) }

CC @Major- thoughts?

We'll write some shim code to improve this as part of #395. Fine for now.

garyttierney · 2018-04-06T18:01:01Z

game/plugin/skills/herblore/src/ingredient.kt

+
+class CrushIngredientAction (val player: Player, val ingredient: CrushIngredient) : AsyncAction<Player>(1, false, player) {
+    override fun action(): ActionBlock = {
+        if (player.inventory.remove(ingredient.id, 1) > 0) {


Same as other remove() calls below.

garyttierney · 2018-04-06T18:01:35Z

game/plugin/skills/herblore/src/potion.kt

+import java.util.*
+
+private val MIXING_ANIM = Animation(363)
+val WATER_VIAL = 227


Is this used outside of here? If not you can use const val

garyttierney · 2018-04-06T18:02:56Z

cc @Major- we should be distributing code style settings at this point.

garyttierney · 2018-04-06T18:20:19Z

@Chivvon there's also some code-style changes needed throughout - could you run your code through IntelliJ's auto formatter? The .editorconfig we ship should apply most changes.

tlf30 · 2018-04-06T20:10:33Z

Looking good, thank you for the contribution!

garyttierney · 2018-04-06T20:37:15Z

game/plugin/skills/herblore/src/herblore.plugin.kts

+import org.apollo.game.message.impl.ItemOnItemMessage
+
+on { ItemOptionMessage::class }
+        .where { option == 1 }


Could you reduce this to 4 spaces instead of 8? I think .editorconfig should be doing that (cc @Major- if it's not).

(and same below)

garyttierney · 2018-04-06T20:37:32Z

game/plugin/skills/herblore/src/herblore.plugin.kts

+        }
+
+on { ItemOnItemMessage::class }
+        . where { id == WATER_VIAL || targetId == WATER_VIAL }


Remove the extra whitespace before the dots here.

garyttierney · 2018-04-06T20:38:57Z

game/plugin/skills/herblore/src/potion.kt

+            stop()
+        }
+
+        if (player.inventory.remove(finished.unfinished.id)) {


Ah, just re-read the Inventory docs. You can shorten this to: player.inventory.remove(finished.unfinished.id, finished.ingredient).

garyttierney · 2018-04-06T20:39:18Z

game/plugin/skills/herblore/build.gradle

+plugin {
+    name = "herblore_skill"
+    authors = [
+            "Chivvon"


Same here with indentation (8 spaces to 4).

Major- · 2018-04-06T20:52:51Z

game/plugin/skills/herblore/src/potion.kt

+    }
+}
+
+class MakeUnFinishedPotionAction(val player: Player, val unfinished: UnfinishedPotion) : AsyncAction<Player>(1, false, player) {


MakeUnfinishedPotionAction please

Major- · 2018-04-06T20:53:12Z

game/plugin/skills/herblore/src/potion.kt

+        val level = mob.herblore.current
+
+        if (level < unfinished.level) {
+            player.sendMessage("You need an herblore level of ${unfinished.level} to make this unfinished potion.")


should be "a Herblore level"

Major- · 2018-04-06T20:54:37Z

game/plugin/skills/herblore/src/potion.kt

+        if (player.inventory.remove(WATER_VIAL)) {
+            if (player.inventory.remove(unfinished.herb)) {
+                player.inventory.add(unfinished.id)
+                player.sendMessage("You have successfully made the ${Definitions.item(unfinished.id)!!.name.toLowerCase()}.")


This message should be "You mix the ${Definitions.item(unfinished.id)!!.name.toLowerCase()}. into your potion."

Major- · 2018-04-06T20:55:29Z

game/plugin/skills/herblore/src/potion.kt

+        val level: Int,
+        val experience: Double
+) {
+    ATTACK(id = 121, unfinished = UnfinishedPotion.GUAM, ingredient = NormalIngredient.EYE_NEWT.id, level = 1, experience = 25.0),


Can you statically import all of the enumerators in UnfinishedPotion, NormalIngredient, and CrushIngredient

How would I do this if NormalIngredient and CrushedIngredient are using the val ingredient: Int?
Or did i just misunderstand what you ment?

you can do:

import UnfinishedPotion.*

and change

- unfinished = UnfinishedPotion.GUAM + unfinished = GUAM

Major- · 2018-04-06T20:56:44Z

game/plugin/skills/herblore/src/ingredient.kt

+import java.util.*
+
+private val GRINDING_ANIM = Animation(364)
+val PESTLE_MORTAR = 233


These ints can be const.

Major- · 2018-04-06T20:57:36Z

game/plugin/skills/herblore/src/identify.kt

+        val level = mob.herblore.current
+
+        if (level < herb.level) {
+            player.sendMessage("You need an herblore level of ${herb.level} to clean this herb.")


Should be a Herblore level

Major- · 2018-04-06T20:58:28Z

game/plugin/skills/herblore/src/identify.kt

+        if (player.inventory.removeSlot(slot, 1) > 0) {
+            player.inventory.add(herb.identified)
+            player.herblore.experience += herb.experience
+            player.sendMessage("You identify the herb as a ${Definitions.item(herb.identified)!!.name.toLowerCase()}.")


Can you add a // TODO handle indefinite article comment here please

Major- · 2018-04-06T21:01:43Z

game/plugin/skills/herblore/src/herblore.plugin.kts

+
+on { ItemOnItemMessage::class }
+        . then {
+            val finished = FinishedPotion[Pair(targetId, id)] ?: return@then


You need to check which way round the id and targetId should go here - it needs to be unfinished first, ingredient second, so you should check which is which.

Major- · 2018-04-06T21:02:32Z

game/plugin/skills/herblore/src/herblore.plugin.kts

+        }
+
+on { ItemOnItemMessage::class }
+        .where { (id == PESTLE_MORTAR || id == KNIFE) && (targetId == PESTLE_MORTAR || targetId == KNIFE) }


This check is wrong - will only pass if both id and targetId are a knife or a pestle & mortar.

Major- · 2018-04-06T21:03:16Z

game/plugin/skills/herblore/src/herblore.plugin.kts

+on { ItemOnItemMessage::class }
+        . where { id == WATER_VIAL || targetId == WATER_VIAL }
+        . then {
+            val unfinished = UnfinishedPotion[id] ?: return@then


Same thing with the on handler below, you need to make sure what you pass to UnfinishedPotion is actually the ingredient id (not the vial of water).

That is the way I intially wanted to write the code. I just didn't know how to do it with plugin format you guys provided, "the on/where"-format.

I'm still not sure how to do that logic in apollo with kotlin.

Just something like:

val unfinishedId = if (id == WATER_VIAL) { targetId } else { id } val unfinished = UnfinishedPotion[unfinishedId]!!

ghost closed this Apr 6, 2018

ghost deleted the herblore branch April 6, 2018 15:25

ghost restored the herblore branch April 6, 2018 15:31

garyttierney reopened this Apr 6, 2018

garyttierney changed the base branch from master to kotlin-experiments April 6, 2018 15:32

Major- changed the title ~~Herblore~~ Port Herblore plugin to Kotlin Apr 6, 2018

garyttierney added plugin ruby-to-kotlin Part of the port from ruby to kotlin labels Apr 6, 2018

garyttierney requested changes Apr 6, 2018

View reviewed changes

garyttierney reviewed Apr 6, 2018

View reviewed changes

Major- requested changes Apr 6, 2018

View reviewed changes

Port Herblore plugin to Kotlin

a528379

Major- approved these changes Apr 8, 2018

View reviewed changes

garyttierney approved these changes Apr 8, 2018

View reviewed changes

Major- merged commit a528379 into apollo-rsps:kotlin-experiments Apr 8, 2018

Major- added a commit that referenced this pull request Apr 8, 2018

Merge pull request #396 from chivvon/herblore

68b7891

ghost deleted the herblore branch May 4, 2018 13:15

ghost restored the herblore branch May 4, 2018 13:15

ghost deleted the herblore branch May 4, 2018 13:15

Port Herblore plugin to Kotlin #396

Port Herblore plugin to Kotlin #396

Conversation

ghost commented Apr 6, 2018

garyttierney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyttierney commented Apr 6, 2018

garyttierney commented Apr 6, 2018

tlf30 commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment