Skip to content

2580 Split examples PR feedback#15181

Merged
alice-i-cecile merged 8 commits intobevyengine:mainfrom
bas-ie:2580-split-examples
Sep 13, 2024
Merged

2580 Split examples PR feedback#15181
alice-i-cecile merged 8 commits intobevyengine:mainfrom
bas-ie:2580-split-examples

Conversation

@bas-ie
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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