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

2580 Split examples PR feedback #15181

Merged
merged 8 commits into from
Sep 13, 2024

Conversation

bas-ie
Copy link
Contributor

@bas-ie bas-ie commented Sep 13, 2024

Objective

Applies feedback from previous PR #15135 'cause it got caught up in the merge train 🚂

I couldn't resist including roll, both for completeness and due to playing too many games that implemented it as a child.

cc: @janhohenheim

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Lovely! The example is really really good now, great work :) Left two nits and a correction

}

#[derive(Debug, Default, Resource)]
struct MouseButtonsPressed {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the example would be a bit slimmer without this resource? I get that actions should be separated from keycodes, but we read them in the same system as we do the movement right now anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be me wildly misunderstanding the problem, but: I think only one event gets sent per button pressed (and one on button release). So, the problem becomes how to detect if the user is holding the button, hence the resource.

Copy link
Member

@janhohenheim janhohenheim Sep 13, 2024

Choose a reason for hiding this comment

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

That would be input.just_pressed. input.pressed is true when it was pressed in the first frame and stays true when held the next frames.

Copy link
Member

@janhohenheim janhohenheim Sep 13, 2024

Choose a reason for hiding this comment

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

Edit: Ah, I see your problem! You're using EventReader<MouseButtonInput>. Use Res<ButtonInput<MouseButton>> instead, that already handles this stuff for you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dammit lol I knew something weird was going on!

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

I think this has now become a really good showcase of different features. Fantastic work!

@janhohenheim janhohenheim added C-Examples An addition or correction to our examples C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 13, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 13, 2024
Merged via the queue into bevyengine:main with commit c454db8 Sep 13, 2024
35 checks passed
@bas-ie bas-ie deleted the 2580-split-examples branch September 14, 2024 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants