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

fix: Add version for shared assets bundles #3096

Merged
merged 5 commits into from
Jan 16, 2025
Merged

Conversation

dalkia
Copy link
Collaborator

@dalkia dalkia commented Jan 10, 2025

What does this PR change?

  • Adds the version to the cacheable url flow of asset bundles
  • Adds test to verify that the correct CacheableURL is returned if two scenes request the same AB. If they are equal (or not, depending on versioning) the cache system is LoadSystemBase should return the correct asset accordingly

How to test the changes?

  1. Launch the explorer
  2. Do the happy path

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

Copy link
Contributor

github-actions bot commented Jan 10, 2025

badge

New build in progress, come back later!

Copy link
Member

@pravusjif pravusjif left a comment

Choose a reason for hiding this comment

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

I still think we keep blocking assets-sharing that could be perfectly shared, but maybe given the current scenario this is the best option?

Currently at least these 2 conditions have to be met for the same asset to be shared between different scenes:

  • The scenes have to be using the exact same asset file
  • The scenes have to be in the exact same “asset bundles conversion version”, given the assetBundlesBaseUrl.Append(new URLPath($"{version}/{hash}")); (this is something I think will RARELY happen)

@dalkia dalkia requested review from a team as code owners January 15, 2025 13:12
@dalkia
Copy link
Collaborator Author

dalkia commented Jan 15, 2025

I still think we keep blocking assets-sharing that could be perfectly shared, but maybe given the current scenario this is the best option?

Currently at least these 2 conditions have to be met for the same asset to be shared between different scenes:

  • The scenes have to be using the exact same asset file
  • The scenes have to be in the exact same “asset bundles conversion version”, given the assetBundlesBaseUrl.Append(new URLPath($"{version}/{hash}")); (this is something I think will RARELY happen)

We have to take a decision. If two assets have different version, I think that the safer option is not to share them. If the outdated one is broken, the new one may unintentional broken.

Copy link

@Ludmilafantaniella Ludmilafantaniella left a comment

Choose a reason for hiding this comment

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

🟢 Approved by QA. Sanity check performed on Windows and Mac:

  • LogIn/Log out using differents accounts
  • Backpack
  • Social Interactions
  • Multiplayer
  • Emotes functionality
  • Map Functionality
  • Camera, Camera Reel
  • Profile
  • Teleport using GP Carrousel, Coords and Map. Scenes/world visited: Pravus Olavra, Metadynelabs, Doll House, Seed, Casa Roustan

@dalkia dalkia merged commit 3da92eb into dev Jan 16, 2025
7 checks passed
@dalkia dalkia deleted the fix/add-version-for-shared-abs branch January 16, 2025 00:47
@dalkia dalkia mentioned this pull request Jan 17, 2025
dalkia added a commit that referenced this pull request Jan 20, 2025
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.

4 participants