Skip to content
This repository has been archived by the owner on Aug 14, 2019. It is now read-only.

Remove Transform target and check it earlier #36

Open
ErikOverflow opened this issue Apr 8, 2019 · 3 comments
Open

Remove Transform target and check it earlier #36

ErikOverflow opened this issue Apr 8, 2019 · 3 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ErikOverflow
Copy link
Owner

ErikOverflow commented Apr 8, 2019

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 ErikOverflow added enhancement New feature or request good first issue Good for newcomers labels Apr 9, 2019
@raganvald
Copy link
Contributor

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.

@ErikOverflow
Copy link
Owner Author

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?

@raganvald
Copy link
Contributor

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.

@ErikOverflow ErikOverflow self-assigned this Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants