Skip to content
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

Add Alteration effects and Impulse based weapons support #43

Open
wants to merge 6 commits into
base: master
from

Conversation

@darshan3
Copy link
Member

commented Aug 16, 2019

No description provided.

@@ -10,17 +25,22 @@
* <p>
* {@link HurtEvent} is used to hurt other entity/entities.
*/
public class HurtingComponent implements Component{
public class HurtingComponent implements Component {
/**
* The amount of damage the entity will inflict.
*/
@Replicate
public int amount = 3;

This comment has been minimized.

Copy link
@iaronaraujo

iaronaraujo Aug 18, 2019

Contributor

magic number

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

It's a magic number, yes, but what would be a better solution here?

If this is a default value it should be mentioned in the doc comment. Maybe there's a more sensible default value such as 0 which is kind of neutral?

@iaronaraujo Do you think the default values are important to know outside this component?



@Replicate
public long duration = 2000;

This comment has been minimized.

Copy link
@iaronaraujo

iaronaraujo Aug 18, 2019

Contributor

magic number

@@ -9,7 +24,7 @@
* Damaging critically means multiplying the original damage by <b>critFactor</b>.
* The critical damage has <b>critChance</b>/100 probability of occurring.
*/
public class CritDamageComponent implements Component{
public class CritDamageComponent implements Component {

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

Note to self: figure out why CritDamageComponent has an int value do denote a percentage 🤔

public HurtEvent(){
private EntityRef target = EntityRef.NULL;

private Vector3f impulseDirection = new Vector3f();

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

If the impulseDirection cannot be changed for this event you can make it final:

Suggested change
private Vector3f impulseDirection = new Vector3f();
final private Vector3f impulseDirection;

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

The member then needs to be initialized in the constructor(s).


private Vector3f impulseDirection = new Vector3f();

public HurtEvent() {

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

Is this constructor needed? What is a HurtEvent without a target or direction?

@@ -10,17 +25,22 @@
* <p>
* {@link HurtEvent} is used to hurt other entity/entities.
*/
public class HurtingComponent implements Component{
public class HurtingComponent implements Component {
/**
* The amount of damage the entity will inflict.
*/
@Replicate
public int amount = 3;

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

It's a magic number, yes, but what would be a better solution here?

If this is a default value it should be mentioned in the doc comment. Maybe there's a more sensible default value such as 0 which is kind of neutral?

@iaronaraujo Do you think the default values are important to know outside this component?

dotAlterationEffect.applyEffect(instigator, otherEntity, hurtingComponent.amount, hurtingComponent.duration);
}
break;
default:

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

This treats everything else than the previously matched cases as "damage" type - is that intended? Keep in mind that the hurtingType is an arbitrary string, thus it may contain full garbage such as "watermelon" - should that be treated as damage or do we want to ignore that case (and just log a warning)?

@Replicate
public Prefab explosionPrefab;
@Replicate
public Vector3f impulseDirection;

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Aug 21, 2019

Does the ExplodeComponent really need an impulseDirection? Or can it be derived from the location and the fact that it is an explosion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.