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

Resetting Escape Ability #157

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Kyof0
Copy link
Contributor

@Kyof0 Kyof0 commented Oct 2, 2024

Added ReduceCooldownAmount to BaseUsable's constructor default value is 0f. It can be used if we want to reduce cooldown based on any condition. It's only used in EscapeSpecial for now. Also added HasDurability to BaseUsable and than override this in both AmmoWeapon and BladeWeapon. Main reason i did this is BladeWeapon.CanUse() is also looking for HumanState.Idle and this disables blade weapon to use escape special. Escape Special has 300s cooldown, 50f ReduceCooldownAmount and its unable to use at start.

Kyof0 added 5 commits October 1, 2024 20:38
Added ReduceCooldownAmount to BaseUsable's constructor default value is 0f. It can be used if we want to reduce cooldown based on any condition. It's only used in EscapeSpecial for now.  Also added HasDurability to BaseUsable and than override this in both AmmoWeapon and BladeWeapon. Main reason i did this is BladeWeapon.CanUse() is also looking for HumanState.Idle and this disables blade weapon to use escape special. Escape Special has 300s cooldown, 50f ReduceCooldownAmount and its unable to use at start.
Fixed a bug which causes not to play sound effects.
Escaping from titan makes its arm disabled
public int UsesLeft;
public int MaxUses;
public bool IsActive;
public string Name;
protected float _lastUseTime = -1000f;
protected BaseCharacter _owner;

public BaseUseable(BaseCharacter owner, float cooldown = 0f, int maxUses = -1)
public BaseUseable(BaseCharacter owner, float cooldown = 0f, float _reduceCooldownAmount =0f,int maxUses = -1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use regular field naming convention: "reduceCooldownAmount = 0, int maxUses = -1"

public int UsesLeft;
public int MaxUses;
public bool IsActive;
public string Name;
protected float _lastUseTime = -1000f;
protected BaseCharacter _owner;

public BaseUseable(BaseCharacter owner, float cooldown = 0f, int maxUses = -1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are changing the BaseUseable here, this will affect all specials, is this intended?

@@ -1163,6 +1165,10 @@ public override void OnHit(BaseHitbox hitbox, object victim, Collider collider,
}

}
if (GetCurrentSpecial() is EscapeSpecial || GetCurrentSpecial() is ShifterTransformSpecial)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are tying special logic to certain specials, this should be handled either using OOP principles so that all specials of a new special type called BaseChargeOnkillSpecial implement this logic so that adding new ones doesn't require going and changing this random if statement.

@@ -2515,7 +2520,15 @@ public void SetSpecial(string special)
Special = HumanSpecials.GetSpecialUseable(this, special);
((InGameMenu)UIManager.CurrentMenu).HUDBottomHandler.SetSpecialIcon(HumanSpecials.GetSpecialIcon(special));
}

public BaseUseable GetCurrentSpecial()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Special is exposed as a public property, you should reference it as this.Special or human.Special, please remove this.

{
return Special;
}
public void ReduceSpecialCooldown()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call this.Special.ReduceCooldown() or human.Special.ReduceCooldown()

@@ -1,6 +1,12 @@
using Effects;
using Codice.CM.Client.Differences.Graphic;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these imports do not look used, please remove them

using UnityEngine;
using static UnityEngine.GraphicsBuffer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one in particular is really weird, did auto import add the static field here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't change those, will remove

if (human.Weapon is APGWeapon)
{
//var gunInfo = CharacterData.HumanWeaponInfo["APG"];
//float minRange = gunInfo["MinRange"].AsFloat;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused code

}
UsesLeft = -1;
Cooldown = 300;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cooldown should be implemented so that there is a
Cooldown variable for the max cooldown and an internal one
_currentCooldown
This way we dont have magic numbers randomly littered throughout the code.

If someone changes the cooldown at the top, they now need to search the codebase for all references to it bc of this change. Please avoid this at all costs. I do not want to come off as rude but this was the bane of my existence working on RRC and I hate it with a passion.

using UI;
using UnityEngine;
using Utility;
using static UnityEngine.GraphicsBuffer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More weird imports

@@ -75,5 +78,21 @@ protected override void Activate()
human.HumanCache.APGHit.Activate(0f, 0.1f);
((InGameMenu)UIManager.CurrentMenu).HUDBottomHandler.ShootAPG();
}
public object[] GetSettings()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just expose each of these individual settings as props.
If they have the be calculated at runtime, use lambda notation

public float ThatFirstValueBelow => midpoint + direction * height * 0.5f;

This is heavily preferred to creating a new object everytime you need to reference some basic settings.

@@ -169,7 +169,10 @@ int KillTitansInRadius(float radius)
((InGameCamera)SceneLoader.CurrentCamera).TakeSnapshot(titan.BaseTitanCache.Neck.position, damage);
titan.GetHit(_owner, damage, "Thunderspear", collider.name);
}

if(((Human)_owner).GetCurrentSpecial() is EscapeSpecial || ((Human)_owner).GetCurrentSpecial() is ShifterTransformSpecial)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again magic list of specials that this applies to, this should be generic somewhere else so that we don't have duplicate code.

@AutumnThyme AutumnThyme self-requested a review October 22, 2024 00:17
@@ -16,7 +16,7 @@ abstract class BaseUseable
protected float _lastUseTime = -1000f;
protected BaseCharacter _owner;

public BaseUseable(BaseCharacter owner, float cooldown = 0f, int maxUses = -1)
public BaseUseable(BaseCharacter owner, float cooldown = 0f,int maxUses = -1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix the missing space here

Copy link
Collaborator

@AutumnThyme AutumnThyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the weird space thing and you're good. Still need rc to look this over though.

@AutumnThyme AutumnThyme requested a review from rc174945 October 22, 2024 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants