gltf iridescence #309
gltf iridescence #309
Conversation
WalkthroughThe pull request includes updates to the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
.github/workflows/release.yml (1)
5-16: Consider adding environment protection rules for releasesWhile the permissions are correctly scoped, consider enhancing security by:
- Adding environment protection rules for production releases
- Restricting the workflow to specific branches (e.g., main/master)
- Adding required reviewers for manual dispatches
package.json (2)
23-24: Consider enhancing the release scriptThe new release script using
bumppis minimal. Consider enhancing it to:
- Run tests before release
- Ensure clean working directory
- Generate changelog
Example enhancement:
- "release": "bumpp -r" + "release": "pnpm test && pnpm build && bumpp -r && pnpm changeset"
Line range hint
1-85: Consider separating release automation from feature changesThe PR title suggests GLTF iridescence feature changes, but this file contains significant version management and release automation changes. Consider:
- Splitting these into separate PRs for clearer change management
- Documenting the rationale for the version bump from 1.2.0-beta.0 to 1.3.9
- Adding release notes or changelog entries describing the changes
packages/shaderlab/src/shaders/PBR.gs (2)
44-48: Document the components of material_IridescenceInfo Vector4.While the iridescence properties are well-structured and follow the established patterns, it would be helpful to document what each component of the
material_IridescenceInfoVector4 represents (x, y, z, w) to improve maintainability and usability.
Line range hint
44-71: Consider enhancing documentation and standards compliance.While the iridescence implementation is well-structured, consider:
- Adding comments or documentation explaining the thin film iridescence implementation details and equations used
- Verifying that the implementation aligns with standard PBR practices (e.g., glTF's KHR_materials_iridescence extension)
rollup.config.mjs (1)
159-159: Consider explicit build stages for release workflow.While the change is correct, prioritizing UMD builds through sorting suggests a build order dependency. Consider implementing explicit build stages in your release workflow instead of relying on sort order.
This would:
- Make build dependencies more explicit
- Improve reliability of the release process
- Make it easier to parallelize independent builds
packages/gizmo/src/Group.ts (1)
336-342: Simplify the conditional structureThe code structure can be simplified by combining the conditions and removing the unnecessary else block.
Consider this cleaner approach:
- if (renderer instanceof ParticleRenderer) { - // Ignore particle bounding box. - continue; - } else { - isEffective = true; - BoundingBox.merge(tempBoundBox, renderers[j].bounds, tempBoundBox); - } + if (!(renderer instanceof ParticleRenderer)) { + isEffective = true; + BoundingBox.merge(tempBoundBox, renderers[j].bounds, tempBoundBox); + }🧰 Tools
🪛 Biome
[error] 341-341: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
packages/shaderlab/src/shaders/Shadow.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/ReflectionLobe.glslis excluded by!**/*.glsl
📒 Files selected for processing (27)
.github/workflows/release.yml(1 hunks).npmrc(0 hunks)package.json(3 hunks)packages/auxiliary-lines/package.json(1 hunks)packages/controls/package.json(1 hunks)packages/custom-gltf-parser/package.json(1 hunks)packages/custom-material/package.json(1 hunks)packages/draco/package.json(1 hunks)packages/dynamic-bone/package.json(1 hunks)packages/framebuffer-picker/package.json(1 hunks)packages/galacean-engine-toolkit/package.json(1 hunks)packages/geometry-sketch/package.json(1 hunks)packages/gizmo/package.json(1 hunks)packages/gizmo/src/Group.ts(2 hunks)packages/input-logger/package.json(1 hunks)packages/lines/package.json(1 hunks)packages/navigation-gizmo/package.json(1 hunks)packages/outline/package.json(1 hunks)packages/shaderlab/package.json(1 hunks)packages/shaderlab/src/shaders/PBR.gs(2 hunks)packages/skeleton-viewer/package.json(1 hunks)packages/stats/package.json(1 hunks)packages/tween/package.json(1 hunks)packages/way-point/package.json(1 hunks)packages/xr/package.json(1 hunks)rollup.config.mjs(1 hunks)tests/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- .npmrc
✅ Files skipped from review due to trivial changes (20)
- packages/auxiliary-lines/package.json
- packages/controls/package.json
- packages/custom-gltf-parser/package.json
- packages/custom-material/package.json
- packages/dynamic-bone/package.json
- packages/framebuffer-picker/package.json
- packages/galacean-engine-toolkit/package.json
- packages/geometry-sketch/package.json
- packages/gizmo/package.json
- packages/input-logger/package.json
- packages/lines/package.json
- packages/navigation-gizmo/package.json
- packages/outline/package.json
- packages/shaderlab/package.json
- packages/skeleton-viewer/package.json
- packages/stats/package.json
- packages/tween/package.json
- packages/way-point/package.json
- packages/xr/package.json
- tests/package.json
🧰 Additional context used
🪛 Biome
packages/gizmo/src/Group.ts
[error] 341-341: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🪛 eslint
rollup.config.mjs
[error] 156-156: Insert ;
(prettier/prettier)
[error] 157-157: Insert ;
(prettier/prettier)
🔇 Additional comments (10)
packages/draco/package.json (2)
8-10: LGTM! Repository field addition is correct.
The repository field addition follows npm best practices and correctly points to the project repository.
3-3: Verify the significant version bump from 1.3.2 to 1.3.9
The version increment of 7 minor versions seems substantial for what appears to be a single GLTF iridescence feature. Please verify if:
- This includes breaking changes requiring a major version bump instead
- This version aligns with the scope of changes across all packages
✅ Verification successful
Let me generate additional scripts to gather more context about the changes between these versions.
Based on the verification results, I can now provide the final response:
Version bump to 1.3.9 appears to be part of coordinated release
The version bump is consistent across all packages in the monorepo, indicating this is part of a coordinated release strategy rather than an isolated change. While the increment is significant:
- All 21 packages are being updated to version 1.3.9 in sync
- No breaking changes were found in the commit history
- The draco package itself shows minimal changes, suggesting it's part of the broader release cycle rather than containing major changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes and verify version consistency
# Test 1: Look for breaking changes in commit history
echo "Checking for breaking changes in commit messages..."
git log --oneline | grep -i "breaking"
# Test 2: Verify version consistency across all package.json files
echo "Checking version consistency across packages..."
fd package.json --exec grep -H '"version":'
Length of output: 1541
Script:
#!/bin/bash
# Check the commit history and changes between the versions
echo "Checking commits and changes between versions..."
# Look for version bump commits
git log --oneline | grep -i "version" -A 5
# Check for changes in the draco package
echo -e "\nChecking changes in draco package..."
git log --oneline -- packages/draco/
# Look for CHANGELOG entries
echo -e "\nChecking CHANGELOG entries..."
fd CHANGELOG -t f --exec cat {}
Length of output: 433
.github/workflows/release.yml (1)
39-39: Verify the custom publish action usage
Let's verify if this custom action is used consistently across the organization's workflows.
✅ Verification successful
Custom publish action usage appears to be consistent
The gz65555/publish@v0.1.5 action is used only once in the repository's workflows, specifically in the release workflow. The action is used in a release context with proper environment configuration including NPM token and Oasis backend settings. No other publish-related actions are being used, indicating a consistent approach to publishing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the custom publish action across all workflow files
echo "Searching for publish action usage patterns..."
fd -e yml -e yaml . .github/workflows/ -x rg -l "uses: gz65555/publish@"
Length of output: 207
Script:
#!/bin/bash
# Let's check the specific version usage and context in the release.yml
echo "Checking publish action version and context in release.yml:"
rg -A 5 -B 5 "uses: gz65555/publish@" .github/workflows/release.yml
# Let's also check if there are any other publish-related actions being used
echo -e "\nChecking for other publish-related actions:"
rg "uses:.+publish" .github/workflows/
Length of output: 788
package.json (2)
41-41: LGTM: Appropriate dev dependency addition
The addition of bumpp as a dev dependency aligns with the new release script functionality.
3-4: Verify version bump consistency across all packages
The version bump from 1.2.0-beta.0 to 1.3.9 is significant. The addition of packageManager field is good for ensuring consistent tooling.
Let's verify version consistency across all package.json files:
✅ Verification successful
Version consistency verified across all packages
All packages within the @galacean/engine-toolkit monorepo are correctly synchronized at version 1.3.9, including:
- Root package
- All toolkit packages (controls, draco, gizmo, etc.)
- Test package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version consistency across all package.json files
# Expected: All packages should be at version 1.3.9
echo "Checking package versions..."
find . -name "package.json" -exec jq -r '. | select(.version != null) | .name + ": " + .version' {} \;
Length of output: 4838
packages/shaderlab/src/shaders/PBR.gs (1)
69-71: LGTM! Well-structured macro definitions.
The new iridescence-related macros follow the established patterns and provide a complete set of conditional checks for the feature and its textures.
rollup.config.mjs (2)
153-154: LGTM! Good refactoring.
The extraction of configuration generation into a separate variable improves code readability and enables further processing of the configurations.
153-159: Verify integration with release workflow.
Since these changes affect build order and are part of release automation, let's verify the integration with the release workflow.
✅ Verification successful
Build configuration changes are properly integrated with release workflow
The changes to the build order in rollup.config.mjs are well-integrated with the release workflow:
- The release workflow uses
pnpm b:allwhich executes the full build process including the modified rollup configuration - The build script is properly defined in package.json and is executed before the publish step
- The UMD build prioritization in rollup config will work as expected since it's part of the same build chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify release workflow configuration and build script usage
# Check if release workflow uses pnpm b:all
echo "Checking release workflow configuration..."
rg -A 5 "pnpm b:all" .github/workflows/release.yml
# Check if there are any UMD-specific build steps
echo "Checking for UMD-specific build steps..."
rg "umd" .github/workflows/release.yml
Length of output: 441
Script:
#!/bin/bash
# Let's verify the build script configuration in package.json
echo "Checking build script configuration..."
rg "\"b:all\"" package.json -A 3
# Check if there are any other build-related scripts
echo "Checking other build scripts..."
rg "\"build" package.json
# Let's also check the release workflow file content
echo "Checking release workflow content..."
cat .github/workflows/release.yml
Length of output: 1619
🧰 Tools
🪛 eslint
[error] 156-156: Insert ;
(prettier/prettier)
[error] 157-157: Insert ;
(prettier/prettier)
packages/gizmo/src/Group.ts (2)
1-1: LGTM: Import statement updated correctly
The addition of ParticleRenderer to the imports is necessary for the new type checking, and the imports maintain good alphabetical ordering.
336-339: Verify the exclusion of particle renderer bounds
The code now explicitly ignores particle renderer bounds during bounding box calculations. This could have implications for the overall behavior of the gizmo system.
Please confirm:
- Is this exclusion intentional and documented?
- What is the rationale behind ignoring particle renderer bounds?
- Are there any side effects to consider?
Let's check for any related documentation or tests:
✅ Verification successful
From the context found, I can now generate a final response based on the code surrounding the ParticleRenderer check. The code shows that this is part of a bounding box calculation where particle renderers are explicitly excluded, with other renderers contributing to the bounding box merge operation.
Particle renderer bounds exclusion is intentional and reasonable
The code context reveals this is part of a bounding box calculation where:
- Only active entities in the hierarchy are considered
- Particle renderers are explicitly excluded with a clear comment
- Other renderers contribute their bounds to a merged bounding box
This exclusion makes sense because:
- Particle systems typically have dynamic, constantly changing bounds
- Including particle bounds could lead to unstable or overly large bounding boxes
- The comment "Ignore particle bounding box" indicates this was a deliberate design decision
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation about particle renderer handling
echo "Searching for documentation about particle renderer handling..."
rg -i "particle.*renderer.*bound" --type md
rg -i "particle.*bound.*box" --type md
# Search for related tests
echo "Searching for related tests..."
fd -e test.ts -e spec.ts -x rg -l "ParticleRenderer.*bound"
Length of output: 356
Script:
#!/bin/bash
# Let's look for the ParticleRenderer class definition and its usage context
echo "Searching for ParticleRenderer class definition..."
ast-grep --pattern 'class ParticleRenderer {
$$$
}'
# Look for any comments or documentation near the particle renderer bounds check
echo "Searching for context around particle renderer bounds check..."
rg -B 5 -A 5 "if \(renderer instanceof ParticleRenderer\)"
# Search for any tests or code that might explain the behavior
echo "Searching for ParticleRenderer usage in tests..."
rg "ParticleRenderer" -A 5 -B 5 --type typescript
# Look for any commit messages that might explain this change
echo "Searching git history for relevant commits..."
git log -p --all -S "ParticleRenderer.*bound"
Length of output: 1518
.github/workflows/release.yml
Outdated
| - name: Use Node.js LTS | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '>=22.6.0' | ||
| registry-url: https://registry.npmjs.org/ | ||
| cache: pnpm | ||
|
|
There was a problem hiding this comment.
Revise Node.js version requirement and add security checks
- Node.js 22.6.0 is too recent (released in Oct 2023) and is not LTS. Consider using the current LTS version (20.x) instead.
- Add a step to verify lockfile integrity:
- name: Use Node.js LTS
uses: actions/setup-node@v4
with:
- node-version: '>=22.6.0'
+ node-version: '20.x'
registry-url: https://registry.npmjs.org/
cache: pnpm
+ - name: Verify lockfile
+ run: pnpm install --frozen-lockfile📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Use Node.js LTS | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '>=22.6.0' | |
| registry-url: https://registry.npmjs.org/ | |
| cache: pnpm | |
| - name: Use Node.js LTS | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '20.x' | |
| registry-url: https://registry.npmjs.org/ | |
| cache: pnpm | |
| - name: Verify lockfile | |
| run: pnpm install --frozen-lockfile |
.github/workflows/release.yml
Outdated
| - name: Build | ||
| run: pnpm b:all | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add build verification step
The current build step doesn't verify the output. Consider adding checks:
- name: Build
run: pnpm b:all
+ - name: Verify build artifacts
+ run: |
+ if [ ! -d "dist" ] || [ -z "$(ls -A dist)" ]; then
+ echo "Build artifacts are missing"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build | |
| run: pnpm b:all | |
| - name: Build | |
| run: pnpm b:all | |
| - name: Verify build artifacts | |
| run: | | |
| if [ ! -d "dist" ] || [ -z "$(ls -A dist)" ]; then | |
| echo "Build artifacts are missing" | |
| exit 1 | |
| fi | |
.github/workflows/release.yml
Outdated
| - name: Release current monorepo | ||
| uses: gz65555/publish@v0.1.5 | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}} | ||
| NPM_CONFIG_PROVENANCE: true | ||
| OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload | ||
| OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}} | ||
| OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}} |
There was a problem hiding this comment.
Add version validation and enhance release process
The release process could be improved with additional checks:
- Validate that package versions match the git tag
- Generate/update changelog
- Consider using established release actions like
semantic-releaseinstead of a custom action
Also, sensitive environment variables should use secrets masking:
- name: Release current monorepo
uses: gz65555/publish@v0.1.5
env:
NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
NPM_CONFIG_PROVENANCE: true
- OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
+ OASISBE_UPLOAD_URL: ${{secrets.OASISBE_UPLOAD_URL}}
OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Release current monorepo | |
| uses: gz65555/publish@v0.1.5 | |
| env: | |
| NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}} | |
| NPM_CONFIG_PROVENANCE: true | |
| OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload | |
| OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}} | |
| OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}} | |
| - name: Release current monorepo | |
| uses: gz65555/publish@v0.1.5 | |
| env: | |
| NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}} | |
| NPM_CONFIG_PROVENANCE: true | |
| OASISBE_UPLOAD_URL: ${{secrets.OASISBE_UPLOAD_URL}} | |
| OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}} | |
| OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}} |
rollup.config.mjs
Outdated
| builderConfigs.sort((_, b) => { | ||
| if (b.output.format === "umd") return -1 | ||
| }) |
There was a problem hiding this comment.
Fix incomplete sort function implementation.
The current sort function has several issues:
- Only handles the case where the second item is UMD
- Missing return value for other cases
- Missing semicolons (style issue)
This could lead to unpredictable build order and potential failures.
Apply this fix:
-builderConfigs.sort((_, b) => {
- if (b.output.format === "umd") return -1
-})
+builderConfigs.sort((a, b) => {
+ if (a.output.format === "umd" && b.output.format !== "umd") return -1;
+ if (b.output.format === "umd" && a.output.format !== "umd") return 1;
+ return 0;
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| builderConfigs.sort((_, b) => { | |
| if (b.output.format === "umd") return -1 | |
| }) | |
| builderConfigs.sort((a, b) => { | |
| if (a.output.format === "umd" && b.output.format !== "umd") return -1; | |
| if (b.output.format === "umd" && a.output.format !== "umd") return 1; | |
| return 0; | |
| }); |
🧰 Tools
🪛 eslint
[error] 156-156: Insert ;
(prettier/prettier)
[error] 157-157: Insert ;
(prettier/prettier)
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
New Features
Version Updates
1.3.9to1.4.0-alpha.1, signaling a pre-release status across the toolkit.