-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: マナが無くなったら掘れなくするパッシブスキルを追加 #2370
base: 1_18
Are you sure you want to change the base?
feat: マナが無くなったら掘れなくするパッシブスキルを追加 #2370
Conversation
14a859f
to
571abd3
Compare
@rito528 |
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.
develop
ブランチの変更を取り込んでしまっているようなので、 git rebase --rebase-merges 1_18
などを実行して develop
ブランチの変更をコミット履歴から消していただけると助かります🙇♂️🙇♂️
了解です。 |
bb09299
to
509644b
Compare
509644b
to
e7c7cd3
Compare
@rito528 |
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.
一旦2つ変更をお願いします
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/github/unchama/seichiassist/util/BreakUtil.scala
Outdated
Show resolved
Hide resolved
@rito528 |
...ssist/subsystems/breakskilltriggerconfig/application/actions/BreakSkillTriggerSettings.scala
Outdated
Show resolved
Hide resolved
...ssist/subsystems/breakskilltriggerconfig/application/actions/BreakSkillTriggerSettings.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/github/unchama/seichiassist/subsystems/mana/domain/ManaManipulation.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala
Outdated
Show resolved
Hide resolved
@rito528 |
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.
テーブル構成が変わることで今と実装がかなり変わりそうなので、一度以下の内容の変更をお願いします!
src/main/scala/com/github/unchama/seichiassist/subsystems/mana/domain/ManaManipulation.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala
Outdated
Show resolved
Hide resolved
src/main/resources/db/migration/V1.19.0__Create_break_trigger_preference_table.sql
Outdated
Show resolved
Hide resolved
@rito528 |
@@ -0,0 +1,28 @@ | |||
package com.github.unchama.seichiassist.subsystems.breakskilltriggerconfig.domain | |||
|
|||
case class BreakSkillTriggerConfig(config: Map[BreakSkillTriggerConfigKey, Boolean]) { |
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.
914bf47 の変更で、この subsystem は「マナがなくなった時に採掘機能を停止するか」を扱うと決めたので、この case class
では「設定が有効か無効か」を保持できれば十分だと思います。
なので
case class BreakSkillTriggerConfig(config: Map[BreakSkillTriggerConfigKey, Boolean]) { | |
case class BreakSuppressionPreference(doBreakSuppression: Boolean) { |
とすると良いと思います。(ついでにファイル名も BreakSuppressionPreference.scala
とすると良いと思います)
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.
BreakSkillTriggerConfig
に対して書いたレビューに沿った変更が行われると、BreakSkillTriggerConfigKey
の定義が不要になると思います。
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.
SQL 周りも現在のテーブルスキーマに合わせた実装に変更をお願いします!
def tryConsume(amount: ManaAmount)(manaMultiplier: ManaMultiplier): Option[ManaAmount] = { | ||
val resultingAmount = value | ||
Option.when(resultingAmount >= amount.multiply(manaMultiplier.value).value)( | ||
ManaAmount(resultingAmount) | ||
) | ||
} | ||
|
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.
tryConsume
関数を新たに定義する必要はないかもしれません。
この関数を使用しているのは canAcquire
関数ですが、canAcquire
関数は「任意の分だけマナを消費することができるか」を確認する関数なので、tryUse
関数を呼び出すことで必要な要件を満たせると思います
(tryUse
関数は消費したいマナに manaMultiplier
をかけ合わせた分を、現在のマナから引いて、負の値になるならば None
を返す関数なので)
dragonNightTimeMultiplierRef.get.flatMap { multiplier => | ||
ref.modify { original => | ||
original.tryConsume(amount)(multiplier) match { | ||
case Some(reduced) => (reduced, true) | ||
case None => (original, false) | ||
} | ||
} | ||
} |
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.
canAcquire
関数は「amount
だけマナを消費することが可能ならば true
、そうでないなら false
を返す作用」なので、ref.modify
を実行してしまうとまずいです(ref
の値を更新してしまっているので)
なので
dragonNightTimeMultiplierRef.get.flatMap { multiplier => | |
ref.modify { original => | |
original.tryConsume(amount)(multiplier) match { | |
case Some(reduced) => (reduced, true) | |
case None => (original, false) | |
} | |
} | |
} | |
for { | |
multiplier <- dragonNightTimeMultiplierRef.get | |
original <- ref.get | |
usedResult <- original.tryUse(amount)(multiplier) | |
} yield usedResult.isDefined |
とすると良いと思います。
def tryConsume( | ||
amount: ManaAmount | ||
)(manaMultiplier: ManaMultiplier): Option[LevelCappedManaAmount] = { | ||
manaAmount.tryConsume(amount)(manaMultiplier).map(LevelCappedManaAmount(_, level)) | ||
} | ||
|
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.
ManaAmount.scala
の tryConsume
関数に対して書いたレビューと同様なので理由は省略しますが、tryConsume
関数は必要ないと思います。
close #1964
このPRの変更点と理由:
主な変更点
補足情報: