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

Fill out Material documentation and clarify render_priority and next_pass sorting #83931

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

clayjohn
Copy link
Member

Partially address: #83774 (we still need to add more detail to the manual)

@clayjohn clayjohn added bug topic:rendering documentation cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Oct 25, 2023
@clayjohn clayjohn added this to the 4.2 milestone Oct 25, 2023
@clayjohn clayjohn requested a review from a team as a code owner October 25, 2023 08:52
@akien-mga akien-mga changed the title Fill out Material documentation and clarify render_priority and next_pass sorting Fill out Material documentation and clarify render_priority and next_pass sorting Oct 25, 2023
[b]Note:[/b] This only applies to [StandardMaterial3D]s and [ShaderMaterial]s with type "Spatial".
[b]Note:[/b] This only applies to sorting of transparent objects. This will not impact how transparent objects are sorted relative to opaque objects. This is because opaque objects are not sorted, while transparent objects are sorted from back to front (subject to priority).
[b]Note:[/b] This will not impact how transparent objects are sorted relative to opaque objects or how dynamic meshes will be sorted relative to other opaque meshes. This is because all transparent objects are drawn after all opaque objects and all dynamic opaque meshes are drawn before other opaque meshes.
Copy link
Member

@AThousandShips AThousandShips Oct 25, 2023

Choose a reason for hiding this comment

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

Suggested change
[b]Note:[/b] This will not impact how transparent objects are sorted relative to opaque objects or how dynamic meshes will be sorted relative to other opaque meshes. This is because all transparent objects are drawn after all opaque objects and all dynamic opaque meshes are drawn before other opaque meshes.
[b]Note:[/b] This will not affect how transparent objects are sorted relative to opaque objects, or how dynamic meshes will be sorted relative to other opaque meshes. This is because all transparent objects are drawn after all opaque objects and all dynamic opaque meshes are drawn before other opaque meshes.

Small style nitpick (didn't just correct the spelling)

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer "impact" and the comma is incorrect. Thanks anyways!

Copy link
Member

@AThousandShips AThousandShips Oct 25, 2023

Choose a reason for hiding this comment

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

The comma breaks up the long sentence, but impact/affect is preference based

But to me the current sentence is too long and needs a pause, the comma is correct (I'm not assuming an oxford comma incorrectly) but a matter of preference

Copy link
Member

@AThousandShips AThousandShips Oct 25, 2023

Choose a reason for hiding this comment

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

Generally speaking "impact" is considered to be more powerful than "affect" and generally considered negative (for example docs regularly use it to refer to performance), but it's a matter of preference

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

We discussed following one of the existing patterns for the first sentences of virtual methods, which may still be worth doing. But this should be a great addition already.

@YuriSizov YuriSizov merged commit d3e9033 into godotengine:master Nov 6, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Let's go forwards with this, additional style improvements can be done in a follow-up. Thanks!

@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. (Including some changes from previous PRs updating the descriptions, since they were causing conflicts and we're invalidating translations anyway.)

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.

4 participants