changed: Remove asset_path field from ScriptAsset. #450
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.
Congrats on getting 0.14.0 and 0.15.0 releases out! Very exciting.
Summary
I removed the
asset_path
field fromScriptAsset
and altered thescript_asset:asset_path()
code to use the path present in the handle or use the path given by theAssetServer
based on the handle. This preserves the prior behavior and the tests pass.Why?
I feel like the asset path is not a property of the asset. It's a property of the handle to the asset, which may or may not have an
AssetPath
.The asset to me is like the contents of a file. Some files may declare what they believe their file path to be—and they can be wrong; most files don't. The file's path is an external concern likewise for an asset path.
Also I worry that having the
AssetPath
as a field invites confusion. I know you can summon a path withAssetPath::default()
but that is effectively wrong or invalid. A field ofOption<AssetPath>
would be less prone to confusion, but I don't think it's warranted or necessary as evidenced by this implementation.Another reason to prefer not having an
AssetPath
is there are many valid uses that do not have an asset path. Suppose an app uses a string given by the user at runtime to create aScriptAsset
. There is no asset path in that case. Procedurally generated cases have similar uses.Alternatives
One could use
Option<AssetPath>
as the field to avoid forcing the user to provide a path or provide a default/invalid path.Testing
I ran the tests. Found the
script_asset:asset_path()
test failing, which I believe that function is the reason the field exists. I changed its implementation to offer the same results as before but without the field.