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.

Sign up for GitHub

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

Fix reource load issue in local downloaded npm package #2436

Merged
merged 41 commits into from
Nov 15, 2024
Merged

Fix reource load issue in local downloaded npm package #2436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
5a26f3d
fix(shader-lab): compatible with empty macro
Sway007 Sep 20, 2023
5ecc318
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 20, 2023
cafc24f
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 20, 2023
dc69489
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 21, 2023
221b7b6
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 21, 2023
0d45d9c
fix(shader-lab): add break and continue syntax
Sway007 Sep 21, 2023
0f14c3f
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 27, 2023
8871d9b
fix: typo
Sway007 Oct 11, 2023
3b4ffd7
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 1, 2023
f649a58
fix(shader-lab): Make usepass compatible with buitin shader
Sway007 Nov 1, 2023
598fc56
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 2, 2023
e33a66f
fix(shader-lab): compatible with no varying variable
Sway007 Nov 2, 2023
41ef06f
feat(shader-lab): detect mismatch return type
Sway007 Nov 6, 2023
b5214fc
fix(shader-lab): renderState assignment
Sway007 Nov 7, 2023
f36ce02
feat: extend material loader data type
Sway007 Nov 13, 2023
ff8b7c2
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 13, 2023
634236f
fix(shader-lab): glsl type pattern
Sway007 Nov 13, 2023
91e6fa4
fix(shader-lab): glsl type pattern
Sway007 Nov 13, 2023
3932448
fix: switch case break
Sway007 Nov 15, 2023
671cace
fix: array index loss
Sway007 Jun 11, 2024
92b972e
feat: merge
Sway007 Jun 11, 2024
9226d38
fix: test-case
Sway007 Jun 11, 2024
ff6a69a
fix: test-case
Sway007 Jun 12, 2024
83b9ca2
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Jun 13, 2024
f510c2a
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Jul 15, 2024
3bcaeb0
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 5, 2024
79a7a0c
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 5, 2024
fa7131c
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 6, 2024
d4ea485
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 26, 2024
2af1c65
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 10, 2024
a828991
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 13, 2024
24cb887
fix: new scope when parse pass
Sway007 Oct 10, 2024
ab8bf3b
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Oct 22, 2024
fd870a0
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 7, 2024
daf6521
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 14, 2024
b3f8e57
fix: resouce manager load local resource
Sway007 Nov 14, 2024
809a3cb
fix: resouce manager load local resource
Sway007 Nov 14, 2024
1607698
fix: resouce manager load local resource
Sway007 Nov 14, 2024
7e4a771
fix: resouce manager load local resource
Sway007 Nov 14, 2024
c76cba5
feat: code clean
Sway007 Nov 15, 2024
ef32bc8
feat: resourcemanage test case
Sway007 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions packages/core/src/asset/ResourceManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ export class ResourceManager {
let assetBaseURL = baseUrl;
if (searchStr) {
const params = searchStr.split("&");
for (let i = 0; i < params.length; i++) {
for (let i = params.length - 1; i >= 0; i--) {
const param = params[i];
if (param.startsWith(`q=`)) {
queryPath = decodeURIComponent(param.split("=")[1]);
Expand Down Expand Up @@ -573,11 +573,9 @@ export class ResourceManager {
Logger.warn(`refId:${refId} is not find in this._editorResourceConfig.`);
return Promise.resolve(null);
}
const remoteUrl = resourceConfig.path;
const queryPath = new URL(remoteUrl).search;
let url = resourceConfig.virtualPath + queryPath;
let url = resourceConfig.virtualPath;
if (key) {
url += (url.indexOf("?") > -1 ? "&" : "?") + "q=" + key;
url += "?q=" + key;
}

promise = this.load<any>({
Expand Down
15 changes: 14 additions & 1 deletion tests/src/core/resource/ResourceManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AssetPromise, ResourceManager, Texture2D } from "@galacean/engine-core";
import { AssetPromise, AssetType, ResourceManager, Texture2D } from "@galacean/engine-core";
import { WebGLEngine } from "@galacean/engine-rhi-webgl";
import chai, { expect } from "chai";
import spies from "chai-spies";
Expand Down Expand Up @@ -81,4 +81,17 @@ describe("ResourceManager", () => {
chai.spy.restore(glTFLoader, "load");
});
});

describe("gltf subAsset load", () => {
it("invalid q case", async () => {
const loadRes = await engine.resourceManager.load({
// contains invalid q value cdn url.
url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json",
type: AssetType.Project
});
expect(loadRes).to.equal(undefined);
});

Comment on lines +85 to +93
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Test case needs improvement for reliability and clarity

Several issues with the current test implementation:

  1. Using an external CDN URL makes the test unreliable and dependent on network connectivity. Consider using a mock URL or local test fixture.
  2. The test description and implementation don't clearly document what constitutes an "invalid q value" or why undefined is the expected return value.
  3. The test case should be expanded to better validate the local npm package resource loading fix mentioned in the PR objectives.

Consider refactoring like this:

 describe("gltf subAsset load", () => {
-  it("invalid q case", async () => {
+  it("should handle invalid query parameters in resource URLs", async () => {
     const loadRes = await engine.resourceManager.load({
-      // contains invalid q value cdn url.
-      url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json",
+      url: "/test-assets/mock-project.json?q=invalid[query]",  // Use local test asset
       type: AssetType.Project
     });
+    // Document why undefined is expected
+    // Consider adding more assertions to validate the error handling
     expect(loadRes).to.equal(undefined);
   });

Committable suggestion skipped: line range outside the PR's diff.

// TODO: case for gltf loader load invalid q url, expect to throw
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implement the TODO test case before merging

Given that this PR addresses resource loading issues, the TODO test case for GLTF loader error handling should be implemented now rather than deferred. This test is important for validating the error handling behavior of the resource loading fix.

Would you like me to help implement the test case for GLTF loader error handling? Here's a suggested structure:

it("should throw error when loading GLTF with invalid query URL", async () => {
  try {
    await engine.resourceManager.load({
      url: "/test-assets/model.gltf?q=invalid[query]",
      type: AssetType.GLTF
    });
    expect.fail("Should have thrown an error");
  } catch (error) {
    expect(error).to.be.instanceOf(Error);
    expect(error.message).to.include("Invalid query parameter");
  }
});

});
Loading