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

Documenting OrthographicProjection. #5824

Closed
wants to merge 27 commits into from
Closed

Documenting OrthographicProjection. #5824

wants to merge 27 commits into from

Conversation

bzm3r
Copy link
Contributor

@bzm3r bzm3r commented Aug 28, 2022

Objective

Documentation for OrthographicProjection is something that is often asked for implicitly in the #help channel on Discord. More explicitly, we now have #5818 , which this PR aims to fix.

Solution

Provided explanation for what left/right/top/bottom are, and in what units they are given. Also provided explanations for concepts like "field of view", "scale" and "window".

Brian Merchant and others added 27 commits May 6, 2022 13:30
Co-authored-by: Robert Swain <robert.swain@gmail.com>
…w being merged.

Co-authored-by: Robert Swain <robert.swain@gmail.com>
…ce memory"

Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
…t of links to other data storage options at end of docstring fro each particular data storage option
@bzm3r bzm3r marked this pull request as draft August 28, 2022 19:23
@bzm3r
Copy link
Contributor Author

bzm3r commented Aug 28, 2022

@superdump ping as you've requested, for rendering related docs 😄

@Weibye Weibye added the C-Docs An addition or correction to our documentation label Aug 28, 2022
@Weibye Weibye added the A-Rendering Drawing game state to the screen label Aug 28, 2022
@@ -131,14 +131,16 @@ impl Default for PerspectiveProjection {
#[derive(Debug, Clone, Reflect, FromReflect, Serialize, Deserialize)]
#[reflect(Serialize, Deserialize)]
pub enum WindowOrigin {
/// `(0, 0, 0)` is at the center of the screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `(0, 0, 0)` is at the center of the screen.
/// `(0, 0)` is at the center of the screen.

This affects only 2 dimensions.

Comment on lines +199 to +201
/// The value by which `left`, `right`, `bottom`, and `top` are multiplied (scaled). The smaller the `scale`, the smaller the
/// field of view, and thus, the *greater* the magnification of objects in the field of view. The larger the `scale`, the
/// larger the field of view, and thus the *smaller* the magnification of objects in the field of view.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is too specific, but it would be nice to note somewhere that this is what people will typically want to manipulate to achieve a "camera zoom" effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can put a doc alias here

Comment on lines +164 to +165
/// than the field of view, then some objects will naturally not appear on the screen. The relationship between the field of view
/// and the window is governed by [`window_origin`].
Copy link
Contributor

@sarkahn sarkahn Aug 29, 2022

Choose a reason for hiding this comment

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

That last sentence feels a bit off to me - aside from it being a bit vague, I think the purpose of window_origin should be explained within the struct or a related function, not in the general overview.

pub near: f32,
/// The location of the face (in the z-direction) farthest from the camera, in world units.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The location of the face (in the z-direction) farthest from the camera, in world units.
/// The distance of the clipping plane (in the z-direction) farthest from the camera, in world units.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't "furthest" more common than "farthest"?

pub top: f32,
/// The location of the face (in the z-direction) nearest to the camera, in world units.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The location of the face (in the z-direction) nearest to the camera, in world units.
/// The distance of the clipping plane (in the z-direction) nearest to the camera, in world units.

Copy link
Contributor

@Nilirad Nilirad 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 basic writing guidelines. Make sure you're only talking about the projection itself. Anything concerning the camera should go in its relative item.

I also advice semantic line breaks: they're getting popular lately in the repo.

Note: CI fails because rustdoc can't find links to objects like [`window_origin`]. You should transform those into [`window_origin`](Self::window_origin) to make them work.

@@ -154,17 +156,49 @@ pub enum ScalingMode {
FixedHorizontal(f32),
}

/// A camera using `OrthographicProjection` displays the world within a rectangular prism (the "viewport", the "frustum", or
Copy link
Contributor

Choose a reason for hiding this comment

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

The description should be phrased in the terms of the type itself, not of the camera.

It could be something like: “Projects a 3D space onto a 2D surface using a rectangular prism.”, but I don't know if it is 100% correct since rendering is not my strong point.

The point is that the description of an item should not go outside its scope. You should also isolate the first sentence into its own paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also "viewport" and "frustum" are two different things, but are used as synonyms here.

@@ -154,17 +156,49 @@ pub enum ScalingMode {
FixedHorizontal(f32),
}

/// A camera using `OrthographicProjection` displays the world within a rectangular prism (the "viewport", the "frustum", or
/// "field of view"). Objects that do not lie in this prism are culled ("frustum culling"): in other words, they are not displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Triangles that fall outside the frustum are naturally not displayed, since they can't be projected. Frustum culling is just an algorithm that optimizes this process by not even trying to render certain things that are certainly out of sight.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is unrelated to the projection component. This should probably be deleted.

#[derive(Component, Debug, Clone, Reflect, FromReflect)]
#[reflect(Component, Default)]
pub struct OrthographicProjection {
/// The location of the left face of the camera's field of view, relative to the origin specified in [`window_origin`], in
Copy link
Contributor

Choose a reason for hiding this comment

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

"field of view" generally refers to the concept about how wide an angle the camera uses. Here we're talking about planes/faces, which is more about the frustum I think.

pub far: f32,
/// Specifies where `(0, 0, 0)` is located.
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not really help comprehension I find. The "origin" is (0,0,0) so the comment is kind of redundant. It would be better to explain what this affects and how to configure it, maybe with an example.

@xgbwei
Copy link
Contributor

xgbwei commented Oct 6, 2022

@alice-i-cecile This PR has been inactive so I've started another and applied most changes suggested here.

@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 Oct 6, 2022
@alice-i-cecile
Copy link
Member

Closing in favor of #6177.

@bzm3r bzm3r deleted the ortho-proj-docs branch October 6, 2022 14:40
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 S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants