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.

Sign up for GitHub

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

Jump to bottom

Add frustum gizmo #10038

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

Add frustum gizmo #10038

wants to merge 5 commits into from
+279 −32

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Oct 6, 2023 •
edited
Loading

Adds the FrustumGizmo which works in a similar fashion to the previously added AabbGizmo.

Closes #4082

Preventing cameras from drawing their own frustums made this a lot messier but as a result this is now technically the first retained gizmo q:

Here's a screenshot of a modified split_screen example showing the frustum of one view in the other.
image

Changelog

Added the FrustumGizmo component for drawing the frustums of cameras with the relevant settings added to the GizmoConfig resource.

superdump
superdump reviewed Oct 8, 2023
View reviewed changes
crates/bevy_gizmos/src/pipeline_2d.rs
@@ -160,6 +161,12 @@ fn queue_line_gizmos_2d(
| Mesh2dPipelineKey::from_hdr(view.hdr);

for (entity, handle) in &line_gizmos {
// The frustum gizmo adds a linegizmo to the same entity.
// We can use that here to prevent views from rendering their own frustum.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

superdump
superdump requested changes Oct 8, 2023
View reviewed changes
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Left some minor comments, and I think the frustum corner calculations could maybe be on the Frustum impl.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Gizmos Visual editor and debug gizmos labels Oct 9, 2023
@tim-blackbird
Copy link
Contributor Author

Thanks for the comments :)

I took a look at the frusta on the lights after reading the comments on discord and noticed that spotlights already have a Frustum component. That means this PR should have just workedtm but unfortunately the halfspace intersection tests to determine the corners are failing for the spotlight frusta.

@jeliag jeliag mentioned this pull request Nov 2, 2023
Merged
@torsteingrindvik
Copy link
Contributor

I'd use this feature if it was in.

@irate-devil any blockers on this?

github-merge-queue bot pushed a commit that referenced this pull request Jan 18, 2024
@jeliag @nicopap
Multiple Configurations for Gizmos (#10342)
f6b40a6 
# Objective

This PR aims to implement multiple configs for gizmos as discussed in
#9187.

## Solution

Configs for the new `GizmoConfigGroup`s are stored in a
`GizmoConfigStore` resource and can be accesses using a type based key
or iterated over. This type based key doubles as a standardized location
where plugin authors can put their own configuration not covered by the
standard `GizmoConfig` struct. For example the `AabbGizmoGroup` has a
default color and toggle to show all AABBs. New configs can be
registered using `app.init_gizmo_group::<T>()` during startup.

When requesting the `Gizmos<T>` system parameter the generic type
determines which config is used. The config structs are available
through the `Gizmos` system parameter allowing for easy access while
drawing your gizmos.

Internally, resources and systems used for rendering (up to an including
the extract system) are generic over the type based key and inserted on
registering a new config.

## Alternatives

The configs could be stored as components on entities with markers which
would make better use of the ECS. I also implemented this approach
([here](https://github.com/jeliag/bevy/tree/gizmo-multiconf-comp)) and
believe that the ergonomic benefits of a central config store outweigh
the decreased use of the ECS.

## Unsafe Code

Implementing system parameter by hand is unsafe but seems to be required
to access the config store once and not on every gizmo draw function
call. This is critical for performance. ~Is there a better way to do
this?~

## Future Work

New gizmos (such as #10038, and ideas from #9400) will require custom
configuration structs. Should there be a new custom config for every
gizmo type, or should we group them together in a common configuration?
(for example `EditorGizmoConfig`, or something more fine-grained)

## Changelog

- Added `GizmoConfigStore` resource and `GizmoConfigGroup` trait
- Added `init_gizmo_group` to `App`
- Added early returns to gizmo drawing increasing performance when
gizmos are disabled
- Changed `GizmoConfig` and aabb gizmos to use new `GizmoConfigStore`
- Changed `Gizmos` system parameter to use type based key to retrieve
config
- Changed resources and systems used for gizmo rendering to be generic
over type based key
- Changed examples (3d_gizmos, 2d_gizmos) to showcase new API

## Migration Guide

- `GizmoConfig` is no longer a resource and has to be accessed through
`GizmoConfigStore` resource. The default config group is
`DefaultGizmoGroup`, but consider using your own custom config group if
applicable.

---------

Co-authored-by: Nicola Papale <[email protected]>
@JMS55 JMS55 added this to the 0.14 milestone Mar 6, 2024
@0x0539
Copy link

0x0539 commented Mar 6, 2024

It would be great to have this functionality

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 16, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone May 16, 2024
@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superdump superdump superdump requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-Gizmos Visual editor and debug gizmos C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add debug wireframe rendering for Aabb, Frustum, and Sphere
6 participants
@tim-blackbird @torsteingrindvik @0x0539 @superdump @alice-i-cecile @JMS55