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

Feature: Secret Chime #2453

Closed
wants to merge 2 commits into from
Closed

Feature: Secret Chime #2453

wants to merge 2 commits into from

Conversation

Ovi-111
Copy link
Contributor

@Ovi-111 Ovi-111 commented Sep 3, 2024

What

This Pull Request introduces a Secret Chime feature for dungeons.

  • Plays a sound when a chest, lever, or wither essence is clicked inside dungeons.
  • Secret Chime pitch and sound can be changed.

I'm still new to contributing. So I'm still learning. How things work.

Changelog New Features

  • Secret Chime. - Ovi_1
    • Plays a user-specified sound when a chest, lever, or wither essence is clicked inside dungeons.

@Ovi-111 Ovi-111 changed the title Added Secret Chime Feature: Secret Chime Sep 3, 2024
Copy link
Collaborator

@CalMWolfs CalMWolfs left a comment

Choose a reason for hiding this comment

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

Just a few suggestions

Also you did not follow the pull request template which means your pr will not be added to the changelogs. It will not be merged until this is fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should link to the list of sounds like other features in the mod that allow you to select a sound do

else -> false
}

if (event.position.getBlockAt() == Blocks.skull) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why doesnt this use the position variable you created above

}

if (event.position.getBlockAt() == Blocks.skull) {
val text = BlockUtils.getTextureFromSkull(position.toBlockPos())
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably rename the variable to texture so that its more descriptive

Comment on lines 23 to 26
val blockType = when (position.getBlockAt()) {
Blocks.chest, Blocks.trapped_chest, Blocks.lever, Blocks.skull -> true
else -> false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of setting a boolean variable here, just return where you are currently setting it to false. If you want to make this code better you would also do the wither essence texture check in this when statement if the block is of skull type

@hannibal002 hannibal002 added this to the Version 0.28 milestone Sep 4, 2024
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

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

Code looks overall fine. I have not yet tested it in game.

One big problem though: this is code duplication to DungeonHighlightClickedBlocks.
You can avoid this by moving the enum ClickedBlockType into a own class, and create a "dungeon clicked block event" with contains one of those enums as well. move that check into dungeon api. then you can listen to that new event inside the clicked blocks and secret chime feature at the same time.

@Ovi-111 Ovi-111 deleted the branch hannibal002:beta September 7, 2024 15:21
@Ovi-111 Ovi-111 closed this Sep 7, 2024
@Ovi-111 Ovi-111 deleted the beta branch September 7, 2024 15:21
@Ovi-111 Ovi-111 restored the beta branch September 7, 2024 15:21
@Ovi-111 Ovi-111 deleted the beta branch September 7, 2024 15:22
@CalMWolfs CalMWolfs removed this from the Version 0.28 milestone Sep 20, 2024
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.

3 participants