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

Improve documentation for CameraAttributesPhysical.exposure_shutter_speed #85599

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

addmix
Copy link
Contributor

@addmix addmix commented Dec 1, 2023

Fixes #85601

@addmix addmix requested a review from a team as a code owner December 1, 2023 09:28
@addmix addmix closed this Dec 1, 2023
@addmix addmix reopened this Dec 1, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Dec 1, 2023
AThousandShips

This comment was marked as outdated.

@AThousandShips AThousandShips dismissed their stale review December 1, 2023 10:11

My bad got it backwards from the original

@Mickeon
Copy link
Contributor

Mickeon commented Dec 1, 2023

I like the wording here. "Allow" over "Let in" is less of a manner of speech, and more accessible.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

From the property limits and suffix this change is correct

doc/classes/CameraAttributesPhysical.xml Outdated Show resolved Hide resolved
@AThousandShips AThousandShips modified the milestones: 4.x, 4.3 Dec 1, 2023
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Dec 1, 2023
@addmix
Copy link
Contributor Author

addmix commented Dec 1, 2023

This is incorrect, the shutter speed is in seconds, not in traditional shutter steps like a camera, the current description is correct

A camera uses shutter speeds in fractions of a second, so 500 is 1/500 of a second, but with Godot the entered value would be "1/500" not "500"

You had me second guessing myself here. Had to scramble to test it again to make sure I wasn't doing something stupid 😅

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I reviewed the wording and it looks good. Did not check if the math is flipped, but the conversation above seems to conclude that the math is correct.

@fire fire requested a review from a team December 1, 2023 18:04
@clayjohn
Copy link
Member

clayjohn commented Dec 1, 2023

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@addmix addmix force-pushed the CameraAttributesPhysical-docs branch from 54d426d to 3894fb2 Compare December 1, 2023 20:16
@addmix
Copy link
Contributor Author

addmix commented Dec 1, 2023

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

Done.

@addmix addmix force-pushed the CameraAttributesPhysical-docs branch from 3894fb2 to 28f5f90 Compare December 1, 2023 22:10
@YuriSizov YuriSizov changed the title Update CameraAttributesPhysical.xml Improve documentation for CameraAttributesPhysical.exposure_shutter_speed Dec 4, 2023
@YuriSizov
Copy link
Contributor

Could you please amend your commit message to be a bit more descriptive? The current title of this PR is a good option. And you also don't need to include reviewers as co-authors, so that part of your commit message can be removed.

@addmix addmix force-pushed the CameraAttributesPhysical-docs branch from 28f5f90 to 1fdc1ea Compare December 5, 2023 00:34
@addmix
Copy link
Contributor Author

addmix commented Dec 5, 2023

Could you please amend your commit message to be a bit more descriptive? The current title of this PR is a good option. And you also don't need to include reviewers as co-authors, so that part of your commit message can be removed.

My mistake. I thought I had already done it.

@addmix addmix force-pushed the CameraAttributesPhysical-docs branch from 1fdc1ea to b7b0022 Compare December 5, 2023 00:35
@akien-mga akien-mga merged commit c4f872e into godotengine:master Dec 5, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CameraAttributesPhysical.exposure_shutter_speed description is incorrect.
7 participants