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

feat: Cannon re-implement #965

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

DRuppFv
Copy link
Contributor

@DRuppFv DRuppFv commented Apr 9, 2024

This PR closes #576.

I started this as draft because I think the team feedback is important before I head to writing the cannon itself.

There are many .yaml fields that are used only when holding the kickbomb, and I wonder if I should delete these from the cannonball (as it will only work at the lit state) or just leave it there because the game would just crash if the cannonball is used as non-lit.

I also used .clone() while dealing with SVec<> and animations in kick_bomb.rs because I think it was the only option, but using .clone() is quite inefficient so I wonder if there's another way of doing that.

@MaxCWhitehead
Copy link
Collaborator

MaxCWhitehead commented Apr 9, 2024

There are many .yaml fields that are used only when holding the kickbomb, and I wonder if I should delete these from the cannonball (as it will only work at the lit state) or just leave it there because the game would just crash if the cannonball is used as non-lit.

Not sure exactly but good question. What do you mean by the game will crash if cannonball used as non-lit? Not suggesting this is untrue just not clear to me where exactly this would crash. Generally, it's best not to crash unless in a very unrecoverable situation or really want to draw attention to a problem that cannot be handled in a reasonable way. Otherwise logging warnings or errors to indicate an issue and trying to continue game is usually best course of action.

I also used .clone() while dealing with SVec<> and animations in kick_bomb.rs because I think it was the only option, but using .clone() is quite inefficient so I wonder if there's another way of doing that.

I wouldn't worry too much about the clone - should only be when setting up new cannon ball, and it's not a lot of data to clone. While not the perfect solution, but this is simple at least and doing something else is probably not super easy.

@DRuppFv
Copy link
Contributor Author

DRuppFv commented Apr 10, 2024

What do you mean by the game will crash if cannonball used as non-lit? Not suggesting this is untrue just not clear to me where exactly this would crash. Generally, it's best not to crash unless in a very unrecoverable situation or really want to draw attention to a problem that cannot be handled in a reasonable way. Otherwise logging warnings or errors to indicate an issue and trying to continue game is usually best course of action.

Oh, I think there was a misunderstanding, by "crash" I just meant it would get an error - which I'm sure is 100% recoverable. Yeah I will delete the fields, it just felt weird writing cannonball using kick_bomb as a base while I can't use all kick_bomb functions.

I wouldn't worry too much about the clone - should only be when setting up new cannon ball, and it's not a lot of data to clone. While not the perfect solution, but this is simple at least and doing something else is probably not super easy.

I see, I am gonna keep it as it is. Thank you for the feedback.

@DRuppFv DRuppFv force-pushed the reimp_cannon branch 2 times, most recently from 61878bd to 3833bb7 Compare April 20, 2024 07:23
@DRuppFv DRuppFv closed this Apr 20, 2024
@DRuppFv DRuppFv reopened this Apr 24, 2024
@DRuppFv DRuppFv marked this pull request as ready for review April 27, 2024 23:02
@MaxCWhitehead
Copy link
Collaborator

The cannon is awesome. Is an exciting weapon, great work!

@MaxCWhitehead MaxCWhitehead added this pull request to the merge queue May 2, 2024
Merged via the queue into fishfolk:main with commit fe29788 May 2, 2024
8 checks passed
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.

Re-implement Cannon
2 participants