-
-
Notifications
You must be signed in to change notification settings - Fork 310
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 pbr document #2523
base: main
Are you sure you want to change the base?
Add pbr document #2523
Conversation
WalkthroughThis pull request involves documentation changes for Physically Based Rendering (PBR) in the graphics shader section. The existing Changes
Sequence DiagramsequenceDiagram
participant User
participant PBRMaterial
participant Renderer
User->>PBRMaterial: Configure material properties
PBRMaterial->>PBRMaterial: Set metallic, roughness
PBRMaterial->>PBRMaterial: Set base color, emissive
PBRMaterial->>Renderer: Apply material settings
Renderer->>Renderer: Render with PBR techniques
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2523 +/- ##
==========================================
- Coverage 68.59% 68.56% -0.04%
==========================================
Files 957 957
Lines 100068 100068
Branches 8562 8562
==========================================
- Hits 68643 68612 -31
- Misses 31169 31200 +31
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/en/graphics/shader/builtins/pbr.mdx (2)
1-4
: Consider providing full English translationThe document currently mixes Chinese and English content. Consider providing a complete English version or maintaining separate language files for better accessibility to international users.
8-24
: Enhance parameter documentation with code examplesWhile the parameter documentation is comprehensive, adding code snippets demonstrating the usage of each parameter would make it more practical for developers. Consider adding examples like:
const material = new PBRMaterial(); material.metallic = 0.8; material.roughness = 0.2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/en/graphics/shader/builtins/pbr.md
(0 hunks)docs/en/graphics/shader/builtins/pbr.mdx
(1 hunks)docs/en/graphics/shader/builtins/thin.mdx
(0 hunks)
💤 Files with no reviewable changes (2)
- docs/en/graphics/shader/builtins/thin.mdx
- docs/en/graphics/shader/builtins/pbr.md
|
||
我们调节材质的金属度,可以发现,金属度越大,周围的环境越清晰,并且开始从白色纯色变成彩色。这是因为电介质(即金属度为 1 时)材质会将光线 100% 全部反射出物体表面,即反射出彩色的周边环境: | ||
|
||
<img src="https://gw.alipayobjects.com/zos/OasisHub/711f8b97-247c-465e-8cf2-4896b0c78534/metal.gif" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance image accessibility
While the documentation includes helpful visual examples, consider:
- Adding
alt
text to all images for screen readers - Ensuring images have appropriate contrast ratios
- Providing text descriptions of the visual changes shown in animations
Example improvement:
<Image
+ alt="Demonstration of metallic parameter effect showing increasing reflection of environment"
src="https://gw.alipayobjects.com/zos/OasisHub/711f8b97-247c-465e-8cf2-4896b0c78534/metal.gif"
/>
Also applies to: 49-51, 67-69, 89-91, 106-109, 120-122, 126-128, 142-145, 151-153
| [glossiness](/apis/core/#PBRMaterial-glossiness) | 光泽度。模拟光滑程度,与粗糙度相反。(注,只有关闭金属粗糙工作流才生效)。 | | ||
| [specularGlossinessTexture](/apis/core/#PBRMaterial-specularGlossinessTexture) | 高光光泽度纹理。搭配高光光泽度使用,是相乘的关系。 | | ||
|
||
> **注**:PBR 必须开启[环境光](/docs/graphics-light-ambient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Move critical requirements to the beginning
The note about PBR requiring ambient light is crucial for functionality but is currently placed at the end of the document. Consider moving this requirement to the beginning of the document and expanding it to explain why ambient light is necessary for PBR.
- 首先需要分别打开 Camera 下和场景中的 Opaque Texture: | ||
|
||
<Image | ||
src="https://mdn.alipayobjects.com/huamei_9ahbho/afts/img/A*_tBbRYNaBPMAAAAAAAAAAAAADgDwAQ/original" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Make prerequisites more prominent
The requirement to enable Opaque Texture in both Camera and scene settings is crucial for transmission to work. Consider moving these prerequisites to the beginning of the Transmission section and highlighting them in a warning callout, similar to how you've handled other important notes.
+<Callout type="warning">
+Prerequisites for Transmission:
+- Enable Opaque Texture in Camera settings
+- Enable Opaque Texture in scene settings
+</Callout>
Committable suggestion skipped: line range outside the PR's diff.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit