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

feat: fetch asset bundle textures for scenes #3280

Merged
merged 11 commits into from
Feb 18, 2025
Merged

Conversation

dalkia
Copy link
Collaborator

@dalkia dalkia commented Feb 3, 2025

What does this PR change?

Low hanging fruit that we could use for memory savings. The ABConverter already converts textures, so we could get those directly to get the compressed versions of them

Using this image as reference

bafkreigtdgo3eqskps45xwkl4zby4fldpdwt6ksybbjhueowxczdsqp42y

With AB (BC7 compression, 342KB in memory):
image

Without (ARGB32 compression, 1.3MB in memory):
image

Other examples:

image

image

image

Since the ABs are already compressed, make sense to use them when possible

How to test the changes?

  1. Launch the explorer
  2. Teleport to Genesis Plaza and walk around the main functionality. Look for irregularities in the textures (play the basket game, teleport back using the pipe, for example)

Our Code Review Standards

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

@dalkia dalkia marked this pull request as ready for review February 7, 2025 19:00
@dalkia dalkia requested review from a team as code owners February 7, 2025 19:00
@nearnshaw
Copy link
Member

Question: when the asset bundle process compresses textures, does it force a maximum texture size? like max 512x512?

If so that could be problematic in some scenes that might see a visible degradation, text in an AI image could turn illegible in some cases. And creators would have no way to anticipate that change until the scene is in production.

Copy link
Contributor

@anicalbano anicalbano left a comment

Choose a reason for hiding this comment

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

✔️ PR reviewed and approved by QA following instructions. Might seem on the evidence that the textures are a little lower but it's due to the recording quality.

Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:

  • Log In/Log Out
  • Backpack and wearables in world
  • Emotes in world and in backpack
  • Teleport with map/coordinates/Jump In
  • Chat and multiplayer
  • Profile card
  • Camera
  • Skybox
  • Settings

(Let me know if it needs re-testing due to Nico's comment)

capture_250207_170424

07.02.2025_17.04.35_REC.mp4

@nearnshaw
Copy link
Member

@anicalbano it's hard to tell w the video quality, but the images on the museum in the ship-shaped building on the North of Genesis Plaza, those looked alright? they didn't lose significant quality?

@dalkia
Copy link
Collaborator Author

dalkia commented Feb 7, 2025

@nearnshaw you are correct Nico, textures resolutions are resized at a max of 512. We can definitely revisit if necessary.

Besides the obvious file size reasons, I guess that the decision was taken considering the docs recomendations.

Keep in mind that this PR solves enconding as well. Si even if we decide to increase the resolution limit, the enconding will help us in memory savings as well. As they Will be BC7 and not the UnityWebTexture default of ARGB32.

On the other hand, we could also resize in local development, to make the result look the same before they are even assets bundles.

@dalkia
Copy link
Collaborator Author

dalkia commented Feb 14, 2025

@anicalbano it's hard to tell w the video quality, but the images on the museum in the ship-shaped building on the North of Genesis Plaza, those looked alright? they didn't lose significant quality?

Hey @nearnshaw! Just to let you know, before we merge this, we have discussed with Aga that we could test and increase the texture cap size to 1024.

Using GP as reference, we saw an increase of only 40MB in memory. So, its worth the try

image

@dalkia dalkia enabled auto-merge (squash) February 18, 2025 10:49
@dalkia dalkia merged commit 510e3f5 into dev Feb 18, 2025
5 checks passed
@dalkia dalkia deleted the feat/asset-bundle-textures branch February 18, 2025 13:20
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