Add damage events #2

e-aakash · 2019-05-22T09:27:25Z

Add damage events and update HealthAuthoritySystem.

Add author in module.txt. Add javadoc for public methods in HealthAuthoritySystem. Renamed FullHealthEvent as OnFullyHealedEvent.

syntaxi

Tried to leave overall comments onto the code regardless of if you have written it.
Only minor things like simplifying and splitting code (reducing duplicates)

syntaxi · 2019-05-23T04:05:16Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+import org.terasology.logic.characters.events.AttackEvent;
+import org.terasology.logic.characters.events.HorizontalCollisionEvent;
+import org.terasology.logic.characters.events.VerticalCollisionEvent;
+import org.terasology.logic.health.event.*;


Avoid star imports.
If IntelliJ is auto collapsing them then you can change the settings to increase the number of classes before it collapses to some high number like 999

syntaxi · 2019-05-23T04:08:34Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+        damageEntity(event, targetEntity);
+    }
+
+    static void damageEntity(AttackEvent event, EntityRef targetEntity) {


Why is this a separate static method to the above?

I couldn't find any other methods using damageEntity method. So will move it into the onAttackEntity method.

I did not check in the original Core/Health directory for usage. Turns out BlockDamageAuthoritySystem uses this method. See here
So I will have to either revert the commit or add it back in the next commit.

syntaxi · 2019-05-23T04:18:40Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+            ghost = (characterMovementComponent.mode == MovementMode.GHOSTING);
+        }
+        if ((health != null) && !ghost) {
+            int damagedAmount = health.currentHealth - Math.max(health.currentHealth - damageAmount, 0);


This is overly complicated and the name is ambiguous.
Can't you replace this line with

int damagedAmount = Math.min(health.currentHealth, damageAmount)

Further more based on that it makes sense to use maxDamage or cappedDamage as the name, eliminating the confusion between damageAmount and damagedAmount

This also applies to a similar section of the doHeal method

syntaxi · 2019-05-23T04:23:02Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+     */
+    @ReceiveEvent(components = {HealthComponent.class})
+    public void onLand(VerticalCollisionEvent event, EntityRef entity) {
+        HealthComponent health = entity.getComponent(HealthComponent.class);


If you are going to be using the component, just place it into the constructor to simplify things

ie,

@ReceiveEvent public void onLand(VerticalCollisionEvent event, EntityRef entity, HealthComponent health) { /* ... */ }

syntaxi · 2019-05-23T04:24:42Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+        vel.y = 0;
+        float speed = vel.length();
+
+        if (speed > health.horizontalDamageSpeedThreshold) {


Parts of this method and the onLand method could likely be extracted into a single highSpeedDamage method

syntaxi · 2019-05-23T04:28:37Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+
+        if (velocity > healthComponent.horizontalDamageSpeedThreshold) {
+            if (characterSounds.lastSoundTime + CharacterSoundSystem.MIN_TIME < time.getGameTimeInMs()) {
+                StaticSound sound = random.nextItem(characterSounds.landingSounds);


Similarly, there are a few segments of code where sounds are played, which could be extracted into a common private method

Though this can be extracted, the onCrash only plays sound on the client (by sending PlaySoundEvent), but damage plays sound on both client and server.
So should the client sound way be extracted to a private method?

Cervator

Good stuff! Throwing in another round of fairly minor comments 👍

Cervator · 2019-05-23T04:36:41Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+import org.terasology.logic.characters.events.AttackEvent;
+import org.terasology.logic.characters.events.HorizontalCollisionEvent;
+import org.terasology.logic.characters.events.VerticalCollisionEvent;
+import org.terasology.logic.health.event.*;


Avoid wildcard import - configure IntelliJ (or whatever) to no collapse into wildcard, or at least not until 9999 imports or something crazy :-)

Cervator · 2019-05-23T04:37:36Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

@@ -45,7 +58,18 @@
 * - BeforeHealEvent
 * - (HealthComponent saved)
 * - OnHealedEvent
- * - FullHealthEvent (if at full health)
+ * - OnFullyHealedEvent (if at full health)


Maybe

Suggested change

* - OnFullyHealedEvent (if at full health)

* - OnFullyHealedEvent (if healed to full)

Cervator · 2019-05-23T04:38:57Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

@@ -140,16 +165,199 @@ private int calculateTotal(int base, TFloatList multipliers, TIntList modifiers)

    }

-    /** Handles DoHeal event to increase health of entity */
+    /**
+     * Handles DoHeal event to increase health of entity.


Maybe tweak this with a code tag for DoHealEvent or even just avoid using the class name, instead referring to it generally "Event handler for increasing health ..."

Cervator · 2019-05-23T04:40:24Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+        damageEntity(event, targetEntity);
+    }
+
+    static void damageEntity(AttackEvent event, EntityRef targetEntity) {


Missing access level? Also it might be worth throwing some bits of javadoc on some of these utility methods, even if not public :)

Cervator · 2019-05-23T04:42:49Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+     * @param event VerticalCollisionEvent sent when falling speed threshold is crossed.
+     * @param entity The entity which is damaged due to falling.
+     */
+    @ReceiveEvent(components = {HealthComponent.class})


Since the HealthComponent is used inside this event handler you should be able to move it from the @ReceiveEvent filter into a method parameter. That way you don't need to separately retrieve it 👍

Cervator · 2019-05-23T04:43:23Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+    }
+
+    /**
+     * Inflicts damage to entity horizontalDamageSpeedThreshold is breached.


Think there's an "if" missing here somewhere

Cervator · 2019-05-23T04:45:52Z

src/main/java/org/terasology/logic/health/event/DamageSoundComponent.java

+
+import java.util.List;
+
+/**


Blank javadoc - leftovers from when we did a bulk remove of @author tags ages ago. Feel free to remove them (or better yet fill them in!) wherever you see them

Cervator · 2019-05-23T04:47:34Z

src/main/java/org/terasology/logic/health/event/HealthChangedEvent.java

+     * Method to get the amount by which health has changed.
+     *
+     * @return The amount by which health changed. This is capped (by the implementing method),
+     *         so if the entity received 9999 damage and only had 10 health, it will be -10.


It will be -10 or it will have 10 subtracted from it?

Cervator · 2019-05-23T04:47:52Z

src/test/java/org/terasology/logic/health/DamageEventTest.java

+import org.slf4j.LoggerFactory;
+import org.terasology.entitySystem.entity.EntityManager;
+import org.terasology.entitySystem.entity.EntityRef;
+import org.terasology.logic.health.event.*;


More wildcard :-)

Cervator · 2019-05-23T04:48:33Z

src/test/java/org/terasology/logic/health/DamageEventTest.java

+                    event.add(5);
+                    event.multiply(2);
+                });
+        // Expected damage value is ( initial:10 + 5 ) * 2 = 30


Might want to keep an eye out for = vs ==

I think this line is just calculation, so = should work.
Will keep an eye out for them

It is just math, true - I tend to always use the code style setup when it is in code even if it would be legit otherwise ... 😁

Cervator

Looks pretty solid 👍

Cervator · 2019-05-24T04:06:14Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

@@ -268,7 +276,7 @@ private void checkDamage(EntityRef entity, int amount, Prefab damageType, Entity
     * @param characterSounds Component having sound settings.
     */
    @ReceiveEvent
-    public void onDamaged(OnDamagedEvent event, EntityRef entity, CharacterSoundComponent characterSounds) {
+    public void onDamaged(OnDamagedEvent event, EntityRef entity, CharacterSoundComponent characterSounds)  {


Super trivial but this ended up with two spaces instead of one for some reason

Cervator · 2019-05-24T04:07:00Z

src/main/java/org/terasology/logic/health/HealthAuthoritySystem.java

+        highSpeedDamage(speed, entity, health.horizontalDamageSpeedThreshold, health.excessSpeedDamageMultiplier);
+    }
+
+    private void highSpeedDamage(float speed, EntityRef entity, float threshold, float damageMultiplier) {


Nice utility method get! 👍

Cervator

Just a few more comments

Cervator · 2019-05-24T04:32:40Z

src/test/java/org/terasology/logic/health/HealEventTest.java

 import org.terasology.entitySystem.entity.EntityManager;
 import org.terasology.entitySystem.entity.EntityRef;
 import org.terasology.logic.health.event.BeforeHealEvent;
+import org.terasology.logic.health.event.DoDamageEvent;


I think this is unused now

Cervator · 2019-05-24T04:33:07Z

src/test/java/org/terasology/logic/health/HealEventTest.java

@@ -38,7 +39,6 @@
 public class HealEventTest extends ModuleTestingEnvironment {

    private EntityManager entityManager;
-    private Time time;

    private static final Logger logger = LoggerFactory.getLogger(HealEventTest.class);


And this line could be moved above the entityManager to resolve a variable order issue

Cervator · 2019-05-24T04:47:07Z

src/test/java/org/terasology/logic/health/HealEventTest.java

        player.send(new DoHealEvent(10));
        assertEquals(30, player.getComponent(HealthComponent.class).currentHealth);
    }

+    @Test
+    public void healNegativeTest() {


Uh oh! This test actually fails for me! Expected 40, got 50.

Not worried about it though, merging anyway :-)

e-aakash added 4 commits May 21, 2019 23:52

Add damage events and update HealthAuthoritySystem

23d00bd

Add HealthCommands

27a88bc

Review changes

19bda11

Add author in module.txt. Add javadoc for public methods in HealthAuthoritySystem. Renamed FullHealthEvent as OnFullyHealedEvent.

Add tests for damage events

64b5329

syntaxi reviewed May 23, 2019

View reviewed changes

Cervator requested changes May 23, 2019

View reviewed changes

e-aakash added 3 commits May 23, 2019 11:21

Update JavaDoc and style changes

2fa7d51

Code refactor

af1725f

Add damageEntity method as helper for BlockDamageAuthoritySystem

63d6475

Cervator approved these changes May 24, 2019

View reviewed changes

Cervator merged commit 63d6475 into Terasology:master May 24, 2019

e-aakash deleted the damage branch June 1, 2019 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add damage events #2

Add damage events #2

e-aakash commented May 22, 2019

syntaxi left a comment

syntaxi May 23, 2019

syntaxi May 23, 2019

e-aakash May 23, 2019

e-aakash May 24, 2019

syntaxi May 23, 2019

syntaxi May 23, 2019

syntaxi May 23, 2019

syntaxi May 23, 2019

syntaxi May 23, 2019

e-aakash May 23, 2019

Cervator left a comment

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

Cervator May 23, 2019

e-aakash May 23, 2019

Cervator May 24, 2019

Cervator left a comment

Cervator May 24, 2019

Cervator May 24, 2019

Cervator left a comment

Cervator May 24, 2019

Cervator May 24, 2019

Cervator May 24, 2019

	* - OnFullyHealedEvent (if at full health)
	* - OnFullyHealedEvent (if healed to full)

Add damage events #2

Add damage events #2

Conversation

e-aakash commented May 22, 2019

syntaxi 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

Cervator 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

Choose a reason for hiding this comment

Cervator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cervator 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