-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Enforce read-only global transform #1460
Conversation
Are we sure there is no case when user code would need to mutate the global transform directly? |
As mentioned previously, I want to experiment removing the Transform component on ui bundles and directly set GlobalTransform. |
I don't know if this is what you would call a "valid use case," but in the past I was messing with But that wouldn't be needed if Edit: potentially also not needed if labels get added to these systems and text size is calculated prior to transform propagation? |
Perhaps create an extension trait that isnt Part of the Prelude and still allows setting the GlobalTransform. |
I think there are a few perspectives we can take here:
I included (3) mainly as a thought exercise. I think the added complexity isn't worth it, despite the consistent interface it provides. (1) and (2) are both "valid" in my mind, its just a matter of picking the tradeoffs we want. |
If we're using approach 1, Personally, I prefer approach 2: the simplicity of being able to manipulate a single component no matter whether or not we're in a hierarchy seems really valuable for refactors that you'll see during prototyping and the mental model is much simpler. I agree that approach 3 is far too elaborate for its benefits. |
That's already the case, if both are included you can continue to use Maybe it would be clearer by having a My preference is on the current approach, since the second one prevents (or makes expensive) an (uncommon) use case I care about. |
Yeah calling the "local transform"
// current
commands.spawn(SpriteBundle {
transform: Transform::from_xyz(1.0, 0.0, 0.0),
..Default::default()
})
// nested bundle
commands.spawn(SpriteBundle {
transform_bundle: TransformBundle {
transform: Transform::from_xyz(1.0, 0.0, 0.0),
..Default::default()
},
..Default::default()
}) Given the frequency of bundle usage vs bundle declarations, I don't think thats a worthwhile tradeoff (at least for core bundles). I guess editor workflows might benefit from a TransformBundle. And custom user bundles might sometimes prefer that (although I would personally never use that in my custom bundles for ergo reasons). GlobalTransform+Transform will be the common case no matter what, so its probably worth optimizing the ux for that case. In that context, GlobalTransform should never be set. We need to weight that against the benefits we get from standalone GlobalTransform use cases. I think we should enumerate those use cases and weigh their value against the "foot-gun reduction" this pr adds. |
What if Transforms were enums? enum Transform {
Local {
pub translation: Vec3,
pub rotation: Quat,
pub scale: Vec3,
},
Global {
pub translation: Vec3,
pub rotation: Quat,
pub scale: Vec3,
},
} Pros:
Cons:
Alternatively, we could use generics here, which would be very similar to the current situation, except the transform types would be addressed through a single generic transform type, something like: struct Transform<T: IsTransform> (T);
trait IsTransform {
// from_xyz, etc
}
struct Local {
pub translation: Vec3,
pub rotation: Quat,
pub scale: Vec3,
}
impl IsTransform for Local {}
struct Global {
pub translation: Vec3,
pub rotation: Quat,
pub scale: Vec3,
}
impl IsTransform for Global{} This would allow you to query the type of transform if needed for propagation and such, like Regardless, I think this would make it much more clear compared to having two separate component types, and documentation could be put in the |
Those are interesting ideas, but they don't solve the problem this pr is addressing (users have the ability to override GlobalTransform values set by the hiearchy system). The suggestions are more about "discoverability' (in the case of the enums) and code-size reduction / consistency (the generic). |
Making it difficult/obscure to change |
I should have given some more context to that comment. This was in response to your earlier suggestions
If we plan to change the structs/components as an alternative to this PR, such as your 3rd suggestion, I thought the ideas I brought up would be worth discussing. While those suggestions are more about "discoverability" as you said, they do have implications for the problems this PR solves. The enum route, for example, wouldn't have this issue because you would be guaranteed that there is only one type of Transform component present on an entity, and thus, you should be able to access and edit its fields. If these larger type changes are out of scope for this discussion, then I am in full agreement with @alice-i-cecile's comment. |
This isn't something that we want to ensure. The only two valid configurations are [GlobalTransform] and [GlobalTransform, LocalTransform]. LocalTransform by itself has no meaning. |
Yeah, the LocalTransform would need to be doubly sized in the case of the enum, to hold both a local and global transform in the LocalTransform variant. That would be encapsulated by only exposing setters/getters to the users of the API. This would come at the expense of the enum taking double the memory, and only being 50% memory efficient when using the global variant of the enum. The generic route wouldn't suffer from the memory downside though, as My goal with these suggestions is to address the root cause driving this PR. There are two ways an entity's transform can be expressed:
Currently, we have two independent components which do not make their relationship clear, nor do we use the type system to prevent misuse. I'm advocating for a variant of your "perspecting 3", which (I think) solves most of the cons, as well as:
Consider this tweaked version of the generic transform I suggested above: pub struct Transform<T: IsTransform> (T);
// IsTransform trait and impls not shown
struct Global {
translation: Vec3,
rotation: Quat,
scale: Vec3,
}
struct Local {
translation: Vec3,
rotation: Quat,
scale: Vec3,
global_transform: Option<Global>
} This lets us:
|
An issue is that we could not query over GobalTransform |
You can though! You would query over The added benefit is now you know when you are reading from a transform that has not yet propagated, because this design will return a This could make queries messy though, as you may need to specify both concrete versions of the type for the query, though there might be a way around this using trait objects. |
I'm all for being able to query traits when it makes sense, but I don't think it does here. Transforms are too fundamentals, they need to be as performant as possible. Even the extra discriminant check of an enum should be avoided IMO. |
Yeah, I don't think enums are the way to go, which is why I've been continuing down the generics route. Or are you concerned that the Trait objects were a suggestion to aid the UX of querying |
To me, this is one of the more serious bits of confusion with the current API. When an entity has no parent, |
Basically the only reason we don't currently do a combined transform is performance. Putting them together means that we're pulling in the global transform values every time a local transform value is needed (and local-only is the common case). This is less cache friendly. I also don't see the value of the generics. |
So, my overall feeling here is that In the absence of any performance concerns at all, it feels like the most natural API for this would be: pub struct TransformData {
translation: Vec3,
rotation: Quat,
scale: Vec3,
}
pub struct Transform{
pub global: TransformData
local: Option<TransformData>
}
// Getter and setter methods for the local fields of Transform
// When you set the local field of Transform, the global field is automatically set to match Unfortunately, we're also quite performance-limited here, because of how central and relatively costly transform components and updates are. Taking the example of a realistic graphics-intensive game though, almost all Transform entities will be part of an object hierarchy of some sort: as part of a UI, as a piece of a whole that gets animated, as part of a graphics effect or so on. Of those within the object hierarchy, only those objects at the very start of the hierarchy will not have a local transform. These, by the nature of trees, will be relatively rare. As a result, I'm not terribly concerned about the extra memory allocation of doubling the size of the Transform type in terms of bulk storage. After reading @cart's most recent comment though, I'm afraid he might be right: coupling the two into a single component inherently causes more data to have to be fetched when we only care about a single field, and only caring about a single field (on a per-system, rather than per-entity basis) is very common. |
As a follow-up to this: what if every |
Yes, this leads us back into the land of AoS vs. SoA and the cache tradeoffs. At the risk of derailing this conversation, this makes me think, again, that what we need is a way to express component dependencies. This is something I've brought up in the past with other component types. All of this discourse and PR revolve around the fact that there is some relationship between the transform component types that we wish to express to users of the API. Maybe this is something that can be added in component bundles? |
If we had full archetype invariants (#1481), what I would do is:
|
Yeah I think we've clearly isolated the core issue. Heres my take: allowing GlobalTransform to be set in non-hierarchical contexts creates confusion. A "combined transform" doesn't solve that inherently (unless we abstract that out internally and pick what is set based on the context, which is bad news bears in my opinion). I think "GlobalTransform and LocalTransform must appear together and always behave the same way" follows from that. Archetype invariants are a nice-to-have that would help enforce that constraint, but they aren't needed to start following it / assuming it. If GlobalTransform and LocalTransform must behave the same way in all contexts, and GlobalTransform cannot be written to directly in hierarchy, then that means we should make GlobalTransform read-only (regardless of context). This has the added benefit of disallowing "incorrect" behavior if someone only adds a From there, that means that contexts that don't want "hierarchical transforms" will need to use a different component. Cases like UI could either create a new UiTransform component or extend an existing component like Node. Thoughts? |
Agreed.
I like this.
Yes: this is the correct behavior. We should be sure that it is trivial to render entities with only a simple |
I'm not sure I agree. Adding another transform type for "simple" use cases makes the system harder to reason about. Then we have 3 types for people to sift through (GlobalTransform, LocalTransform, and SimpleTransform). What happens when someone Parents a SimpleTransform to a SimpleTransform? Or a SimpleTransform to a GlobalTransform? It also gets more complicated because SimpleTransform and LocalTransform are different types and must be queried for separately. Imo its simpler to just say "Add both LocalTransform and GlobalTransform" (or TransformBundle) if you want your entity to have a transform. Its just one extra component and it gives us consistency. (Also to be clear, I still think "LocalTransform" should be called |
I guess we could also consider an alternative: we could consider treating GlobalTransform as a read-only product of hierarchy, and selectively use it in place of Transform when it is present. It would require some additional internal complexity for data binding, but its something users wouldn't need to care about. The reason we've avoided that in the past is that it makes it hard to query for the "source of truth" world space position. Users need to check for the presence of GlobalTransform and use that in place of Transform when appropriate. |
So yeah im still biased toward (Transform, GlobalTransform) pairs for userspace and one-off newtypes for things that can't operate under those constraints (which should be relatively uncommon) |
Lol this is hard. I'm sort of coming around to the "make GlobalTransform hard to set but not impossible" idea put forward by @MinerSebas. That would enable ui-like-scenarios (where GlobalTransform is just a derived value, much like it is in normal hiearchy cases) without new types. |
struct GlobalTransform {
transform: Transform,
}
impl GlobalTransform {
pub fn translation(&self) -> Vec3 {
self.transform.translation
}
/// Mutably access the internal world space translation
/// # WARNING
/// this is rarely what you want. (explain why here)
pub fn translation_internal_mut(&mut self) -> &mut Vec3 {
&mut self.transform.translation
}
} Edit: reusing Transform probably isn't worth it here |
I like that draft: clearly signals how it should be used without a lot of overhead and doesn't force users to think hard about how to query for a source of truth. I agree that reusing Transform just makes things less clear though. This change plus a bit better documentation plus #1471 should greatly reduce the footguns involved in prototyping with transforms. |
I could live with that, but I'll try to make my case one last time.
That's all. It's simple, efficient, extensible and standard. I don't find it incoherent or unclear. If some beginners trip up on it, it only shows our lack of documentation, let's fix that instead. |
Solid points. Thanks pushing back. This issue is not clear cut and adding restrictions does complicate the mental model in some ways. This change is also breaking, so I think rushing it in would be a mistake given how much back and forth there is (I've already changed my mind a couple of times in this thread). I'm thinking we should hold off for now in favor of doc improvements and continue this conversation in a Github Discussion. Any objections? |
@cart that seems sensible. We can close this PR, and link this thread in the discussion. Then, I'll make an issue for better docs and give it a 0.5 milestone so we can be sure it's out in time. We should expect another influx of new users once 0.5 goes live, and so we'll be able to see how well it works. |
Yep, this sounds reasonable. |
This PR makes the GlobalTransform fields private to enforce read-only behavior which is expected by the transform propagation.