Add HealthComponent #1

e-aakash · 2019-05-16T17:52:35Z

For now, just moved the HealthComponent of Logic/Health to the new module.

Cervator

Throwing in some general comments hoping bits of it might be useful :-)

Another suggestion: Make a paired draft PR for the engine that removes anything this PR adds to the Health module, then update it over time. No intent to merge the engine PR for some time, but it might be good for visibility and being prepared. Then finally when Health is ready switch the engine PR out of draft mode (and/or recreate it fresh depending on how stale that branch went)

Cervator · 2019-05-17T05:02:50Z

src/main/java/org/terasology/HealthComponent.java

+import org.terasology.network.Replicate;
+import org.terasology.rendering.nui.properties.TextField;
+
+public class HealthComponent implements Component {


Well-written class level javadoc can be useful in very central components like this. Think long term: what might somebody like to see when they're fishing around for relevant components and run into Health?

Cervator · 2019-05-17T05:04:01Z

src/main/java/org/terasology/HealthComponent.java

+    @TextField
+    public int currentHealth = 20;
+
+    // Regen info


Prefer one-liner javadoc comments IMHO and document each field you think can be meaningfully enhanced. Example:

`/** Next tick time that will trigger regeneration */

Cervator · 2019-05-17T05:05:49Z

src/main/java/org/terasology/HealthComponent.java

+
+    public boolean destroyEntityOnNoHealth;
+
+    public HealthComponent() {


As a practice it may be wise to leave a comment on the no-arg constructor to highlight the reason it is here: if it isn't and there is an arg-constructor then the "invisible" no-arg constructor disappears. That then breaks stuff as some of the magic relies on there always being a no-arg constructor :-)

Cervator · 2019-05-17T05:07:48Z

src/main/java/org/terasology/HealthComponent.java

+    public HealthComponent() {
+    }
+
+    public HealthComponent(int maxHealth, float regenRate, float waitBeforeRegen) {


I figure this constructor is here because it already existed - I wonder how frequently it is used and if it is really justified for it to exist? It might be a good practice for us to always encourage the pattern of setting whichever exact fields you need rather than use a custom constructor

HealthComponent health; health.maxHealth = 123; health.regenRate = 42; //apply the component to something

After searching in the mega workspace, I couldn't find any usage of this constructor. I am hence removing this constructor.
And, since the unique constructor is removed, the default constructor is not needed explicitly.

iaronaraujo · 2019-05-20T13:02:56Z

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

@@ -0,0 +1,70 @@
+/*


I am not sure if this test should be here or in the MTE module.

Cervator

Left some more comments but mostly trivial / future thoughts so not going to block merging from it. Will push to merge without a merge commit so you can continue in your next branch without any updates needed. Can address any actionable review comments in a new commit during that effort :-)

Nice tests! They actually work for me for some reason, without hacking my workspace. I don't understand, but ... ¯_(ツ)_/¯

Haven't tested in-game as I don't know if that's ready yet.

Cervator · 2019-05-22T02:43:01Z

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

+    private EntityManager entityManager;
+
+    @In
+    private org.terasology.engine.Time time;


Any reason for having this fully qualified?

Cervator · 2019-05-22T02:43:20Z

module.txt

@@ -4,6 +4,6 @@
    "author" : "",


Feel free to toss an author name in there :-)

Cervator · 2019-05-22T03:06:53Z

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

+
+    }
+
+    /** Handles DoHeal event to increase health of entity */


One-liner javadoc is good for simple fields, but methods should always use the expanded version including doc for parameters etc :-)

Cervator · 2019-05-22T03:15:02Z

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

+import org.terasology.entitySystem.event.Event;
+
+/**
+ * Send this event to an entity to heal it.


"to heal it ..." - fully? Or could a negative number be hurtful instead? Maybe that's covered in the System

A negative value is supposed to cause damage, but not working at present. Will update the Javadoc when the issue is solved.

Cervator · 2019-05-22T03:16:29Z

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

+ * Event sent upon an entity reaching full health if previously on less than full health.
+ *
+ */
+public class FullHealthEvent implements Event {


Might this be more descriptive as OnFullHealthEvent ?

Would also put it more in line with OnHealedEvent - although then I'd wonder if this would work better as OnFullyHealedEvent plus wonder what the implications of both triggering would be

Renaming it as OnFullyHealedEvent, seems more apt for full health event.

Cervator · 2019-05-22T03:19:01Z

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

+    }
+
+    /**
+     * @return The amount by which health changed. This is capped, so if the entity received 9999 damage and only had 10 health, it will be -10.


Is it though? This method just seems to blindly return the change - I don't see any capping here? It could happen elsewhere but then that'd make this javadoc somewhat presumptuous :-)

It also helps to still write out both statements separately - the method description and the return value. Sometimes that can be tricky to write in a meaningful way split into two fields but since they each go to a different place in displayed javadoc it is ideal to have both (IMHO anyway)

An alternative might be to throw a boolean into a single event that indicates whether or not it resulted in a full (or partial) heal

I think it does show the changed amount because the capping is done not in the event but in the method calling the event.
https://github.com/Terasology/Health/pull/1/files#diff-e0c50392da47b79c5d45c9c66a5ec5feR112

So, i think the javadoc should be modified to say so.

Cervator · 2019-05-22T03:23:37Z

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

+                    event.subtract(5);
+                    event.multiply(2);
+                });
+        // expected = ( initial:10 + 10 - 5 ) * 2 = 30


I'd rewrite this ever so slightly for clarity. Like:

Suggested change

// expected = ( initial:10 + 10 - 5 ) * 2 = 30

// Expected value is ( initial:10 + 10 - 5 ) * 2 == 30

Add HealthComponent

274360a

Cervator requested changes May 17, 2019

View reviewed changes

e-aakash added 3 commits May 17, 2019 23:01

Remove Constructors

f8f242a

Add javadoc for HealthComponent

93a9aec

Update directory structure

f341472

Cervator approved these changes May 17, 2019

View reviewed changes

e-aakash added 3 commits May 17, 2019 23:58

Add heal events and HealthAuthoritySystem

211799b

Add healTest for testing heal event

84ade1c

Add javadoc for HealthAuthoritySystem methods

9b6f581

iaronaraujo approved these changes May 20, 2019

View reviewed changes

Add more tests for HealthAuthoritySystem

a4d033f

Cervator approved these changes May 22, 2019

View reviewed changes

Cervator merged commit a4d033f into Terasology:master May 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HealthComponent #1

Add HealthComponent #1

e-aakash commented May 16, 2019

Cervator left a comment

Cervator May 17, 2019

Cervator May 17, 2019

Cervator May 17, 2019

Cervator May 17, 2019

e-aakash May 17, 2019

iaronaraujo May 20, 2019

Cervator left a comment

Cervator May 22, 2019

Cervator May 22, 2019

Cervator May 22, 2019

Cervator May 22, 2019

e-aakash May 22, 2019

Cervator May 22, 2019

e-aakash May 22, 2019

Cervator May 22, 2019

e-aakash May 22, 2019 •

edited

Cervator May 22, 2019


		public boolean destroyEntityOnNoHealth;

		public HealthComponent() {

	// expected = ( initial:10 + 10 - 5 ) * 2 = 30
	// Expected value is ( initial:10 + 10 - 5 ) * 2 == 30

Add HealthComponent #1

Add HealthComponent #1

Conversation

e-aakash commented May 16, 2019

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

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

e-aakash May 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-aakash May 22, 2019 •

edited