Fix duplicate Studio asset URI scheme#160
Merged
Merged
Conversation
3 tasks
The Display impl for AssetRef already prepends "rbxasset://" for Studio
and "rbxassetid://" for Cloud variants. The studio backend was packing
those prefixes inside the inner string, causing codegen to emit doubled
URLs like:
["test1.png"] = "rbxasset://rbxasset://.asphalt-name/<hash>.png"
For cloud-uploaded models/animations, return AssetRef::Cloud(id) so the
correct "rbxassetid://{id}" is produced. For locally synced files, drop
the "rbxasset://" prefix from the inner string so Display adds it once.
Also adds an ASPHALT_TEST env var that routes the sync folder to the
project directory instead of the Roblox Studio content path. This is
test-only plumbing that lets the integration test exercise sync studio
without requiring a Roblox Studio install. The common test harness
already sets this env var in Project::run().
Includes a regression test (studio_sync_emits_single_rbxasset_prefix)
that fails on the previous behavior.
5102258 to
3176b0a
Compare
Owner
|
Hey. Thanks for the fix, but I don't think we need these tests. Do you mind reducing your PR to the few lines of code necessary to fix us adding that prefix? |
Reduces PR jackTabsCode#160 to just the prefix fix in studio.rs as requested.
Contributor
Author
|
removed tests, just the minimal fix now |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
Displayimpl forAssetRefalready prependsrbxasset://forStudiovariants andrbxassetid://forCloudvariants:The studio backend was packing those prefixes inside the inner string as well, so codegen ended up emitting doubled URLs:
For models/animations with a lockfile entry (i.e. already cloud-uploaded), the result was even more confused:
Fix
rbxasset://prefix from the inner string ofAssetRef::Studio(...)soDisplayadds it once.AssetRef::Cloud(entry.asset_id)so the correctrbxassetid://{id}is produced viaDisplay.Test infrastructure
This PR also adds an
ASPHALT_TESTenv var that routes the studio sync folder toparams.project_dirinstead of the Roblox Studio content path. This is test-only plumbing — the existing common test harness (tests/common/mod.rs) already setsASPHALT_TEST=trueinProject::run(), but the studio backend ignored it. With the env var honored, integration tests forsync studiono longer require a Roblox Studio install.Regression test
Added
studio_sync_emits_single_rbxasset_prefixintests/sync.rs. It runsasphalt sync studioand asserts that the generatedoutput/assets.luau:rbxasset://.asphalt-test/(single prefix, correct format)rbxasset://rbxasset://rbxasset://rbxassetid://I verified the test fails on current
main(captured output:["test1.png"] = "rbxasset://rbxasset://.asphalt-...") and passes with the fix.Test plan
cargo buildclean on rc.5cargo test— 6/6 unit + 11/11 integration pass