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

BelowTheSurface - fix/players-attack #202

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

Conversation

zhusim222
Copy link

Below the surface - what's going on with the player's attack,

this was fixed as the code within the collision file did not perform the required task.
collision file was changed to reflect the desired damage and killing mechanic in the game

Fixes # (issue)

The palyer attack did nothing except the level 10 boss, the code had it remove HP but nothing happens when the enemy gets to 0 HP. this has now been fixed with if 0 then die, if higher it will do damage.
[ x ] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration

the attack was tested on each monster to ensure it worked, including enemies with more than 1 HP. went through each level with monsters and used the attack key to ensure their damage and HP matched against their death.

Testing Checklist

tested in MYSYS2

Checklist

[ X] My code follows the style guidelines of this project
[ X] I have performed a self-review of my own code
[ X] I have commented my code in hard-to-understand areas
[X ] I have made corresponding changes to the documentation
[ X] My changes generate no new warnings
[ X] I have requested a review from Tien on the Pull Request

@github-actions github-actions bot added the compiled the source code has been successfully compiled label May 16, 2024
@zhusim222
Copy link
Author

Copy link
Contributor

@MHLoppy MHLoppy left a comment

Choose a reason for hiding this comment

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

Can you change the check on line 342 to be <= 0 instead of == 0? The original check can lead to unintended bugs if other sources of damage reduce HP below 0.

@zhusim222 zhusim222 changed the title fix/players-attack BelowTheSurface - fix/players-attack May 17, 2024
Updated the if check to avoid error as per marks comment
@zhusim222
Copy link
Author

Thank you Mark, have made the changes

@zhusim222
Copy link
Author

Answering Marks question
Attack
The technical change itself looks good.

Do you have any comment on whether allowing the attack to affect non-boss enemies makes the game much easier? I know for example that the fast moving snake things are quite hard to kill via jumping -- basically: does this make the game too easy?

I don't believe it will affect the gameplay much, yes it will make the monsters easier to kill and avoid. but i believe the main focus on the game should be puzzle solving with some action on the side.

Copy link
Contributor

@MHLoppy MHLoppy left a comment

Choose a reason for hiding this comment

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

The gist of the change looks good and works fine in testing. However can you change the SFX line to run its usual if (!sound_effect_playing()) check first? Sorry for not noticing its absence in my earlier review on the double-PR - that's my bad as a reviewer.

if (!sound_effect_playing("EnemyDead"))
    play_sound_effect("EnemyDead");

I think this is the normal practice for playing sounds in SplashKit to avoid accidentally calling SFX more than once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiled the source code has been successfully compiled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants