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 missing RenderingDevice method descriptions #80716

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

ARez2
Copy link
Contributor

@ARez2 ARez2 commented Aug 17, 2023

I went through the RenderingDevice.xml file and added all the missing descriptions of methods. I also updated some descriptions to be more complete.

This is in an effort to become more familiar with the Godot source code, specifically compute shaders.

@ARez2 ARez2 requested a review from a team as a code owner August 17, 2023 12:49
@clayjohn clayjohn added this to the 4.2 milestone Aug 17, 2023
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.

Thank you for your contribution! Some phrasing and grammar details

doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
@ARez2 ARez2 force-pushed the add-rd-method-descriptions branch 2 times, most recently from 0d948ef to 1740366 Compare August 17, 2023 13:06
@ARez2
Copy link
Contributor Author

ARez2 commented Aug 17, 2023

Thanks for the quick review @AThousandShips . I fixed those issues and committed your suggestions. I also squashed the suggestion commit back into one commit :D

@AThousandShips
Copy link
Member

Great! I'm not exactly sure of the exact wordings and someone else will come around as well to review it, just added some suggestions on things where I thought it'd help

@AThousandShips AThousandShips requested a review from a team August 17, 2023 13:08
@ARez2
Copy link
Contributor Author

ARez2 commented Aug 17, 2023

Yes those were some good suggestions. Especially the "ranging" stuff ^^ Also my bad for having a typo in the first place. I forgot to set up my pre commit hooks properly.

Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

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

Suggested some things I randomly noticed

doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
@ARez2 ARez2 force-pushed the add-rd-method-descriptions branch from 6e58819 to d57eb81 Compare August 17, 2023 14:12
@ARez2
Copy link
Contributor Author

ARez2 commented Aug 17, 2023

Alright added all your suggestions too @RedMser . Thanks for looking over it ^^

doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
doc/classes/RenderingDevice.xml Outdated Show resolved Hide resolved
@ARez2 ARez2 force-pushed the add-rd-method-descriptions branch from 176a89b to e398533 Compare August 17, 2023 15:06
@ARez2
Copy link
Contributor Author

ARez2 commented Aug 17, 2023

Alright, once again merged everything together. I also removed the double space after "clearing" (as mentioned by @akien-mga )

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! Good work

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 29, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I'd prefer "Prints an error" as opposed to "Throws an error", as Godot doesn't use exceptions.

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@ARez2 ARez2 force-pushed the add-rd-method-descriptions branch from e0058b1 to 41daf29 Compare August 29, 2023 10:30
@ARez2 ARez2 force-pushed the add-rd-method-descriptions branch from 41daf29 to bd4d430 Compare August 29, 2023 10:32
@ARez2
Copy link
Contributor Author

ARez2 commented Aug 29, 2023

Alright! I think I got everything right now. When trying to commit the changes @Calinou suggested, I must have messed something up and had a bunch of other commits in my branch history.
It also had created a "Merge branch 'master' of https://github.com/ARez2/godot into add-rd-method-descriptions" which I am not sure why it did. So I was not sure what exactly to do in the rebase. I did multiple git rebase -i upstream/master before, but this time in the editor window that openend, there weren't 2 commits, but only that one "Add missing RenderingDevice method descriptions" commit. So I just picked that one and that seemed to have rerolled the branch commit history to the correct state.

TLDR: My faults are fixed now and everything should be correct now.

@akien-mga akien-mga merged commit bec94a6 into godotengine:master Aug 29, 2023
@ARez2 ARez2 deleted the add-rd-method-descriptions branch August 29, 2023 10:58
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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.

7 participants