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

Main elf enhancement #162

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Main elf enhancement #162

merged 2 commits into from
Nov 29, 2023

Conversation

bpyeh
Copy link
Collaborator

@bpyeh bpyeh commented Nov 29, 2023

Adds another Elf asset depicted after throwing the present, and a small state machine to model the action.

One subtle thing missing here is the overhead present doesn't disappear after throwing, but the action is quick enough that we can kind of get away with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this logic could probably be simplified by just having 1 variable, the animateCooldown. You could get rid of currentUserEvent and playerElfState and just update the logic to see if the animateCooldown is greater than 0 or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's a bit of futureproofing - if we wanted a state for player elf diving into the cart to fetch another present this could easily be extended. I guess the currentUserEvent isn't adding too much, but I wanted to decouple the click callback from the update logic

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the reason behind the suggestion is to contain the state to as few variables as possible, and then calculate everything from that. It's possible for someone to make a change and put the variables in an invalid state at the moment, whereas if say you had a function that was getPlayerElfState() that just returned the state based on the animateCooldown then there's no way to set the playerElfState in a way that conflicts with the animateCooldown.

For the future proofing case, we'd apply the same logic - everything could be calculated from the animateCooldown, including which frame of the animation to play. I've used that approach in some of the games I've made in the past and it works well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you're saying. I'll try to get to this if I have time.

Copy link
Collaborator

@Jezzamonn Jezzamonn left a comment

Choose a reason for hiding this comment

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

Leaving my approval so we can merge this in it's current state if needed!

@Jezzamonn Jezzamonn merged commit db970cc into main Nov 29, 2023
5 checks passed
@Jezzamonn Jezzamonn deleted the main-elf-enhancement branch November 29, 2023 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants