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

Add examples for orthographic and perspective zoom #15092

Merged
merged 12 commits into from
Sep 9, 2024

Conversation

bas-ie
Copy link
Contributor

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

Objective

Add examples for zooming (and orbiting) orthographic and perspective cameras.

I'm pretty green with 3D, so please treat with suspicion! I note that if/when #15075 is merged, .scale will go away so this example uses .scaling_mode.

Closes #2580

@IQuick143 IQuick143 added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples S-Blocked This cannot move forward until something else changes S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Docs An addition or correction to our documentation and removed S-Blocked This cannot move forward until something else changes labels Sep 8, 2024
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.

A few notes, but overall I really like these examples. Very actionable and relevant, with clear code.

@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 9, 2024

BTW I've seen both, but do we have a standard for choosing "instructions on the screen as UI" vs "instructions in an info! in the terminal output"?

@alice-i-cecile
Copy link
Member

Moderate preference for instructions on screen, especially if it's not going to interfere with the example otherwise.

@bas-ie bas-ie force-pushed the add-zoom-projection-examples branch from 2bda09e to d8f6caa Compare September 9, 2024 01:24
@alice-i-cecile
Copy link
Member

Fantastic, thank you :)

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.

Reviewed only one example, but the comments apply to both.
I like that these examples exist, but there are some things in here that are a bit suboptimal when using the examples as a general learning resource, which a lot of beginners do. I'd like to showcase best practices a bit more if possible.

examples/3d/perspective_zoom.rs Outdated Show resolved Hide resolved
examples/3d/perspective_zoom.rs Outdated Show resolved Hide resolved
examples/3d/perspective_zoom.rs Outdated Show resolved Hide resolved
examples/3d/perspective_zoom.rs Outdated Show resolved Hide resolved
examples/3d/perspective_zoom.rs Outdated Show resolved Hide resolved
examples/3d/perspective_zoom.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,125 @@
//! Shows how to zoom and orbit a perspective projection camera.
Copy link
Member

Choose a reason for hiding this comment

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

Could you cross-reference this and the first person view model example? Both feature perspective zoom.

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'm not entirely convinced that this should be two separate examples instead of one. Would it complicate the logic a lot if you added a toggle between the two modes when pressing space?

Cargo.toml Outdated Show resolved Hide resolved
@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 9, 2024

Would it complicate the logic a lot if you added a toggle between the two modes when pressing space?

TBH, I did think there was an awful lot of repetition. Let's see what it looks like as a combined effort.

@bas-ie bas-ie force-pushed the add-zoom-projection-examples branch from 41f8cf7 to 2631c71 Compare September 9, 2024 10:55
@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 9, 2024

@janhohenheim How do we feel about this? I don't know if I've just muddied the waters, but at least everything is in one place... 🤔

Will clean up last bits and pieces tomorrow, otherwise I will be pitchin' and yawin' in my sleep.

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.

Looks pretty good now! Minor naming suggestion and we are good to go once you did your edits. Approving already :)

@@ -255,6 +255,7 @@ Example | Description
--- | ---
[2D top-down camera](../examples/camera/2d_top_down_camera.rs) | A 2D top-down camera smoothly following player movements
[First person view model](../examples/camera/first_person_view_model.rs) | A first-person camera that uses a world model and a view model with different field of views (FOV)
[Projection Zoom](../examples/camera/projection_zoom.rs) | Shows how to zoom and orbit orthographic and perspective projection cameras.
Copy link
Member

Choose a reason for hiding this comment

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

I quite like the orbit showcase. I don't think we had that before :)
Maybe rename the file to orbit_and_projection_zoom, with the example name changing accordingly?

@janhohenheim
Copy link
Member

@richchurcher I'd also be happy if we split this into an example that showed the projection scaling on a static scene (e.g. our fox model) without any movement controls, and one example that only showed the orbit. That would probably be cleaner, but I'll leave the choice to you :)

@alice-i-cecile
Copy link
Member

The merge train waits for no man 🚋 Seriously though, I think this is good enough to merge, but do feel free to tweak it in follow-ups.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 9, 2024
@alice-i-cecile
Copy link
Member

Ah, this needs revision following #15073 <3

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 9, 2024
@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 9, 2024

The train stops for no-one! I'll revise in a sec.

@bas-ie bas-ie force-pushed the add-zoom-projection-examples branch from bba4dbe to 095da01 Compare September 9, 2024 22:59
@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 9, 2024

Does some kind person want to expand a tiny bit on into_inner vs deref_mut? lol

@alice-i-cecile
Copy link
Member

into_inner is a function that turns Mut<T> into &mut T. @janhohenheim got it confused with deref_mut, which didn't compile here. The ultimate goal was just a style nit to avoid the &mut * pattern which can be ugly to read. CI was failing so I checked out your PR locally and fixed it before pushing to your branch :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2024
Merged via the queue into bevyengine:main with commit b9b43ad Sep 9, 2024
30 checks passed
@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 9, 2024

Thanks! I'll split the example in a separate PR.

@bas-ie bas-ie deleted the add-zoom-projection-examples branch September 9, 2024 23:58
github-merge-queue bot pushed a commit that referenced this pull request Sep 10, 2024
# Objective

As previously discussed, split camera zoom and orbiting examples to keep
things less cluttered. See discussion on #15092 for context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add example demonstrating how to zoom orthographic cameras
4 participants