-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Rename "labeled asset" to "subasset". #22666
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
base: main
Are you sure you want to change the base?
Conversation
eb04f58 to
322c93b
Compare
322c93b to
b2abd9a
Compare
ChristopherBiscardi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking this through and reading the PR I think this is the right direction. "Labels" as they currently exist do not semantically differ from the way Bevy uses "Names" (ex: the Name component). Having two distinct words for what is the same thing is awkward.
APIs like LoadedAsset::get_labeled are hard to defend compared to LoadedAsset::get_subasset (or a theoretical LoadedAsset::get_asset). The concept of "labels" today doesn't seem to add anything over "names", and we already use "names" in other places.
Additionally, the biggest counter-argument to this that I can see is that GltfAssetName will still use indices, which I've done previous research and thought on in the past and come to the conclusion that the way we use glTF indices is flawed. Indices are meant to be internal references, and anything that is supposed to be exposed from a glTF file is supposed to be named, according to the spec (emphasis mine in the following quote)
Whereas indices are used for internal glTF references, optional names are used for application-specific uses such as display. Any top-level glTF object MAY have a name string property for this purpose.
We should as such entertain a future PR that replaces index-based access with name-based access entirely, and let people who want to use index-based access load the Gltf asset to do so, effectively flipping the defaults from index-forward to name-forward. (I intend to propose one when I find the time).
| pub fn get_subasset( | ||
| &self, | ||
| label: impl Into<CowArc<'static, str>>, | ||
| subasset_name: impl Into<CowArc<'static, str>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Name" vs "label" is a bit of an interesting question. "Name" has a "canonical-ness" to it that "label" does not. Label was initially chosen because we thought we might eventually want to support multiple labels per asset.
In practice, that never ended up being very important, and label has served as a unique / canonical identifier. In that context, "name" makes more sense.
Additionally, with our current implementation and culture, I think subasset is clearly the better name than "labeled asset".
However we also have the upcoming "assets as entities" to consider. And as discussed above, Name is already a Bevy concept / component. Depending on how we choose to represent "asset entities" and "subasset entities", Name may or may not make sense in that context. I suspect it will (and the tooling benefits we get from that approach are extremely compelling), but that is uncharted territory.
Additionally, with "assets as entities" we might choose represent subassets as children. In that context, it might make more sense to call them "child assets" rather than subassets. Or we might choose to add a new SubassetOf Relationship (perhaps freeing up ChildOf for representing asset "scene hierarchies" directly). The "assets as entities" world might need a whole new set of entity / relationship APIs that replaces the current API.
The point is, we have big changes on the horizon that could very well affect the terminology here, and are likely to break these exact asset APIs. I'm not sure it makes sense to make these "superficial" breaking changes now. I'd rather direct this energy toward spec-ing out what the "assets as entities" data model / terminology looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm clicking approve as I agree with the general direction - I've always found the "labeled" asset terminology a bit confusing.
There's a couple of points I disagree with:
- I think
AssetPath::subasset_nameshould beAssetPath::subasset.- Being specific that it's a name is useful in some places.
- But with
AssetPathI think the context is clear, so brevity wins.
- I wouldn't change
GltfAssetLabel.- The behavior of glTF labels is contested and there's a small but non-zero chance it gets renamed again.
- So I'm maybe being too paranoid, but I think it's best to wait until that gets sorted. Or it should have saved for a follow up PR.
There's also an inconsistency between "subasset" and "sub-asset" in documentation. I'm not sure if this PR needs to fix it though.
Objective
Solution
labelinbevy_assetand rename it tosubassetas relevant.AssetPath::labeltoAssetPath::subasset_name.bevy_gltf(as its the main user of subassets in the bevy repo).Testing