SimpleIncremental icon indicating copy to clipboard operation
SimpleIncremental copied to clipboard

Remove Transform target and check it earlier

Open ErikOverflow opened this issue 6 years ago • 3 comments

Unsure of what the purpose of this is. targeting.target is going to be called no matter what. Storing it in a new Transform variable seems wasteful.

Doing a null check proves valuable, but it needs to happen earlier so that we aren't creating/pulling unnecessary prefabs from the Object Pool.

EnemyAttackRanged.cs line 74

ErikOverflow avatar Apr 08 '19 20:04 ErikOverflow

I added this to fix a bug. If the the enemy went to fire but the player moved out of target range, the value can get set to null before the enemy fires and causes a null exception. When this code executes it tries to access target.position with a null target and causes a null exception. This is a result of when firing the animation gets played than the animation tells when to actually fire based on its callback.

There are a few ways to fix this, but this seemed the simplest. I probably should have broke this fix out to a separate git commit so it could have explanation. Since the error was introduced as part of the functionality of syncing the animations with firing, i just included it with that commit.

raganvald avatar Apr 09 '19 15:04 raganvald

That makes sense. If we separate projectile firing from the animation it should eliminate this issue. We might want to revisit how projectiles fire. I'm still flip flopping on triggering projectile launch from the animation. I think a solution of "queueing" the projectile launch with its parameters at the start of the attack and "releasing" it via animation (so it's not doing a target check during the release portion) might work.

Also, rather than have a reloadTime that affects the coroutine to delay the projectile throwing, maybe we refactor it to attackSpeed and have speed affects the animator's Attack animation speed? Then we can have an "Attacking" bool parameter (instead of a trigger), and have the attack animation continue looping on itself while "Attacking" is true.

Does that make sense?

ErikOverflow avatar Apr 10 '19 15:04 ErikOverflow

You can do the same thing by changing the speed of the animation in code. My biggest concern with the separate coroutine is trying to sync them. The only way to tell would be to try and see what works better or has better drawbacks. Im guessing your trading independence from animation for syncing difficulties.

raganvald avatar Apr 10 '19 20:04 raganvald