Skip to content

Upstream DebugPickingPlugin from bevy_mod_picking #17177

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

Merged
merged 12 commits into from
Jan 7, 2025

Conversation

skimmedsquare
Copy link
Contributor

Objective

The debug features (DebugPickingPlugin) from bevy_mod_picking were not upstreamed with the rest of the core changes, this PR reintroduces it for usage inside bevy_dev_tools

Solution

Vast majority of this code is taken as-is from bevy_mod_picking aside from changes to ensure compilation and code style, as such @aevyrie was added as the co-author for this change.

Main changes

  • multiselection support - the relevant code was explicitly not included in the process of upstreaming the rest of the package, so it also has been omitted here.
  • bevy_egui support - the old package had a preference for using bevy_egui instead of bevy_ui if possible, I couldn't see a way to support this in a core crate, so this has been removed.

Relevant code has been added to the bevy_dev_tools crate instead of bevy_picking as it is a better fit and requires a dependency on bevy_ui for drawing debug elements.

Minor changes

  • Changed the debug text size from 60 to 12 as the former was so large as to be unreadable in the new example.

Testing

  • cargo run -p ci
  • Added a new example in dev_tools/picking_debug and visually verified the in-window results and the console messages

skimmedsquare and others added 2 commits January 5, 2025 15:57
Vast majority of this code is taken as-is from `bevy_mod_picking` aside
from changes to ensure compilation and code style

Main changes
* `selection` feature flag and support - the relevant code was
  explicitly not included in the process of upstreaming the rest of the
  package, so it also has been ommitted here.
* `bevy_egui` support - the old package had a preference for using
  `bevy_egui` instead of `bevy_ui` if possible, I couldn't see a way to
  support this in a core crate, so this has been removed.

Relevant code has been added to the `bevy_dev_tools` crate instead of
`bevy_picking` as it is a better fit and requires a dependency on
`bevy_ui` for drawing debug elements.

---------

Co-authored-by: Aevyrie <[email protected]>
@aevyrie
Copy link
Member

aevyrie commented Jan 5, 2025

Might make more sense to put the example in the picking group. That way people looking for the feature will see it. That, or just enable for all picking examples instead of making a standalone example.

Copy link
Contributor

github-actions bot commented Jan 5, 2025

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@skimmedsquare
Copy link
Contributor Author

Might make more sense to put the example in the picking group. That way people looking for the feature will see it. That, or just enable for all picking examples instead of making a standalone example.

Sounds reasonable - I don't have a strong opinion here, so I'll wait on some other reviewers to chime in before doing this.

Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

The code is a bit janky tbh. I'm not sure if it makes sense to store the data as a component instead of just holding a map in a resource. At times like this I really wish bevy UI was immediate mode.

This is probably fine to upstream as-is, but I'm not super pleased with the code quality, which is my own fault.

@alice-i-cecile this is missing the pointer click and drag overlay that the egui version has, because it's such a pita to just draw shapes on the screen in bevy. We really need screen space gizmos (screen space immediate mode line drawing) to do this kind of thing. This also highlights the usefulness of immediate mode UI when you just need to throw text, boxes, and circles on screen with a system, especially in dev tools. This entire feature could be a single system with imgui.

Edit: @skimmedsquare the criticism here is directed at myself, to be clear. Thanks for doing this legwork!

Copy link
Contributor

github-actions bot commented Jan 5, 2025

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Dev-Tools Tools used to debug Bevy applications. A-Picking Pointing at and selecting objects of all sorts D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 6, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile 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 bit of cleanup to do, but this is almost ready.

Copy link
Contributor

github-actions bot commented Jan 6, 2025

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@skimmedsquare
Copy link
Contributor Author

Might make more sense to put the example in the picking group. That way people looking for the feature will see it. That, or just enable for all picking examples instead of making a standalone example.

@alice-i-cecile Any thoughts on the suggestion from @aevyrie above? Was waiting on this before fixing the check-missing-examples-in-docs failure

@alice-i-cecile
Copy link
Member

Mild preference to move it into the picking group :)

Copy link
Contributor

github-actions bot commented Jan 7, 2025

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile self-requested a review January 7, 2025 05:19
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

That cleaned up nicely. Thanks!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 7, 2025
@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 Jan 7, 2025
Merged via the queue into bevyengine:main with commit 5faff84 Jan 7, 2025
33 checks passed
@skimmedsquare skimmedsquare deleted the upstream-debug-picking branch January 7, 2025 06:08
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
# Objective

The debug features (`DebugPickingPlugin`) from `bevy_mod_picking` were
not upstreamed with the rest of the core changes, this PR reintroduces
it for usage inside `bevy_dev_tools`

## Solution

Vast majority of this code is taken as-is from `bevy_mod_picking` aside
from changes to ensure compilation and code style, as such @aevyrie was
added as the co-author for this change.

### Main changes
* `multiselection` support - the relevant code was explicitly not
included in the process of upstreaming the rest of the package, so it
also has been omitted here.
* `bevy_egui` support - the old package had a preference for using
`bevy_egui` instead of `bevy_ui` if possible, I couldn't see a way to
support this in a core crate, so this has been removed.

Relevant code has been added to the `bevy_dev_tools` crate instead of
`bevy_picking` as it is a better fit and requires a dependency on
`bevy_ui` for drawing debug elements.

### Minor changes
* Changed the debug text size from `60` to `12` as the former was so
large as to be unreadable in the new example.

## Testing
* `cargo run -p ci`
* Added a new example in `dev_tools/picking_debug` and visually verified
the in-window results and the console messages

---------

Co-authored-by: Aevyrie <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Dev-Tools Tools used to debug Bevy applications. A-Picking Pointing at and selecting objects of all sorts C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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.

4 participants