Skip to content

Fix AssetRegistry URL lookups by type#8842

Open
puneetdixit200 wants to merge 1 commit into
playcanvas:mainfrom
puneetdixit200:fix-asset-registry-url-type
Open

Fix AssetRegistry URL lookups by type#8842
puneetdixit200 wants to merge 1 commit into
playcanvas:mainfrom
puneetdixit200:fix-asset-registry-url-type

Conversation

@puneetdixit200

Copy link
Copy Markdown

Description

Store all assets registered for the same file URL so AssetRegistry#getByUrl(url, type) can return the asset matching a requested type. Untyped getByUrl(url) keeps the existing last-added behavior for compatibility.

Fixes #8489

Testing

  • npm test -- test/framework/asset/asset-registry.test.mjs --grep "#getByUrl"
  • npm test -- test/framework/asset/asset-registry.test.mjs
  • npm run lint
  • npm run build:types
  • npm run test:types
  • git diff --check

Checklist

  • I have read the contributing guidelines
  • My code follows the project's coding standards
  • This PR focuses on a single change

}

let result;
assets.forEach((asset) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please replace with a simple for..of loop, with early return inside of it.

@mvaligursky mvaligursky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Automated PR review — posted on my behalf by Claude Code (Opus 4.8). Not a human review; flagging for awareness.


Looks correct and well-tested. Untyped getByUrl(url) preserves the last-added semantics (Set iteration is insertion order, forEach keeps overwriting result), all external callers go through the public method, and the Set-per-url removal is actually more correct than the old map — removing one of two assets sharing a url no longer wipes the other. Nothing blocking. Two minor notes:

  1. getByUrl iterates the whole set with no early exit and returns the last match. That's intentional to keep untyped compat, but for the typed branch a for...of with an early break would be cheaper and clearer about intent (and would return the first match of that type). Sets are tiny so the perf difference is negligible — purely a readability nit.

  2. Pre-existing (not introduced here), but now more relevant: the registry indexes by asset.file.url at add time and never re-indexes if file.url changes later (remove then uses the current url, so a changed-url asset leaves a stale set entry). Harmless single-asset before; with multiple assets per url now supported, a stale entry could shadow a live one. Out of scope for this PR — just flagging in case it's worth a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AssetRegistry.getByUrl() returns wrong asset type for same URL (texture vs cubemap)

4 participants