Module cleanup + Potion "durability" + Documentation #3

xrtariq2594 · 2016-11-10T04:19:20Z

Contains

Moved potion prefabs from Alchemy module to this one.
Cleanup performed on the potion prefabs and related components to remove unnecessary stuff.
Potions can now be turned back into their original bottle prefabs as long as they have enough durability remaining (Add "durability" to potion bottles #1).
Added documentation to all classes.

How to Test

Spawn potions using the console, and drink them. If no potion bottle is returned, then it's a success.
To test the other scenario, modify one of the potion prefabs to include the following code:

    "Durability": {
        "durability": 9,
        "maxDurability": 9
    },

Then, spawn that potion and drink it. It should return an empty GlassBottle.

Proofread the documentation.

Issues

None so far.

Credits

@AWebb3701 and @Cervator for advice.

… update to the Alchemy module.

skaldarnar

Great work, and probably one of the best documented pieces of code in the whole project 😆
Have a look at the comments, mabye they are helpful at some point.

Tested out the durability, and it looks to work just right.

skaldarnar · 2016-11-10T21:58:41Z

src/main/java/org/terasology/potions/HerbEffect.java

+     * @param instigator    The instigator who is applying this effect on the entity. It can be a herb, potion, etc.
+     * @param entity        The entity who the herb effect is being applied on.
+     * @param magnitude     The magnitude of the effect.
+     * @param duration      The duration of the effect.


What is the unit of duration? Imho this should be stated in the javadoc, e.g., "The duration of the effect in seconds"

Done. And actually the duration is in milliseconds.

skaldarnar · 2016-11-10T22:00:00Z

src/main/java/org/terasology/potions/HerbEffect.java

+     * @param id            This particular HerbEffect's ID. Can be used to differentiate different effects under the
+     *                      same family.
+     * @param magnitude     The magnitude of the effect.
+     * @param duration      The duration of the effect.


missing unit spec (2)

skaldarnar · 2016-11-10T22:01:34Z

src/main/java/org/terasology/potions/PotionCommonEffects.java

@@ -15,7 +15,10 @@
 */
 package org.terasology.potions;

-// Class containing a list of constant Strings used for potion-related effect type checks.
+/**
+ * This class contains a list of constant Strings used for potion-related effect type checks. Most are self-explanatory


I would call the "constant Strings" just constants. Are these the effect IDs?

Yep, they are basically effect IDs.

skaldarnar · 2016-11-10T22:06:03Z

src/main/java/org/terasology/potions/component/PotionEffect.java

@@ -17,10 +17,15 @@

 import org.terasology.reflection.MappedContainer;

-// This class or container contains details on a particular effect of a potion.
+/** This class or mapped container contains details on a particular effect of a potion. */


Afaik, the mapped container allows to serialize the effect list. I'd just go for "Common class for describing a potion effect" or something like that.

Okay, going with:

Common class for describing an effect that may be present in a potion.

skaldarnar · 2016-11-10T22:07:39Z

src/main/java/org/terasology/potions/effect/AlterationEffectWrapperHerbEffect.java

+/**
+ * This HerbEffect wraps any AlterationEffect so that the latter can function within the herbalism systems and be treated
+ * as a HerbEffect.
+ */
 public class AlterationEffectWrapperHerbEffect implements HerbEffect {


Neatpicking on naming 😛 what about AlterationToHerbEffectWrapper?

Done! That was an old class from older maintainer (from WoodAndStone in fact).

skaldarnar · 2016-11-10T22:20:07Z

src/main/java/org/terasology/potions/events/BeforeDrinkPotionEvent.java

+     * Informs whether or not this event has been consumed.
+     *
+     * @return True if the the event has been consumed, false otherwise.
+     */
    @Override
    public boolean isConsumed() {


you may checkout {@inheritDoc} and use cases. this case seems like the javadoc should be same in the parent class (the consumable event). If you don't write javadoc at all, it is automatically copied from the override method.

It is done.

skaldarnar · 2016-11-10T22:21:13Z

src/main/java/org/terasology/potions/events/DrinkPotionEvent.java

@@ -20,34 +20,76 @@
 import org.terasology.entitySystem.event.Event;
 import org.terasology.potions.component.PotionComponent;

+/**
+ * This event is sent to an entity to indicate that it'll be drinking a given potion.


is this event treating the potion as one or is it sent for each effect, as the BeforeDrink event?

Treated the Potion as one.

DrinkPotionEvent sends the entire potion.
BeforeDrinkPotionEvent (now titled BeforeApplyPotionEffectEvent) works on one potion effect at a time.

skaldarnar · 2016-11-10T22:23:06Z

src/main/java/org/terasology/potions/system/DrinkPotionAuthoritySystem.java

+     *
+     * @param instigator    The entity who is trying to consume this potion.
+     * @param item          A reference to the potion item entity which has the potion effect.
+     * @param p             The PotionComponent of the potion item.


although well documented, please remove the abbreviated variable names and change them to something more clear

Done. I un-abbreviated the names.

skaldarnar · 2016-11-10T22:24:49Z

src/main/java/org/terasology/potions/system/DrinkPotionAuthoritySystem.java

+        EntityRef potion = event.getItem();
+        DurabilityComponent durability = potion.getComponent(DurabilityComponent.class);
+
+        // If the Durability component exists.


some people might argue about the usefulness of such comments... I don't mind them, but in this case it is very clear from the code what is happening

skaldarnar · 2016-11-10T22:25:11Z

src/main/java/org/terasology/potions/system/DrinkPotionAuthoritySystem.java

+
+        // If the Durability component exists.
+        if (durability != null) {
+            // If the new durability value will be above 0 following the potion drink, or the bottle has inf durability, proceed.


this one is fine, though ;-)

xrtariq2594 · 2016-11-11T02:57:51Z

Overall, I thank you for the suggestions @skaldarnar . I applied to them to the module, and I'll be pushing them tomorrow.

skaldarnar · 2016-11-15T13:29:35Z

Changes look good! What keeps you from merging this? Anything outstanding to be tested?

xrtariq2594 · 2016-11-15T14:46:08Z

Nothing. I was just waiting for your confirmation before merging @skaldarnar.

Now that I have it, I'll merge this in now.

xrtariq2594 added 5 commits October 29, 2016 15:24

Removed unsused items from prefabs and PotionComponent.

13c1062

First revision of issue #1.

b60324d

Removed WorkstationCrafting dependency and added component for future…

6536a29

… update to the Alchemy module.

Added a more durable glass bottle for potions.

6a12433

Added documentation to all classes (#2).

0184662

xrtariq2594 added Category: Doc Requests, Issues and Changes targeting javadoc and module documentation Type: Improvement Request for or addition/enhancement of a feature labels Nov 10, 2016

GooeyHub added the in progress label Nov 10, 2016

xrtariq2594 mentioned this pull request Nov 10, 2016

Drink potion system updates + UI improvements + Documentation Terasology/Alchemy#4

Merged

skaldarnar reviewed Nov 10, 2016

View reviewed changes

Appled @skaldarnar's suggestions.

da4480e

xrtariq2594 merged commit 0b70539 into master Nov 15, 2016

GooeyHub removed the in progress label Nov 15, 2016

xrtariq2594 deleted the develop branch November 15, 2016 14:57

This was referenced Nov 15, 2016

Add comments and documentation to all classes #2

Closed

Add "durability" to potion bottles #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module cleanup + Potion "durability" + Documentation #3

Module cleanup + Potion "durability" + Documentation #3

xrtariq2594 commented Nov 10, 2016 •

edited

Loading

skaldarnar left a comment

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016

skaldarnar Nov 10, 2016

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016 •

edited

Loading

skaldarnar Nov 10, 2016

xrtariq2594 Nov 11, 2016

skaldarnar Nov 10, 2016

skaldarnar Nov 10, 2016

xrtariq2594 commented Nov 11, 2016 •

edited

Loading

skaldarnar commented Nov 15, 2016

xrtariq2594 commented Nov 15, 2016

Module cleanup + Potion "durability" + Documentation #3

Module cleanup + Potion "durability" + Documentation #3

Conversation

xrtariq2594 commented Nov 10, 2016 • edited Loading

Contains

How to Test

Issues

Credits

skaldarnar 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

xrtariq2594 Nov 11, 2016 • edited Loading

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

xrtariq2594 commented Nov 11, 2016 • edited Loading

skaldarnar commented Nov 15, 2016

xrtariq2594 commented Nov 15, 2016

xrtariq2594 commented Nov 10, 2016 •

edited

Loading

xrtariq2594 Nov 11, 2016 •

edited

Loading

xrtariq2594 commented Nov 11, 2016 •

edited

Loading